Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 2/14/2024 10:02 AM, Ville Syrjälä wrote:
> On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote:
> > On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
> > > On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > > > intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
> > > > Lets move this to drm_dp_helper to achieve this.
> > > >
> > > > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > >
> > > My preference would be to have packing functions in
> > > drivers/video/hdmi.c, as we already have
> > > hdmi_audio_infoframe_pack_for_dp() there.
> >
> > My preference is drm_dp_helper because it already has some VSC SDP stuff
> > and after discussion with Ville on IRC, I decided to post it this way.
> >
> > hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the
> > hdmi audio infoframe fields were re-used and packed into a DP SDP
> > thereby re-using the existing struct hdmi_audio_infoframe .
> >
> > This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct
> > dp_sdp both of which had prior usages already in this file.
> >
> > So it all adds up and makes sense to me to be in this file.
> >
> > I will let the other DRM core maintainers comment on this.
> >
> > Ville, Jani?
>
> Yeah, I'm not sure bloating the (poorly named) hdmi.c with all
> SDP stuff is a great idea. Since other related stuff already
> lives in the drm_dp_helper.c that seems reasonable to me at this
> time. And if we get a decent amount of this then probably all
> DP SDP stuff should be extracted into its own file.

Yes, thanks.

> There are of course a few overlaps here andthere (the audio SDP
> I guess, and the CTA infoframe SDP). But I'm not sure that actually
> needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c
> deal with the actual CTA-861 stuff and then have the DP SDP code
> wrap that up in its own thing externally? Dunno, haven't really
> looked at the details.

Thats a good way to look at it. this packing is from DP spec and not CTA 
so makes more sense to be in this file.

In that case, R-b?

> > > > ---
> > > >    drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
> > > >    drivers/gpu/drm/i915/display/intel_dp.c | 73 +----------------------
> > > >    include/drm/display/drm_dp_helper.h     |  3 +
> > > >    3 files changed, 84 insertions(+), 70 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > > > index b1ca3a1100da..066cfbbf7a91 100644
> > > > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > > > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > > > @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
> > > >    }
> > > >    EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
> > > >
> > > > +/**
> > > > + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
> > > > + * @vsc: vsc sdp initialized according to its purpose as defined in
> > > > + *       table 2-118 - table 2-120 in DP 1.4a specification
> > > > + * @sdp: valid handle to the generic dp_sdp which will be packed
> > > > + * @size: valid size of the passed sdp handle
> > > > + *
> > > > + * Returns length of sdp on success and error code on failure
> > > > + */
> > > > +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> > > > +                           struct dp_sdp *sdp, size_t size)
> > >
> > > I know that you are just moving the function. Maybe there can be
> > > patch#2, which drops the size argument? The struct dp_sdp already has
> > > a defined size. The i915 driver just passes sizeof(sdp), which is more
> > > or less useless.
> >
> > Yes this is a valid point, I also noticed this. I can post it on top of
> > this once we get an agreement and ack on this patch first.
> >
> > > > +{
> > > > +       size_t length = sizeof(struct dp_sdp);
> > > > +
> > > > +       if (size < length)
> > > > +               return -ENOSPC;
> > > > +
> > > > +       memset(sdp, 0, size);
> > > > +
> > > > +       /*
> > > > +        * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > > > +        * VSC SDP Header Bytes
> > > > +        */
> > > > +       sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> > > > +       sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> > > > +       sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> > > > +       sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> > > > +
> > > > +       if (vsc->revision == 0x6) {
> > > > +               sdp->db[0] = 1;
> > > > +               sdp->db[3] = 1;
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
> > > > +        * Format as per DP 1.4a spec and DP 2.0 respectively.
> > > > +        */
> > > > +       if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
> > > > +               goto out;
> > > > +
> > > > +       /* VSC SDP Payload for DB16 through DB18 */
> > > > +       /* Pixel Encoding and Colorimetry Formats  */
> > > > +       sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> > > > +       sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> > > > +
> > > > +       switch (vsc->bpc) {
> > > > +       case 6:
> > > > +               /* 6bpc: 0x0 */
> > > > +               break;
> > > > +       case 8:
> > > > +               sdp->db[17] = 0x1; /* DB17[3:0] */
> > > > +               break;
> > > > +       case 10:
> > > > +               sdp->db[17] = 0x2;
> > > > +               break;
> > > > +       case 12:
> > > > +               sdp->db[17] = 0x3;
> > > > +               break;
> > > > +       case 16:
> > > > +               sdp->db[17] = 0x4;
> > > > +               break;
> > > > +       default:
> > > > +               WARN(1, "Missing case %d\n", vsc->bpc);
> > > > +               return -EINVAL;
> > > > +       }
> > > > +
> > > > +       /* Dynamic Range and Component Bit Depth */
> > > > +       if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> > > > +               sdp->db[17] |= 0x80;  /* DB17[7] */
> > > > +
> > > > +       /* Content Type */
> > > > +       sdp->db[18] = vsc->content_type & 0x7;
> > > > +
> > > > +out:
> > > > +       return length;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
> > > > +
> > > >    /**
> > > >     * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
> > > >     * @dpcd: DisplayPort configuration data
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index f5ef95da5534..e94db51aeeb7 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
> > > >           return false;
> > > >    }
> > > >
> > > > -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> > > > -                                    struct dp_sdp *sdp, size_t size)
> > > > -{
> > > > -       size_t length = sizeof(struct dp_sdp);
> > > > -
> > > > -       if (size < length)
> > > > -               return -ENOSPC;
> > > > -
> > > > -       memset(sdp, 0, size);
> > > > -
> > > > -       /*
> > > > -        * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > > > -        * VSC SDP Header Bytes
> > > > -        */
> > > > -       sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> > > > -       sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> > > > -       sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> > > > -       sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> > > > -
> > > > -       if (vsc->revision == 0x6) {
> > > > -               sdp->db[0] = 1;
> > > > -               sdp->db[3] = 1;
> > > > -       }
> > > > -
> > > > -       /*
> > > > -        * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
> > > > -        * Format as per DP 1.4a spec and DP 2.0 respectively.
> > > > -        */
> > > > -       if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
> > > > -               goto out;
> > > > -
> > > > -       /* VSC SDP Payload for DB16 through DB18 */
> > > > -       /* Pixel Encoding and Colorimetry Formats  */
> > > > -       sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> > > > -       sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> > > > -
> > > > -       switch (vsc->bpc) {
> > > > -       case 6:
> > > > -               /* 6bpc: 0x0 */
> > > > -               break;
> > > > -       case 8:
> > > > -               sdp->db[17] = 0x1; /* DB17[3:0] */
> > > > -               break;
> > > > -       case 10:
> > > > -               sdp->db[17] = 0x2;
> > > > -               break;
> > > > -       case 12:
> > > > -               sdp->db[17] = 0x3;
> > > > -               break;
> > > > -       case 16:
> > > > -               sdp->db[17] = 0x4;
> > > > -               break;
> > > > -       default:
> > > > -               MISSING_CASE(vsc->bpc);
> > > > -               break;
> > > > -       }
> > > > -       /* Dynamic Range and Component Bit Depth */
> > > > -       if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> > > > -               sdp->db[17] |= 0x80;  /* DB17[7] */
> > > > -
> > > > -       /* Content Type */
> > > > -       sdp->db[18] = vsc->content_type & 0x7;
> > > > -
> > > > -out:
> > > > -       return length;
> > > > -}
> > > > -
> > > >    static ssize_t
> > > >    intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915,
> > > >                                            const struct hdmi_drm_infoframe *drm_infoframe,
> > > > @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
> > > >
> > > >           switch (type) {
> > > >           case DP_SDP_VSC:
> > > > -               len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
> > > > -                                           sizeof(sdp));
> > > > +               len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
> > > > +                                         sizeof(sdp));
> > > >                   break;
> > > >           case HDMI_PACKET_TYPE_GAMUT_METADATA:
> > > >                   len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
> > > > @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
> > > >           struct dp_sdp sdp = {};
> > > >           ssize_t len;
> > > >
> > > > -       len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
> > > > +       len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
> > > >
> > > >           if (drm_WARN_ON(&dev_priv->drm, len < 0))
> > > >                   return;
> > > > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > > > index 863b2e7add29..f8db34a2f7a5 100644
> > > > --- a/include/drm/display/drm_dp_helper.h
> > > > +++ b/include/drm/display/drm_dp_helper.h
> > > > @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
> > > >                          int bpp_x16, unsigned long flags);
> > > >    int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
> > > >
> > > > +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> > > > +                           struct dp_sdp *sdp, size_t size);
> > > > +
> > > >    #endif /* _DRM_DP_HELPER_H_ */
> > > > --
> > > > 2.34.1