Messages in this thread | | | Date | Thu, 14 Dec 2023 17:34:59 +0100 | Subject | Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range | From | Alex Bee <> |
| |
Hi Heiko, Hi Maxime,
Am 14.12.23 um 12:36 schrieb Maxime Ripard: > On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote: >> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee: >>> Hi Maxime >>> >>> Am 14.12.23 um 08:56 schrieb Maxime Ripard: >>>> Hi Alex, >>>> >>>> Thanks for working on this! >>>> >>>> On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote: >>>>> The display controller will always give full range RGB regardless of the >>>>> mode set, but HDMI requires certain modes to be transmitted in limited >>>>> range RGB. This is especially required for HDMI sinks which do not support >>>>> non-standard quantization ranges. >>>>> >>>>> This enables color space conversion for those modes and sets the >>>>> quantization range accordingly in the AVI infoframe. >>>>> >>>>> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support") >>>>> Signed-off-by: Alex Bee <knaerzche@gmail.com> >>>>> --- >>>>> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------ >>>>> 1 file changed, 32 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c >>>>> index 345253e033c5..32626a75723c 100644 >>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c >>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c >>>>> @@ -33,6 +33,7 @@ struct hdmi_data_info { >>>>> unsigned int enc_in_format; >>>>> unsigned int enc_out_format; >>>>> unsigned int colorimetry; >>>>> + bool rgb_limited_range; >>>>> }; >>>>> >>>>> struct inno_hdmi_i2c { >>>>> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi, >>>>> else >>>>> frame.avi.colorspace = HDMI_COLORSPACE_RGB; >>>>> >>>>> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) { >>>>> + drm_hdmi_avi_infoframe_quant_range(&frame.avi, >>>>> + &hdmi->connector, mode, >>>>> + hdmi->hdmi_data.rgb_limited_range ? >>>>> + HDMI_QUANTIZATION_RANGE_LIMITED : >>>>> + HDMI_QUANTIZATION_RANGE_FULL); >>>>> + } else { >>>>> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT; >>>>> + frame.avi.ycc_quantization_range = >>>>> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED; >>>>> + } >>>>> + >>>>> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0); >>>>> } >>>>> >>>>> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi) >>>>> if (data->enc_in_format == data->enc_out_format) { >>>>> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) || >>>>> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) { >>>>> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1); >>>>> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value); >>>>> - >>>>> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL, >>>>> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP, >>>>> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) | >>>>> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE)); >>>>> - return 0; >>>>> + if (data->enc_in_format == HDMI_COLORSPACE_RGB && >>>>> + data->enc_out_format == HDMI_COLORSPACE_RGB && >>>>> + hdmi->hdmi_data.rgb_limited_range) { >>>>> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT; >>>>> + auto_csc = AUTO_CSC_DISABLE; >>>>> + c0_c2_change = C0_C2_CHANGE_DISABLE; >>>>> + csc_enable = v_CSC_ENABLE; >>>>> + } else { >>>>> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1); >>>>> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value); >>>>> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL, >>>>> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP, >>>>> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) | >>>>> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE)); >>>>> + return 0; >>>>> + } >>>>> } >>>>> } >>>>> >>>>> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi, >>>>> else >>>>> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709; >>>>> >>>>> + hdmi->hdmi_data.rgb_limited_range = >>>>> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED; >>>>> + >>>> This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38): >>>> https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/ >>> I'm aware of that and I mentioned it in the cover letter. Your series is >>> not merged yet and it didn't get much feedback so far. What is the >>> status there? Especially because you are removing things from inno-hdmi >>> driver (which aren't really required to remove there) which will I have >>> to reintroduce. >> Sadly I haven't found the time to look closer at Maxime's series so far, >> but I got the impression that it separates into multiple cleanup steps >> for a number of controllers. > Yeah, one of the previous version comment was to support more > controllers than vc4, which is fair. So I ended up reworking and > converting multiple controllers, but most of the clean up changes can be > applied outside of that series just fine. > > I just didn't find someone to test / review them yet :)
I'm not exactly sure how to proceed here. Do you want me to:
- base my patches on top of Maxime's series and reintroduce csc things (of course only those which touch inno-hdmi w/o the framwork changes)
- only apply the patches that do not touch csc things and base my series on top of that
- adapt Maxime's patches so that RGB full to RGB limited stays in there
- wait with resend until Maxime's series is merged and reintroduce csc after that
- something else
?
Please advise.
Thanks,
Alex
> Maxime
| |