| Message ID | 20230824080517.693621-10-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Improve BW management on shared display links | expand |
Amazing! This work looks awesome Imre, sorry it took me a little bit to get back to this :). For all of the DP MST helper patches: Reviewed-by: Lyude Paul <lyude@redhat.com> On Thu, 2023-08-24 at 11:05 +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); > } > 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 > }, > }; >
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. > } > 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
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. The test cases below would still need to be fixed. > > > } > > 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
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
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
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); } 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 }, };
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(-)