[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3f970571-f454-4615-8234-64ea68a3d63c@gmail.com>
Date: Thu, 14 Dec 2023 17:34:59 +0100
From: Alex Bee <knaerzche@...il.com>
To: Maxime Ripard <mripard@...nel.org>,
Heiko Stübner <heiko@...ech.de>
Cc: Sandy Huang <hjc@...k-chips.com>, Andy Yan <andyshrk@....com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI
quantization range
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@...il.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
Powered by blists - more mailing lists