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

[Liu Ying <victor.liu@nxp.com>]

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@oss.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@nxp.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