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

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

On Wed, Sep 06, 2023 at 01:45:51PM +0300, Ville Syrjälä wrote:
> 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.

Right.

> 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.

Either way is ok, could also just add a multiply_overflows check if it
makes sense.

> > The test cases below would still need to be fixed.
> 
> I thought I fixed the tests as well? Maybe they changed...

The .dsc = true cases could've been added only later, but there
.expected changes after you pass the correctly shifted bpp.

> > > 
> > > >  }
> > > >  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