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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > .../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.
