On Fri, Feb 07, 2025 at 05:31:20PM -0800, Abhinav Kumar wrote:
>
>
> On 2/7/2025 4:27 PM, Dmitry Baryshkov wrote:
> > Extend the driver to send SPD and HDMI Vendor Specific InfoFrames.
> >
> > While the HDMI block has special block to send HVS InfoFrame, use
> > GENERIC0 block instead. VENSPEC_INFO registers pack frame data in a way
> > that requires manual repacking in the driver, while GENERIC0 doesn't
> > have such format requirements. The msm-4.4 kernel uses GENERIC0 to send
> > HDR InfoFrame which we do not at this point anyway.
> >
>
> True that GENERIC_0/1 packets can be used for any infoframe. But because we
> have so many of them, thats why when there are dedicated registers for some
> of them, we use them to save the GENERIC0 ones for others.
True
> Lets take a case where we want to send HVSIF, SPD and HDR together for the
> same frame, then we run out as there are no HDR specific infoframe registers
> we can use. Is the expectation that we will migrate to VENSPEC_INFO regs for
> HVSIF when we add HDR support?
Most likely, yes. That would be a part of the HDR support. At the same
time note, we can use GENERIC0 to send new HFVS InfoFrames defined in
HDMI 2.x (once Linux gets support for that). At the same time we can not
use VENSPEC_INFO to send those.
I can imagine that the driver will have to switch GENERIC1 between HDR
(if required) and SPD (in all other cases).
>
> Also from a validation standpoint, I guess to really validate this change
> you need an analyzer which decodes the HVSIF. So was this mostly sanity
> tested at this pointed to make sure that the sink just comes up?
Vertex 2 dumps received HVS InfoFrame, so the InfoFrame contents has
been validated (validated SPD, AUD, HVS and AVI frames).
>
> > Acked-by: Maxime Ripard <mripard@xxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 93 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 93 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > index 15ab0858105328c2f774ec1f79423614bbbaeb41..aee75eee3d4244cd95e44df46d65b8e3e53de735 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > @@ -69,6 +69,8 @@ static void power_off(struct drm_bridge *bridge)
> > }
> > #define AVI_IFRAME_LINE_NUMBER 1
> > +#define SPD_IFRAME_LINE_NUMBER 1
> > +#define VENSPEC_IFRAME_LINE_NUMBER 3
> > static int msm_hdmi_config_avi_infoframe(struct hdmi *hdmi,
> > const u8 *buffer, size_t len)
> > @@ -142,6 +144,74 @@ static int msm_hdmi_config_audio_infoframe(struct hdmi *hdmi,
> > return 0;
> > }
> > +static int msm_hdmi_config_spd_infoframe(struct hdmi *hdmi,
> > + const u8 *buffer, size_t len)
> > +{
> > + u32 buf[7] = {};
> > + u32 val;
> > + int i;
> > +
> > + if (len != HDMI_INFOFRAME_SIZE(SPD) || len - 3 > sizeof(buf)) {
> > + DRM_DEV_ERROR(&hdmi->pdev->dev,
> > + "failed to configure SPD infoframe\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* checksum gets written together with the body of the frame */
> > + hdmi_write(hdmi, REG_HDMI_GENERIC1_HDR,
> > + buffer[0] |
> > + buffer[1] << 8 |
> > + buffer[2] << 16);
> > +
> > + memcpy(buf, &buffer[3], len - 3);
> > +
> > + for (i = 0; i < ARRAY_SIZE(buf); i++)
> > + hdmi_write(hdmi, REG_HDMI_GENERIC1(i), buf[i]);
> > +
> > + val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
> > + val |= HDMI_GEN_PKT_CTRL_GENERIC1_SEND |
> > + HDMI_GEN_PKT_CTRL_GENERIC1_CONT |
> > + HDMI_GEN_PKT_CTRL_GENERIC1_LINE(SPD_IFRAME_LINE_NUMBER);
> > + hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
> > +
> > + return 0;
> > +}
> > +
> > +static int msm_hdmi_config_hdmi_infoframe(struct hdmi *hdmi,
> > + const u8 *buffer, size_t len)
>
> msm_hdmi_config_hvsif_infoframe() to be more clear?
Well, all DRM Connector callback uses just 'hdmi' here, so there is no
reason to deviate from that.
--
With best wishes
Dmitry
