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 <[email protected]> >>> --- >>> 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 <[email protected]> > >> >>> + 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
