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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, 11 Feb 2024 at 19:18, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 2/10/2024 10:57 PM, Dmitry Baryshkov wrote:
> > On Sun, 11 Feb 2024 at 06:06, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2/10/2024 1:46 PM, Dmitry Baryshkov wrote:
> >>> On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>>>>> ---
> >>>>>>>     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.
> >
> > Yes, we have. However all structure manipulation functions, even for
> > DP, are usually a part of the hdmi.c. If I understand correctly, we
> > can still touch this file from drm-misc (or from other drm trees).
> >
>
> hmm ... I think the only reason we have
> hdmi_audio_infoframe_pack_for_dp() is because of re-using
> hdmi_audio_infoframe to pack into a struct dp_sdp.
>
> We will not be doing that here. It will be from a drm_dp_vsc_sdp to
> dp_sdp. Thats why I thought drm_dp_helper is more appropriate.

Could you please raise this question at #dri-devel? Let's get Jani's
and other maintainers' opinion.

>
> >> 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.
> >
> > I think you can do it in a much easier way:
> >
> > struct dp_sdp sdp
> > u32 header[2];
> >
> > hdmi_dp_vsc_sdp_pack(vsc_sdp, &sdp);
> > msm_dp_pack_header(&sdp, header);
> > dp_write_register(catalog, MMSS_DP_FOO_HEADER0, header[0]);
> > dp_write_register(catalog, MMSS_DP_FOO_HEADER1, header[1]);
> > // write sdp.db
> >
>
> Sure. Just a difference of packing it together Vs a local header. We
> will see which one looks better and post it. I felt that my way makes it
> clear that msm_dp needs extra parity specific to msm.

I just don't like extra wrapping structures. But really, let's see
your patches first.

-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux