On Thu, 1 Feb 2024 at 03:56, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 1/27/2024 9:39 PM, Dmitry Baryshkov wrote:
> > On Sun, 28 Jan 2024 at 07:34, Paloma Arellano <quic_parellan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>> 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.
> >
>
> We can generalize the MMSS_DP_GENERIC0 register writing by breaking it
> up to two things:
>
> 1) Add a function vsc_sdp_pack() similar to hdmi_avi_infoframe_pack_only()
Note, there is already a hdmi_audio_infoframe_pack_for_dp() function.
I think this patchset can add hdmi_colorimetry_infoframe_pack_for_dp()
[you can choose any other similar name that suits from your POV].
Also please extract the function that inits the dp_sdp_header. It can
be reused as is for both existing hdmi_audio_infoframe_pack_for_dp(),
your new function and the dp_audio code.
>
> 2) dp_catalog_write_generic_pkt() which will just write the packed
> buffer byte-by-byte to these MMSS_DP_GENERIC0_xxx register
>
> But audio seems a bit different. We use DP_AUDIO_STREAM_0/1.
> More importantly, it uses this sdp_map and writes each header one by one
> with dp_catalog_audio_set_header().
>
> Not sure if that entirely fits with this pack and then write model.
>
> It can be simplified. But I dont think this effort is needed for this
> series.
>
> So I would prefer to generalize audio SDP programming separately.
I'd definitely ask to add a utility function that merges 4 header
bytes with the parity data. We already have 5 instances of that code
in dp_audio.c, which is already too much by the number of 4. Adding
the 6th copy is NAKed.
BTW, I see both in this path and in dp_audio that the driver reads a
register, ORs it with the value for the next header byte and writes it
back to the hardware. Shouldn't the driver clear the corresponding
data bits first? I see the clears in the techpack, but not in the
upstream code. If my assumption is correct, we should end up with the
utility function that packs dp_sdp_header into u32[2], which can then
be used by both YUV and dp_audio code to just write two corresponding
registers.
BTW2: where is the rest of the audio infoframe? I see that the old
fbdev driver was at least clearing the first 4 bytes of the frame.
>
> >>>
> >>>> +
> >>>> + 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
