[PATCH 11/17] drm/msm/dp: add VSC SDP support for YUV420 over DP
Abhinav Kumar
quic_abhinavk at quicinc.com
Thu Feb 1 01:56:29 UTC 2024
On 1/27/2024 9:39 PM, Dmitry Baryshkov wrote:
> 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.
>
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()
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.
>>>
>>>> +
>>>> + 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);
>
>
More information about the dri-devel
mailing list