Re: [PATCH v3 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 Thu, 15 Feb 2024 at 19:03, Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Thu, 15 Feb 2024 at 18:39, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >
> >
> >
> > On 2/15/2024 12:40 AM, Dmitry Baryshkov wrote:
> > > On Wed, 14 Feb 2024 at 22:15, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > >>
> > >>
> > >>
> > >> On 2/14/2024 11:39 AM, Dmitry Baryshkov wrote:
> > >>> On Wed, 14 Feb 2024 at 20:04, 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 v3:
> > >>>>           - Create a new struct, msm_dp_sdp_with_parity, which holds the
> > >>>>             packing information for VSC SDP
> > >>>>           - Use drm_dp_vsc_sdp_pack() to pack the data into the new
> > >>>>             msm_dp_sdp_with_parity struct instead of specifically packing
> > >>>>             for YUV420 format
> > >>>>           - Modify dp_catalog_panel_send_vsc_sdp() to send the VSC SDP
> > >>>>             data using the new msm_dp_sdp_with_parity struct
> > >>>>
> > >>>> 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 | 113 ++++++++++++++++++++++++++++
> > >>>>    drivers/gpu/drm/msm/dp/dp_catalog.h |   7 ++
> > >>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c    |   4 +
> > >>>>    drivers/gpu/drm/msm/dp/dp_panel.c   |  55 ++++++++++++++
> > >>>>    drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
> > >>>>    drivers/gpu/drm/msm/dp/dp_utils.c   |  48 ++++++++++++
> > >>>>    drivers/gpu/drm/msm/dp/dp_utils.h   |  18 +++++
> > >>>>    7 files changed, 248 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>>> index 5d84c089e520a..61d5317efe683 100644
> > >>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>>> @@ -901,6 +901,119 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
> > >>>>           return 0;
> > >>>>    }
> > >>>>
> > >>>> +static void dp_catalog_panel_send_vsc_sdp(struct dp_catalog *dp_catalog,
> > >>>> +                                         struct msm_dp_sdp_with_parity *msm_dp_sdp)
> > >>>> +{
> > >>>> +       struct dp_catalog_private *catalog;
> > >>>> +       u32 val;
> > >>>> +
> > >>>> +       if (!dp_catalog) {
> > >>>> +               DRM_ERROR("invalid input\n");
> > >>>> +               return;
> > >>>> +       }
> > >>>> +
> > >>>> +       catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog);
> > >>>> +
> > >>>> +       val = ((msm_dp_sdp->vsc_sdp.sdp_header.HB0) << HEADER_BYTE_0_BIT |
> > >>>> +              (msm_dp_sdp->pb.PB0 << PARITY_BYTE_0_BIT) |
> > >>>> +              (msm_dp_sdp->vsc_sdp.sdp_header.HB1) << HEADER_BYTE_1_BIT |
> > >>>> +              (msm_dp_sdp->pb.PB1 << PARITY_BYTE_1_BIT));
> > >>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_0, val);
> > >>>> +
> > >>>> +       val = ((msm_dp_sdp->vsc_sdp.sdp_header.HB2) << HEADER_BYTE_2_BIT |
> > >>>> +              (msm_dp_sdp->pb.PB2 << PARITY_BYTE_2_BIT) |
> > >>>> +              (msm_dp_sdp->vsc_sdp.sdp_header.HB3) << HEADER_BYTE_3_BIT |
> > >>>> +              (msm_dp_sdp->pb.PB3 << PARITY_BYTE_3_BIT));
> > >>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_1, val);
> > >>>
> > >>> I still think that this is not the way to do it. Could you please
> > >>> extract the function that takes struct dp_sdp_header, calculates
> > >>> padding and writes resulting data to the hardware? This way we can
> > >>> reuse it later for all the dp_audio stuff.
> > >>>
> > >>
> > >> hmmm ... dp_utils_pack_sdp_header() does that you are asking for right?
> > >>
> > >> OR are you asking for another function like:
> > >>
> > >> 1) rename dp_utils_pack_sdp_header() to dp_utils_calc_sdp_parity()
> > >> 2) dp_utils_pack_sdp() takes two u32 to pack the header and parity
> > >> together and we move the << HEADER_BYTE_xx | part to it
> > >>
> > >> dp_catalog_panel_send_vsc_sdp() just uses these two u32 to write the
> > >> headers.
> > >
> > > I'm really looking for the following function:
> > >
> > > void dp_catalog_panel_send_vsc_sdp(struct dp_catalog *dp_catalog,
> > > struct dp_sdp *dp_sdp)
> > > {
> > >      dp_write_vsc_header(dp_catalog, MMSS_DP_GENERIC0_0, &dp_sdp->sdp_header);
> > >      dp_write_vsc_packet(dp_catalog, MMSS_DP_GENERIC0_2, dp_sdp);
> > > }
> > >
> > > Then dp_audio functions will be able to fill struct dp_sdp_header and
> > > call dp_write_vsc_header (or whatever other name for that function)
> > > directly.
> > >
> >
> > I think there is some misunderstanding here.
> >
> > Audio does not write or use generic_0 registers. It uses audio infoframe
> > SDP registers. So the catalog function of audio will not change.
>
> Sure, that's why I added the register to the `dp_write_vsc_header` prototype.
>
> E.g.:
>
> void dp_audio_stream_sdp(...)
> {
>     struct dp_sdp_header hdr;
>     hdr.HB0 = 0;
>     hdr.HB1 = 0x2;
>     hdr.HB2 = ...;
>     hdr.HB3 = audio->nchannels - 1;
>     dp_write_vsc_header(dp_catalog, MMSS_DP_AUDIO_STREAM_0, &hdr);
> }
>
>
> >
> > The only part common between audio and vsc sdp is the parity byte
> > calculation and the packing of parity and header bytes into 2 u32s.
> >
> > Thats why I wrote that we will have a common util between audio and vsc
> > sdp only to pack the data but the catalog functions will be different.
> >

After an offline discussion, let's start with the dp_util function
packing struct dp_sdp_header into u32[2], then we can decide on the
fate of the dp_audio stuff later on.


-- 
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