[PATCH v4 13/19] drm/msm/dp: add VSC SDP support for YUV420 over DP

[Dmitry Baryshkov]

On Tue, 20 Feb 2024 at 19:55, Paloma Arellano <quic_parellan at quicinc.com> wrote:
>
>
> On 2/17/2024 12:56 AM, Dmitry Baryshkov wrote:
> > On Sat, 17 Feb 2024 at 01:03, Paloma Arellano <quic_parellan at quicinc.com> wrote:

> >> +       }
> >> +
> >> +       panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> >> +       catalog = panel->catalog;
> >> +       dp_mode = &dp_panel->dp_mode;
> >> +
> >> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
> >> +
> >> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
> >> +       vsc_sdp_data.sdp_type = DP_SDP_VSC;
> >> +       vsc_sdp_data.revision = 0x05;
> >> +       vsc_sdp_data.length = 0x13;
> >> +
> >> +       /* VSC SDP Payload for DB16 */
> >> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
> >> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
> >> +
> >> +       /* VSC SDP Payload for DB17 */
> >> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
> >> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
> >> +
> >> +       /* VSC SDP Payload for DB18 */
> >> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
> >> +
> >> +       len = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &vsc_sdp, header);
> >> +       if (len < 0) {
> >> +               DRM_ERROR("unable to pack vsc sdp\n");
> >> +               return len;
> >> +       }
> > So, at this point we have the header data both in vsc_sdp.sdp_header
> > and in the packed header. The relationship between them becomes less
> > obvious. Could you please pack dp_sdp_header into u32[2] just before
> > writing it? It really makes little sense to pass both at the same
> > time.
>
>
> Just want to clear some stuff up. Do you want to call the
> dp_utils_pack_sdp_header() function right before
> dp_catalog_panel_send_vsc_sdp()? The point of putting the
> dp_utils_pack_sdp_header() function inside dp_utils_pack_vsc_sdp() is so
> that all of the packing could be in the same location. Although I agree
> that we are passing the same values twice, I believe that having it the
> way it is currently is better since it shows that the
> drm_dp_vsc_sdp_pack() and dp_utils_pack_sdp_header() are related since
> they're packing the data to the set of GENERIC0 registers.

I'm perfectly fine with dp_utils_pack_sdp_header() being called from
within dp_catalog_panel_send_vsc_sdp(). This way you are not passing
extra data and it is perfectly clear how the SDP header is handled
before being written to the hardware.


-- 
With best wishes
Dmitry