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

[Luca Coelho]

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 at linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak at 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.

--
Cheers,
Luca.