Re: [PATCH v6 04/28] drm/dp: DRM DP helper/macros to get DP sink DSC parameters

From: Sean Paul <sean@poorly.run>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v6 04/28] drm/dp: DRM DP helper/macros to get DP sink DSC parameters
Date: Wed, 30 Jan 2019 13:06:57 -0500	[thread overview]
Message-ID: <20190130180657.GW114153@art_vandelay> (raw)
In-Reply-To: <CAKMK7uEKP8S4LWbBL2aJ1rpc4y6ygVsPz6L77NCdOfAa9ht9Hw@mail.gmail.com>

On Wed, Jan 30, 2019 at 12:06:45PM +0100, Daniel Vetter wrote:
> On Wed, Dec 19, 2018 at 7:54 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Oct 24, 2018 at 03:28:16PM -0700, Manasi Navare wrote:
> > > This patch adds inline functions and helpers for obtaining
> > > DP sink's supported DSC parameters like DSC sink support,
> > > eDP compressed BPP supported, maximum slice count supported
> > > by the sink devices, DSC line buffer bit depth supported on DP sink,
> > > DSC sink maximum color depth by parsing corresponding DPCD registers.
> > >
> > > v4:
> > > * Add helper to give line buf bit depth (Manasi)
> > > * Correct the bit masking in color depth helper (manasi)
> > > v3:
> > > * Use SLICE_CAP_2 for DP (Anusha)
> > > v2:
> > > * Add DSC sink support macro (Jani N)
> > >
> > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Reviewed-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_helper.c | 90 +++++++++++++++++++++++++++++++++
> > >  include/drm/drm_dp_helper.h     | 30 +++++++++++
> > >  2 files changed, 120 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 37c01b6076ec..6d483487f2b4 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -1352,3 +1352,93 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct drm_dp_desc *desc,
> > >       return 0;
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_read_desc);
> > > +
> > > +/**
> > > + * DRM DP Helpers for DSC
> > > + */
> >
> > This isn't really kerneldoc. Can you pls fix it, including all the other
> > new exported functions?
> 
> Ping. Still not fixed, and it's well over one month now. Would be nice
> to get this sorted. I'd type the docs myself, but I don't really have
> an idea about DSC.
> 

Ha, this is good timing, I just stumbled over the following htmldocs warnings:
../drivers/gpu/drm/drm_dp_helper.c:1368: warning: Function parameter or member 'dsc_dpcd' not described in 'drm_dp_dsc_sink_max_slice_count'
../drivers/gpu/drm/drm_dp_helper.c:1368: warning: Function parameter or member 'is_edp' not described in 'drm_dp_dsc_sink_max_slice_count'

I tried to add them, but there's a lot of dsc functions that need docs and I
don't have the time/energy to document them properly.

So, I'll echo Daniel's request.

Sean

> Thanks, Daniel
> 
> >
> > Thanks, Daniel
> >
> > > +u8 drm_dp_dsc_sink_max_slice_count(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE],
> > > +                                bool is_edp)
> > > +{
> > > +     u8 slice_cap1 = dsc_dpcd[DP_DSC_SLICE_CAP_1 - DP_DSC_SUPPORT];
> > > +
> > > +     if (is_edp) {
> > > +             /* For eDP, register DSC_SLICE_CAPABILITIES_1 gives slice count */
> > > +             if (slice_cap1 & DP_DSC_4_PER_DP_DSC_SINK)
> > > +                     return 4;
> > > +             if (slice_cap1 & DP_DSC_2_PER_DP_DSC_SINK)
> > > +                     return 2;
> > > +             if (slice_cap1 & DP_DSC_1_PER_DP_DSC_SINK)
> > > +                     return 1;
> > > +     } else {
> > > +             /* For DP, use values from DSC_SLICE_CAP_1 and DSC_SLICE_CAP2 */
> > > +             u8 slice_cap2 = dsc_dpcd[DP_DSC_SLICE_CAP_2 - DP_DSC_SUPPORT];
> > > +
> > > +             if (slice_cap2 & DP_DSC_24_PER_DP_DSC_SINK)
> > > +                     return 24;
> > > +             if (slice_cap2 & DP_DSC_20_PER_DP_DSC_SINK)
> > > +                     return 20;
> > > +             if (slice_cap2 & DP_DSC_16_PER_DP_DSC_SINK)
> > > +                     return 16;
> > > +             if (slice_cap1 & DP_DSC_12_PER_DP_DSC_SINK)
> > > +                     return 12;
> > > +             if (slice_cap1 & DP_DSC_10_PER_DP_DSC_SINK)
> > > +                     return 10;
> > > +             if (slice_cap1 & DP_DSC_8_PER_DP_DSC_SINK)
> > > +                     return 8;
> > > +             if (slice_cap1 & DP_DSC_6_PER_DP_DSC_SINK)
> > > +                     return 6;
> > > +             if (slice_cap1 & DP_DSC_4_PER_DP_DSC_SINK)
> > > +                     return 4;
> > > +             if (slice_cap1 & DP_DSC_2_PER_DP_DSC_SINK)
> > > +                     return 2;
> > > +             if (slice_cap1 & DP_DSC_1_PER_DP_DSC_SINK)
> > > +                     return 1;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_dsc_sink_max_slice_count);
> > > +
> > > +u8 drm_dp_dsc_sink_line_buf_depth(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> > > +{
> > > +     u8 line_buf_depth = dsc_dpcd[DP_DSC_LINE_BUF_BIT_DEPTH - DP_DSC_SUPPORT];
> > > +
> > > +     switch (line_buf_depth & DP_DSC_LINE_BUF_BIT_DEPTH_MASK) {
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_9:
> > > +             return 9;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_10:
> > > +             return 10;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_11:
> > > +             return 11;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_12:
> > > +             return 12;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_13:
> > > +             return 13;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_14:
> > > +             return 14;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_15:
> > > +             return 15;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_16:
> > > +             return 16;
> > > +     case DP_DSC_LINE_BUF_BIT_DEPTH_8:
> > > +             return 8;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_dsc_sink_line_buf_depth);
> > > +
> > > +u8 drm_dp_dsc_sink_max_color_depth(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> > > +{
> > > +     u8 color_depth = dsc_dpcd[DP_DSC_DEC_COLOR_DEPTH_CAP - DP_DSC_SUPPORT];
> > > +
> > > +     if (color_depth & DP_DSC_12_BPC)
> > > +             return 12;
> > > +     if (color_depth & DP_DSC_10_BPC)
> > > +             return 10;
> > > +     if (color_depth & DP_DSC_8_BPC)
> > > +             return 8;
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
> > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > index dd33d59739f8..7f7f5b965466 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -1067,6 +1067,36 @@ drm_dp_is_branch(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> > >       return dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT;
> > >  }
> > >
> > > +/* DP/eDP DSC support */
> > > +u8 drm_dp_dsc_sink_max_slice_count(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE],
> > > +                                bool is_edp);
> > > +u8 drm_dp_dsc_sink_line_buf_depth(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE]);
> > > +u8 drm_dp_dsc_sink_max_color_depth(const u8 dsc_dpc[DP_DSC_RECEIVER_CAP_SIZE]);
> > > +
> > > +static inline bool
> > > +drm_dp_sink_supports_dsc(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> > > +{
> > > +     return dsc_dpcd[DP_DSC_SUPPORT - DP_DSC_SUPPORT] &
> > > +             DP_DSC_DECOMPRESSION_IS_SUPPORTED;
> > > +}
> > > +
> > > +static inline u16
> > > +drm_edp_dsc_sink_output_bpp(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> > > +{
> > > +     return dsc_dpcd[DP_DSC_MAX_BITS_PER_PIXEL_LOW - DP_DSC_SUPPORT] |
> > > +             (dsc_dpcd[DP_DSC_MAX_BITS_PER_PIXEL_HI - DP_DSC_SUPPORT] &
> > > +              DP_DSC_MAX_BITS_PER_PIXEL_HI_MASK <<
> > > +              DP_DSC_MAX_BITS_PER_PIXEL_HI_SHIFT);
> > > +}
> > > +
> > > +static inline u32
> > > +drm_dp_dsc_sink_max_slice_width(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> > > +{
> > > +     /* Max Slicewidth = Number of Pixels * 320 */
> > > +     return dsc_dpcd[DP_DSC_MAX_SLICE_WIDTH - DP_DSC_SUPPORT] *
> > > +             DP_DSC_SLICE_WIDTH_MULTIPLIER;
> > > +}
> > > +
> > >  /*
> > >   * DisplayPort AUX channel
> > >   */
> > > --
> > > 2.18.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx