[PATCH v2 13/19] drm/msm/dp: add VSC SDP support for YUV420 over DP
Abhinav Kumar
quic_abhinavk at quicinc.com
Sun Feb 11 04:06:27 UTC 2024
On 2/10/2024 1:46 PM, Dmitry Baryshkov wrote:
> On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
>>>> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano
>>>> <quic_parellan at quicinc.com> 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.
>>>>>
>>>>> Changes in v2:
>>>>> - Rename GENERIC0_SDPSIZE macro to GENERIC0_SDPSIZE_VALID
>>>>> - Remove dp_sdp from the dp_catalog struct since this data is
>>>>> being allocated at the point used
>>>>> - Create a new function in dp_utils to pack the VSC SDP data
>>>>> into a buffer
>>>>> - Create a new function that packs the SDP header bytes into a
>>>>> buffer. This function is made generic so that it can be
>>>>> utilized by dp_audio
>>>>> header bytes into a buffer
>>>>> - Create a new function in dp_utils that takes the packed
>>>>> buffer
>>>>> and writes to the DP_GENERIC0_* registers
>>>>> - Split the dp_catalog_panel_config_vsc_sdp() function into two
>>>>> to disable/enable sending VSC SDP packets
>>>>> - Check the DP HW version using the original useage of
>>>>> dp_catalog_hw_revision() and correct the version checking
>>>>> logic
>>>>> - Rename dp_panel_setup_vsc_sdp() to
>>>>> dp_panel_setup_vsc_sdp_yuv_420() to explicitly state that
>>>>> currently VSC SDP is only being set up to support YUV420
>>>>> modes
>>>>>
>>>>> Signed-off-by: Paloma Arellano <quic_parellan at quicinc.com>
>>>>> ---
>>>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 105 ++++++++++++++++++++++++++++
>>>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 6 ++
>>>>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 ++
>>>>> drivers/gpu/drm/msm/dp/dp_panel.c | 59 ++++++++++++++++
>>>>> drivers/gpu/drm/msm/dp/dp_reg.h | 3 +
>>>>> drivers/gpu/drm/msm/dp/dp_utils.c | 80 +++++++++++++++++++++
>>>>> drivers/gpu/drm/msm/dp/dp_utils.h | 3 +
>>>>> 7 files changed, 260 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> index 5d84c089e520a..0f28a4897b7b7 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> @@ -901,6 +901,111 @@ int dp_catalog_panel_timing_cfg(struct
>>>>> dp_catalog *dp_catalog)
>>>>> return 0;
>>>>> }
>>>>>
<Snip>
>>>>> +static int dp_panel_setup_vsc_sdp_yuv_420(struct dp_panel *dp_panel)
>>>>> +{
>>>>> + struct dp_catalog *catalog;
>>>>> + struct dp_panel_private *panel;
>>>>> + struct dp_display_mode *dp_mode;
>>>>> + struct dp_sdp_header sdp_header;
>>>>> + struct drm_dp_vsc_sdp vsc_sdp_data;
>>>>> + size_t buff_size;
>>>>> + u32 gen_buffer[10];
>>>>> + int rc = 0;
>>>>> +
>>>>> + if (!dp_panel) {
>>>>> + DRM_ERROR("invalid input\n");
>>>>> + rc = -EINVAL;
>>>>> + return rc;
>>>>> + }
>>>>> +
>>>>> + panel = container_of(dp_panel, struct dp_panel_private,
>>>>> dp_panel);
>>>>> + catalog = panel->catalog;
>>>>> + dp_mode = &dp_panel->dp_mode;
>>>>> + buff_size = sizeof(gen_buffer);
>>>>> +
>>>>> + memset(&sdp_header, 0, sizeof(sdp_header));
>>>>> + memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
>>>>> +
>>>>> + /* VSC SDP header as per table 2-118 of DP 1.4 specification */
>>>>> + sdp_header.HB0 = 0x00;
>>>>> + sdp_header.HB1 = 0x07;
>>>>> + sdp_header.HB2 = 0x05;
>>>>> + sdp_header.HB3 = 0x13;
>>>>
>>>> This should go to the packing function.
>>>>
>>>
>>> We can .... but ....
>>>
>>> the header bytes can change based on the format. These values are
>>> specific to YUV420. Thats why this part was kept outside of the generic
>>> vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it
>>> but this was the reason.
>
> These values can be set from the sdp_type, revision and length fields
> of struct drm_dp_vsc_sdp.
> The function intel_dp_vsc_sdp_pack() is pretty much close to what I had in mind.
>
>>>
>>>>> +
>>>>> + /* VSC SDP Payload for DB16 */
>>>>
>>>> Comments are useless more or less. The code fills generic information
>>>> structure which might or might not be packed to the data buffer.
>>>>
>>>>> + vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
>>>>> + vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
>>>>> +
>>>>> + /* VSC SDP Payload for DB17 */
>>>>> + vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
>>>>> +
>>>>> + /* VSC SDP Payload for DB18 */
>>>>> + vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
>>>>> +
>>>>> + vsc_sdp_data.bpc = dp_mode->bpp / 3;
>>>>
>>>> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
>>>>
>>>
>>> intel_dp_compute_vsc_colorimetry() uses colorspace property to pick
>>> YUV420, we do not.
>
> Intel function also uses output_format, but it's true, it is full of
> Intel specifics.
>
>>> Right now, its a pure driver decision to use YUV420
>>> when the mode is supported only in that format.
>>>
>>> Also, many params are to be used within this function cached inside
>>> intel_crtc_state . We will first need to make that API more generic to
>>> be re-usable by others.
>>>
>>> I think overall, if we want to have a generic packing across vendors, it
>>> needs more work. I think one of us can take that up as a separate effort.
>
> Ack, I agree here. I did only a quick glance over
> intel_dp_compute_vsc_colorimetry function() beforehand.
>
>>>
>>>>> +
>>>>> + rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header,
>>>>> gen_buffer, buff_size);
>>>>> + if (rc) {
>>>>> + DRM_ERROR("unable to pack vsc sdp\n");
>>>>> + return rc;
>>>>> + }
>>>>> +
>>>>> + dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
>>>>> +
>>>>> + return rc;
>>>>> +}
>>>>> +
>>>>> void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>> {
>>>>> struct dp_catalog *catalog;
>>>>> @@ -344,6 +399,10 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>>>> catalog->dp_active = data;
>>>>>
>>>>> dp_catalog_panel_timing_cfg(catalog);
>>>>> +
>>>>> + if (dp_panel->dp_mode.out_fmt_is_yuv_420)
>>>>> + dp_panel_setup_vsc_sdp_yuv_420(dp_panel);
>>>>> +
>>>>> panel->panel_on = true;
>>>>>
>>>>> return 0;
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h
>>>>> b/drivers/gpu/drm/msm/dp/dp_reg.h
>>>>> index ea85a691e72b5..2983756c125cd 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_reg.h
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_reg.h
>>>>> @@ -142,6 +142,7 @@
>>>>> #define DP_MISC0_SYNCHRONOUS_CLK (0x00000001)
>>>>> #define DP_MISC0_COLORIMETRY_CFG_SHIFT (0x00000001)
>>>>> #define DP_MISC0_TEST_BITS_DEPTH_SHIFT (0x00000005)
>>>>> +#define DP_MISC1_VSC_SDP (0x00004000)
>>>>>
>>>>> #define REG_DP_VALID_BOUNDARY (0x00000030)
>>>>> #define REG_DP_VALID_BOUNDARY_2 (0x00000034)
>>>>> @@ -201,9 +202,11 @@
>>>>> #define MMSS_DP_AUDIO_CTRL_RESET (0x00000214)
>>>>>
>>>>> #define MMSS_DP_SDP_CFG (0x00000228)
>>>>> +#define GEN0_SDP_EN (0x00020000)
>>>>> #define MMSS_DP_SDP_CFG2 (0x0000022C)
>>>>> #define MMSS_DP_AUDIO_TIMESTAMP_0 (0x00000230)
>>>>> #define MMSS_DP_AUDIO_TIMESTAMP_1 (0x00000234)
>>>>> +#define GENERIC0_SDPSIZE_VALID (0x00010000)
>>>>>
>>>>> #define MMSS_DP_AUDIO_STREAM_0 (0x00000240)
>>>>> #define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>> index 176d839906cec..05e0133eb50f3 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>> @@ -4,6 +4,16 @@
>>>>> */
>>>>>
>>>>> #include <linux/types.h>
>>>>> +#include <drm/display/drm_dp_helper.h>
>>>>> +#include <drm/drm_print.h>
>>>>> +
>>>>> +#include "dp_utils.h"
>>>>> +
>>>>> +#define DP_GENERIC0_6_YUV_8_BPC BIT(0)
>>>>> +#define DP_GENERIC0_6_YUV_10_BPC BIT(1)
>>>>> +
>>>>> +#define DP_SDP_HEADER_SIZE 8
>>>>> +#define DP_VSC_SDP_BUFFER_SIZE 40
>>>>>
>>>>> u8 dp_utils_get_g0_value(u8 data)
>>>>> {
>>>>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
>>>>>
>>>>> return parity_byte;
>>>>> }
>>>>> +
>>>>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
>>>>> *buffer, size_t buff_size)
>>>>> +{
>>>>> + u32 header, parity;
>>>>> +
>>>>> + if (buff_size < DP_SDP_HEADER_SIZE)
>>>>> + return -ENOSPC;
>>>>> +
>>>>> + memset(buffer, 0, sizeof(buffer));
>>>>> +
>>>>> + /* HEADER BYTE 0 */
>>>>> + header = sdp_header->HB0;
>>>>> + parity = dp_utils_calculate_parity(header);
>>>>> + buffer[0] = ((header << HEADER_BYTE_0_BIT) | (parity <<
>>>>> PARITY_BYTE_0_BIT));
>>>>> +
>>>>> + /* HEADER BYTE 1 */
>>>>> + header = sdp_header->HB1;
>>>>> + parity = dp_utils_calculate_parity(header);
>>>>> + buffer[0] |= ((header << HEADER_BYTE_1_BIT) | (parity <<
>>>>> PARITY_BYTE_1_BIT));
>>>>> +
>>>>> + /* HEADER BYTE 2 */
>>>>> + header = sdp_header->HB2;
>>>>> + parity = dp_utils_calculate_parity(header);
>>>>> + buffer[1] = ((header << HEADER_BYTE_2_BIT) | (parity <<
>>>>> PARITY_BYTE_2_BIT));
>>>>> +
>>>>> + /* HEADER BYTE 3 */
>>>>> + header = sdp_header->HB3;
>>>>> + parity = dp_utils_calculate_parity(header);
>>>>> + buffer[1] |= ((header << HEADER_BYTE_3_BIT) | (parity <<
>>>>> PARITY_BYTE_3_BIT));
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
>>>>> struct dp_sdp_header *sdp_header,
>>>>> + u32 *buffer, size_t buff_size)
>>>>> +{
>>>>
>>>> No. This function should pack data into struct dp_sdp and it should go
>>>> to drivers/video/hdmi.c
>>>>
>>>
>>> What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
>>>
>>
>> I think you were referring to the fact that instead of a custom buffer,
>> we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.
>>
>> If yes, I agree with this part.
>
> Yes.
>
>>
>>> It seems like struct drm_dp_vsc_sdp is more appropriate for this.
>>>
>>> Regarding hdmi.c, I think the packing needs to be more generic to move
>>> it there.
>>>
>>> I am not seeing parity bytes packing with other vendors. So there will
>>> have to be some packing there and then some packing here.
>
> Yes. Writing the HB + PB seems specific to Qualcomm hardware. At least
> Intel and AMD seem to write header bytes without parity.
>
>>> Also, like you already noticed, to make the VSC SDP packing more generic
>>> to move to hdmi.c, it needs more work to make it more usable like
>>> supporting all pixel formats and all colorimetry values.
>>>
>>> We do not have that much utility yet for that.
>
> I think you are mixing the filling of drm_dp_vsc_sdp and packing of
> that struct. I suppose intel_dp_vsc_sdp_pack() can be extracted more
> or less as is.
> Once somebody needs to support 3D (AMD does), they can extend the function.
>
Yes once I corrected my understanding of using the function to just pack
struct dp_sdp instead of the buffer, I understood the intention.
We can try it. I have written up a RFC to move the
intel_dp_vsc_sdp_pack() to drm_dp_helper as that seems more appropriate
now for it and not hdmi.c as we already have some vsc sdp utils in
drm_dp_helper.
But before I post, we will do some cleanup and see if things look
reasonable in this patch too because we will still need to write a
common utility to pack the parity bytes for the sdp header which we need
to utilize for audio and vsc sdp in our DP case.
I am thinking of a
struct msm_dp_sdp_pb {
u8 PB0;
u8 PB1;
u8 PB2;
u8 PB3;
} __packed;
struct msm_dp_sdp_with_parity {
struct dp_sdp vsc_sdp;
struct msm_dp_sdp_pb pb;
};
intel_dp_vsc_sdp_pack() will pack the struct dp_sdp but we will have to
do some additional parity byte packing after that. That part will remain
common for audio and vsc for msm.
More information about the dri-devel
mailing list