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

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

 



On Mon, Nov 19, 2018 at 10:33:37PM +0200, Ville Syrjälä wrote:
> 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@xxxxxxxxx>
> > > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  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.
>

Ok will change it to use array notation
 
> > 
> > > {
> > > 	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.

Yes so bail as soon as the value is less than or equal to the max determined from the user requested bpc, right?

Manasi

> 
> > 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
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[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