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))
+ return -EOPNOTSUPP;
+
+ /* should not happen, HDR support not enabled */
+ if (drm_WARN_ON_ONCE(bridge->encoder->dev,
+ conn_state->hdmi.infoframes.hdr_drm.set))
+ return -EOPNOTSUPP;
+
+ 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,
--
2.47.3On 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?
> + 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.
> +
> + /* 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;
> +
> + 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
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.
>
> > + 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.
>
> > +
> > + /* 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.
>
> > +
> > + 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
--
With best wishes
Dmitry
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
© 2016 - 2025 Red Hat, Inc.