Re: [Intel-gfx] [PATCH v2 09/22] drm/dp_mst: Fix fractional bpp scaling in drm_dp_calc_pbn_mode()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 04, 2023 at 01:22:27PM +0300, Imre Deak wrote:
> On Mon, Sep 04, 2023 at 05:53:11AM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 24, 2023 at 11:05:04AM +0300, Imre Deak wrote:
> > > For fractional bpp values passed to the function in a .4 fixed point
> > > format, the fractional part is currently ignored due to scaling bpp too
> > > early. Fix this by scaling the overhead factor instead and to avoid an
> > > overflow multiplying bpp with the overhead factor instead of the clock
> > > rate.
> > > 
> > > While at it simplify the formula, and pass the expected fixed point bpp
> > > values in the kunit tests.
> > > 
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/display/drm_dp_mst_topology.c  | 7 ++-----
> > >  drivers/gpu/drm/tests/drm_dp_mst_helper_test.c | 8 ++++----
> > >  2 files changed, 6 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > index ed96cfcfa3040..bd0f35a0ea5fb 100644
> > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > @@ -4712,12 +4712,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
> > >  	 * factor in the numerator rather than the denominator to avoid
> > >  	 * integer overflow
> > >  	 */
> > > +	u32 bpp_m = (dsc ? 64 / 16 : 64) * 1006 * bpp;
> > >  
> > > -	if (dsc)
> > > -		return DIV_ROUND_UP_ULL(mul_u32_u32(clock * (bpp / 16), 64 * 1006),
> > > -					8 * 54 * 1000 * 1000);
> > > -
> > > -	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> > > +	return DIV_ROUND_UP_ULL(mul_u32_u32(clock, bpp_m),
> > >  				8 * 54 * 1000 * 1000);
> > 
> > I thought I sorted out this mess already...
> > https://patchwork.freedesktop.org/patch/535005/?series=117201&rev=3
> > Apparently I forgot to push that.
> 
> Looks ok, can use that instead. I thought clock * bpp could overflow,
> but probably not in practice.

2^32/(16*3*2^4)~=5.6e6 -> 5.6 GHz dotclock. So should be good for
a few more years. But we can of course move bpp to the other side
of the mul_u32_u32() as you do here and then we don't have anything
to worry about as everything else there is constant.

> 
> The test cases below would still need to be fixed.

I thought I fixed the tests as well? Maybe they changed...

> 
> > 
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
> > > diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > > index 545beea33e8c7..ea2182815ebe8 100644
> > > --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > > +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> > > @@ -40,15 +40,15 @@ static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
> > >  	},
> > >  	{
> > >  		.clock = 332880,
> > > -		.bpp = 24,
> > > +		.bpp = 24 << 4,
> > >  		.dsc = true,
> > > -		.expected = 50
> > > +		.expected = 1191
> > >  	},
> > >  	{
> > >  		.clock = 324540,
> > > -		.bpp = 24,
> > > +		.bpp = 24 << 4,
> > >  		.dsc = true,
> > > -		.expected = 49
> > > +		.expected = 1161
> > >  	},
> > >  };
> > >  
> > > -- 
> > > 2.37.2
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel