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

[Paloma Arellano]

On 2/20/2024 10:09 AM, Dmitry Baryshkov wrote:
> 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.


Ack. Sounds good, I'll implement it that way


>