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
