[Intel-gfx] [PATCH v9 01/24] drm/dsc: Modify DRM helper to return complete DSC color depth capabilities

[Ville Syrjälä]

On Mon, Nov 19, 2018 at 12:10:47PM -0800, Manasi Navare wrote:
> On Mon, Nov 19, 2018 at 09:43:38PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 13, 2018 at 05:52:09PM -0800, Manasi Navare wrote:
> > > DSC DPCD color depth register advertises its color depth capabilities
> > > by setting each of the bits that corresponding to a specific color
> > > depth. This patch defines those specific color depths and adds
> > > a helper to return an array of color depth capabilities.
> > > 
> > > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_helper.c | 29 +++++++++++++++++++----------
> > >  include/drm/drm_dp_helper.h     |  9 +++++----
> > >  2 files changed, 24 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 6d483487f2b4..286567063960 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -1428,17 +1428,26 @@ u8 drm_dp_dsc_sink_line_buf_depth(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> > >  }
> > >  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])
> > > +void drm_dp_dsc_sink_color_depth_cap(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE],
> > > +				     u8 *dsc_sink_color_depth_cap)
> > >  {
> > > +	int i, cnt = 0;
> > >  	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;
> > > +	for (i = 1; i <= 3; i++) {
> > > +		if (!(color_depth & BIT(i)))
> > > +			continue;
> > > +		switch (i) {
> > > +		case 1:
> > > +			dsc_sink_color_depth_cap[cnt++] = DP_DSC_8_BPC;
> > > +			break;
> > > +		case 2:
> > > +			dsc_sink_color_depth_cap[cnt++] = DP_DSC_10_BPC;
> > > +			break;
> > > +		case 3:
> > > +			dsc_sink_color_depth_cap[cnt++] = DP_DSC_12_BPC;
> > > +			break;
> > > +		}
> > > +	}
> > >  }
> > > -EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
> > > +EXPORT_SYMBOL(drm_dp_dsc_sink_color_depth_cap);
> > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > index 3314e91f6eb3..ea3233b0a790 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -242,9 +242,9 @@
> > >  # define DP_DSC_YCbCr420_Native             (1 << 4)
> > >  
> > >  #define DP_DSC_DEC_COLOR_DEPTH_CAP          0x06A
> > > -# define DP_DSC_8_BPC                       (1 << 1)
> > > -# define DP_DSC_10_BPC                      (1 << 2)
> > > -# define DP_DSC_12_BPC                      (1 << 3)
> > > +# define DP_DSC_8_BPC                       8
> > > +# define DP_DSC_10_BPC                      10
> > > +# define DP_DSC_12_BPC                      12
> > 
> > 
> > I'd suggest something simpler like:
> > 
> > int foo(u8 bpc[3])
> 
> Is passing a full array recommended method vs passing the pointer to the array?

It's the same thing in C. The compiler will treat both as a pointer
(eg. sadly you can't use ARRAY_SIZE() on this because it will just use
the size of the pointer rather than the size of the array in the
calculation).

Despite the language shortcomings I like to use the array notation
as a means to document what is the expected size of the passed in
array.

> 
> > {
> > 	int num_bpc = 0;
> > 
> > 	if (color_depth & DP_DSC_12_BPC)
> > 		bpc[num_bpc++] = 12;
> > 	if (color_depth & DP_DSC_10_BPC)
> > 		bpc[num_bpc++] = 10;
> > 	if (color_depth & DP_DSC_8_BPC)
> > 		bpc[num_bpc++] = 8;
> > 
> > 	return num_bpc;
> > }
> 
> Yes I could modify like above except start from lowest bpc in bpc[0] going all the way to highest.

Highest to lowest seems more sensible to me since we want to pick the
max. So when we walk the array we can bail as soon as we find the
highest suitable value.

> Also as of now its only 3 bpcs so its safe to just have an array for 3 bpcs for now right?

Yes. Adding more would require changing the function anyway,
and so the callers can be updated at the same time.

> 
> Manasi
> 
> > 
> > >  
> > >  #define DP_DSC_PEAK_THROUGHPUT              0x06B
> > >  # define DP_DSC_THROUGHPUT_MODE_0_MASK      (0xf << 0)
> > > @@ -1123,7 +1123,8 @@ drm_dp_is_branch(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> > >  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]);
> > > +void drm_dp_dsc_sink_color_depth_cap(const u8 dsc_dpc[DP_DSC_RECEIVER_CAP_SIZE],
> > > +				     u8 *dsc_sink_color_depth_cap);
> > >  
> > >  static inline bool
> > >  drm_dp_sink_supports_dsc(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel