| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a8f13e19-7bb3-4a7f-a264-1e1ede1864e0@gmail.com>
Date: Thu, 14 Dec 2023 15:05:59 +0100
From: Alex Bee <knaerzche@...il.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Sandy Huang <hjc@...k-chips.com>,
Heiko Stübner <heiko@...ech.de>,
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
Am 14.12.23 um 12:33 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
>> 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.
> Sorry, I missed that part.
>
>> Your series is not merged yet and it didn't get much feedback so far.
>> What is the status there?
> It didn't have much reviews, but I'll hope to change that. For the
> patches 22 to 38 though, it doesn't really matter. Those changes are
> self-contained and can be applied as is outside of the series.
>
>> Especially because you are removing things from inno-hdmi driver (which
>> aren't really required to remove there) which will I have to reintroduce.
> I'm not entirely sure which part I remove that are actually going to be
> used here.
I'm refering to [PATCH v5 33/44] which completly removes csc coeffs but
this series needs the CSC_RGB_0_255_TO_RGB_16_235_8BIT coeffs and
[PATCH v5 29/44] which removes writing csc_coeffs to the hardware.
>
>>> I would appreciate if you could test and merge them into your series.
>>>
>>> In particular, there's no need to store the range here: enc_out_format
>> rgb_limited_range is currently not only used for csc, but also for for
>> infoframe creation. So it makes sense to have this stored to avoid calling
>> drm_default_rgb_quant_range twice.
> You're right, I missed one. Still, it shouldn't be stored in the
> hdmi_data_info structure, it's tied to the mode, and the mode is part of
> the state, so it's not a property to a given device, but it's tied to
> the connector state.
>
> So if you want to do so, you should really create a custom state
> structure and store the range there, just like vc4 is doing for example.
OK - I'll check.
Alex
>
> Maxime
Powered by blists - more mailing lists