[PATCH 11/17] drm/msm/dp: add VSC SDP support for YUV420 over DP

[Dmitry Baryshkov]

On Sun, 28 Jan 2024 at 07:34, Paloma Arellano <quic_parellan at quicinc.com> wrote:
>
>
> On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote:
> > On 25/01/2024 21:38, Paloma Arellano wrote:
> >> Add support to pack and send the VSC SDP packet for DP. This therefore
> >> allows the transmision of format information to the sinks which is
> >> needed for YUV420 support over DP.
> >>
> >> Signed-off-by: Paloma Arellano <quic_parellan at quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/msm/dp/dp_catalog.h |   4 +
> >>   drivers/gpu/drm/msm/dp/dp_ctrl.c    |   4 +
> >>   drivers/gpu/drm/msm/dp/dp_panel.c   |  47 +++++++++
> >>   drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
> >>   5 files changed, 205 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index c025786170ba5..7e4c68be23e56 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -29,6 +29,9 @@
> >>     #define DP_INTF_CONFIG_DATABUS_WIDEN     BIT(4)
> >>   +#define DP_GENERIC0_6_YUV_8_BPC        BIT(0)
> >> +#define DP_GENERIC0_6_YUV_10_BPC    BIT(1)
> >> +
> >>   #define DP_INTERRUPT_STATUS1 \
> >>       (DP_INTR_AUX_XFER_DONE| \
> >>       DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \
> >> @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct
> >> dp_catalog *dp_catalog)
> >>       return 0;
> >>   }
> >>   +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog
> >> *dp_catalog)
> >> +{
> >> +    struct dp_catalog_private *catalog;
> >> +    u32 header, parity, data;
> >> +    u8 bpc, off = 0;
> >> +    u8 buf[SZ_128];
> >> +
> >> +    if (!dp_catalog) {
> >> +        pr_err("invalid input\n");
> >> +        return;
> >> +    }
> >> +
> >> +    catalog = container_of(dp_catalog, struct dp_catalog_private,
> >> dp_catalog);
> >> +
> >> +    /* HEADER BYTE 1 */
> >> +    header = dp_catalog->sdp.sdp_header.HB1;
> >> +    parity = dp_catalog_calculate_parity(header);
> >> +    data   = ((header << HEADER_BYTE_1_BIT) | (parity <<
> >> PARITY_BYTE_1_BIT));
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_0, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >> +
> >> +    /* HEADER BYTE 2 */
> >> +    header = dp_catalog->sdp.sdp_header.HB2;
> >> +    parity = dp_catalog_calculate_parity(header);
> >> +    data   = ((header << HEADER_BYTE_2_BIT) | (parity <<
> >> PARITY_BYTE_2_BIT));
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_1, data);
> >> +
> >> +    /* HEADER BYTE 3 */
> >> +    header = dp_catalog->sdp.sdp_header.HB3;
> >> +    parity = dp_catalog_calculate_parity(header);
> >> +    data   = ((header << HEADER_BYTE_3_BIT) | (parity <<
> >> PARITY_BYTE_3_BIT));
> >> +    data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1);
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_1, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >
> > This seems to be common with the dp_audio code. Please extract this
> > header writing too.
> These are two different sdp's. audio and vsc, are different with
> different registers being written to and different amount of registers
> being set. Can you please clarify since in audio we only need 3
> registers to write to, and in vsc we need 10.

Bitmagic with the header is the same. Then the rest of the data is
written one dword per register, if I'm not mistaken.

> >
> >> +
> >> +    data = 0;
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_2, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >
> > Generally this is not how these functions are expected to be written.
> > Please take a look at drivers/video/hdmi.c. It should be split into:
> > - generic function that packs the C structure into a flat byte buffer,
> > - driver-specific function that formats and writes the buffer to the
> > hardware.
> >
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_3, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >> +
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_4, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >> +
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_5, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >> +
> >> +    switch (dp_catalog->vsc_sdp_data.bpc) {
> >> +    case 10:
> >> +        bpc = DP_GENERIC0_6_YUV_10_BPC;
> >> +        break;
> >> +    case 8:
> >> +    default:
> >> +        bpc = DP_GENERIC0_6_YUV_8_BPC;
> >> +        break;
> >> +    }
> >> +
> >> +    /* VSC SDP payload as per table 2-117 of DP 1.4 specification */
> >> +    data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) |
> >> +           ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) << 4) |
> >> +           (bpc << 8) |
> >> +           ((dp_catalog->vsc_sdp_data.dynamic_range & 0x1) << 15) |
> >> +           ((dp_catalog->vsc_sdp_data.content_type & 0x7) << 16);
> >> +
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_6, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >> +
> >> +    data = 0;
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_7, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);
> >> +
> >> +    dp_write_link(catalog, MMSS_DP_GENERIC0_8, data);
> >> +    memcpy(buf + off, &data, sizeof(data));
> >> +    off += sizeof(data);


-- 
With best wishes
Dmitry