linux-kernel - Re: [PATCH v2 3/9] drm/bridge: ite-it6263: handle unsupported InfoFrames

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6db3337a-ba59-4901-b0e2-2b0b93c8a4e7@nxp.com>
Date: Mon, 29 Sep 2025 17:10:59 +0800
From: Liu Ying <victor.liu@....com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Rob Clark <robin.clark@....qualcomm.com>, Dmitry Baryshkov
 <lumag@...nel.org>, Abhinav Kumar <abhinav.kumar@...ux.dev>,
 Jessica Zhang <jessica.zhang@....qualcomm.com>, Sean Paul <sean@...rly.run>,
 Marijn Suijten <marijn.suijten@...ainline.org>,
 Sandy Huang <hjc@...k-chips.com>, Heiko Stübner
 <heiko@...ech.de>, Andy Yan <andy.yan@...k-chips.com>,
 Samuel Holland <samuel@...lland.org>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 freedreno@...ts.freedesktop.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v2 3/9] drm/bridge: ite-it6263: handle unsupported
 InfoFrames

On 09/29/2025, Dmitry Baryshkov wrote:
> On Mon, Sep 29, 2025 at 03:56:31PM +0800, Liu Ying wrote:
>> On 09/28/2025, Dmitry Baryshkov wrote:
>>> Make hdmi_write_hdmi_infoframe() and hdmi_clear_infoframe() callbacks
>>> return -EOPNOTSUPP for unsupported InfoFrames and make sure that
>>> atomic_check() callback doesn't allow unsupported InfoFrames to be
>>> enabled.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
>>> ---
>>>  drivers/gpu/drm/bridge/ite-it6263.c | 27 +++++++++++++++++++++++++--
>>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/ite-it6263.c b/drivers/gpu/drm/bridge/ite-it6263.c
>>> index 2eb8fba7016cbf0dcb19aec4ca8849f1fffaa64c..cf3d76d748dde51e93b2b19cc2cbe023ca2629b8 100644
>>> --- a/drivers/gpu/drm/bridge/ite-it6263.c
>>> +++ b/drivers/gpu/drm/bridge/ite-it6263.c
>>> @@ -26,6 +26,7 @@
>>>  #include <drm/drm_crtc.h>
>>>  #include <drm/drm_edid.h>
>>>  #include <drm/drm_of.h>
>>> +#include <drm/drm_print.h>
>>>  #include <drm/drm_probe_helper.h>
>>>  
>>>  /* -----------------------------------------------------------------------------
>>> @@ -772,7 +773,7 @@ static int it6263_hdmi_clear_infoframe(struct drm_bridge *bridge,
>>>  		regmap_write(it->hdmi_regmap, HDMI_REG_PKT_NULL_CTRL, 0);
>>>  		break;
>>>  	default:
>>> -		dev_dbg(it->dev, "unsupported HDMI infoframe 0x%x\n", type);
>>> +		return -EOPNOTSUPP;
>>>  	}
>>>  
>>>  	return 0;
>>> @@ -812,13 +813,35 @@ static int it6263_hdmi_write_infoframe(struct drm_bridge *bridge,
>>>  			     ENABLE_PKT | REPEAT_PKT);
>>>  		break;
>>>  	default:
>>> -		dev_dbg(it->dev, "unsupported HDMI infoframe 0x%x\n", type);
>>> +		return -EOPNOTSUPP;
>>>  	}
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +static int it6263_bridge_atomic_check(struct drm_bridge *bridge,
>>> +				      struct drm_bridge_state *bridge_state,
>>> +				      struct drm_crtc_state *crtc_state,
>>> +				      struct drm_connector_state *conn_state)
>>> +{
>>> +	/* not supported by the driver */
>>> +	conn_state->hdmi.infoframes.spd.set = false;
>>> +
>>> +	/* should not happen, audio support not enabled */
>>> +	if (drm_WARN_ON_ONCE(bridge->encoder->dev,
>>> +			     conn_state->connector->hdmi.infoframes.audio.set))
>>
>> Maybe use drm_err_once() instead to provide the reason for the warning in
>> a string?
> 
> I can change all of them to drm_err_once(), sure.

With those changed,
Acked-by: Liu Ying <victor.liu@....com>

> 
>>
>>> +		return -EOPNOTSUPP;
>>
>> As this check could return error, it should be moved before
>> 'conn_state->hdmi.infoframes.spd.set = false;' to gain a little performance.
> 
> I'd say, it would be negligible.

Fine, up to you :)

> 
>>
>>> +
>>> +	/* should not happen, HDR support not enabled */
>>> +	if (drm_WARN_ON_ONCE(bridge->encoder->dev,
>>> +			     conn_state->hdmi.infoframes.hdr_drm.set))
>>> +		return -EOPNOTSUPP;
>>
>> I don't think IT6263 chip supports DRM infoframe.  The drm_WARN_ON_ONCE()
>> call could make driver readers think that DRM infoframe could be enabled
>> in the future as audio infoframe has the same warning and IT6263 chip does
>> support audio infoframe.  So, maybe:
>>
>> /* IT6263 chip doesn't support DRM infoframe. */
>> conn_state->hdmi.infoframes.hdr_drm.set = false;
> 
> I'd rather not do that. My point here was that the driver can not end up
> in the state where this frame is enabled, because it can only happen if
> the driver sets max_bpc (which it doesn't). Likewise Audio InfoFrame can
> not get enabled because the driver doesn't call into audio functions. On
> the contrary, SPD frame (or HDMI in several other drivers) can be
> enabled by the framework, so we silently turn then off here.

Ditto.

> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct drm_bridge_funcs it6263_bridge_funcs = {
>>> +	.atomic_check = it6263_bridge_atomic_check,
>>>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>>  	.atomic_reset = drm_atomic_helper_bridge_reset,
>>>
>>
>>
>> -- 
>> Regards,
>> Liu Ying
> 


-- 
Regards,
Liu Ying