Re: [Intel-gfx] [PATCH v2 02/22] drm/i915/dp: Track the pipe and link bpp limits separately

[Imre Deak <imre.deak@intel.com>]

On Thu, Sep 14, 2023 at 01:51:16PM +0300, Luca Coelho wrote:
> On Thu, 2023-09-14 at 12:55 +0300, Imre Deak wrote:
> > On Thu, Sep 14, 2023 at 12:33:59PM +0300, Luca Coelho wrote:
> > > On Thu, 2023-08-24 at 11:04 +0300, Imre Deak wrote:
> > > > A follow-up patch will need to limit the output link bpp both in the
> > > > non-DSC and DSC configuration, so track the pipe and link bpp limits
> > > > separately in the link_config_limits struct.
> > > > 
> > > > Use .4 fixed point format for link bpp matching the 1/16 bpp granularity
> > > > in DSC mode and for now keep this limit matching the pipe bpp limit.
> > > > 
> > > > v2: (Jani)
> > > > - Add to_bpp_int(), to_bpp_x16() helpers instead of opencoding them.
> > > > - Rename link_config_limits::link.min/max_bpp to min/max_bpp_x16.
> > > > 
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_types.h    | 10 ++++++++
> > > >  drivers/gpu/drm/i915/display/intel_dp.c       | 25 +++++++++++--------
> > > >  drivers/gpu/drm/i915/display/intel_dp.h       |  9 ++++++-
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 17 ++++++++-----
> > > >  4 files changed, 44 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 731f2ec04d5cd..5875eff5012ce 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > > > index 788a577ebe16e..ebc7f4e60c777 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > > @@ -26,7 +26,14 @@ struct intel_encoder;
> > > >  struct link_config_limits {
> > > >  	int min_rate, max_rate;
> > > >  	int min_lane_count, max_lane_count;
> > > > -	int min_bpp, max_bpp;
> > > > +	struct {
> > > > +		/* Uncompressed DSC input or link output bpp in 1 bpp units */
> > > > +		int min_bpp, max_bpp;
> > > > +	} pipe;
> > > > +	struct {
> > > > +		/* Compressed or uncompressed link output bpp in 1/16 bpp units */
> > > > +		int min_bpp_x16, max_bpp_x16;
> > > > +	} link;
> > > >  };
> > > 
> > > It's not clear to me from the commit message (nor from the code, for
> > > that matter) why you need to store the values in both formats.  Can you
> > > clarify?
> > 
> > For DSC configuration two separate limits need to be considered:
> > 
> > One is the bpp value which is a property of the pixel format input to
> > the DSC engine, for this the DSC state computation should use the
> > pipe.min/max_bpp limits and this functionality of the DSC HW block can
> > be configured in 1 bits per pixel granularity.
> > 
> > The other one is the bpp value which is the format of pixels output from
> > the DSC engine (and is the actual pixel format on the link), for which
> > the DSC state computation should use link.min/max_bpp_x16. The DSC HW
> > block can be configure this pixel format in 1/16 bits per granularity.
> > 
> > For instance pipe.min/max_bpp will be 16 .. 30 bpp range (in 1 bpp
> > units), link.min/max_bpp_x16 in the 8 .. 27 bpp range (in 1/16 bpp
> > units).
> 
> Okay, but you're storing these two limits in the link structure.  So
> the important difference between them is not x16 vs non-x16.  If it
> were, you wouldn't have to store both, because you can easily convert
> them with your new to_*() functions.
> 
> So, isn't there a better name for these? Maybe input_max/min_bpp and
> output_max/min_bpp? You could keep the _x16 in the relevant one, but I
> think the main difference between the two should be reflected in the
> symbol names.

They are part of a pipe/link sub-structure, so the names are in effect
pipe.min/max_bpp and link.min/max_bpp_x16. pipe and link in turn are the
terms used for these same types of bpps elsewhere in DSC and non-DSC
code, hence I used them here as well for clarity. Maybe the comments
in the struct could be improved how the limits are used?

> --
> Cheers,
> Luca.