[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 14:08 +0300, Imre Deak wrote:
> 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 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.
> 
> 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?

Oh, of course.  Sorry, I missed that.

Reviewed-by: Luca Coelho <luciano.coelho at intel.com>

--
Cheers,
Luca.