| Message ID | 20230102132306.9663-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix timeslots argument for DP DSC SST case | expand |
On Mon, Jan 02, 2023 at 03:23:06PM +0200, Stanislav Lisovskiy wrote: > We now accept timeslots param exactly how the variable > sounds: amount of timeslots, but not ratio timeslots/64. > So for SST case(when we have all timeslots for use), it > should be 64, but not 1. I noticed that at intel_dp_dsc_get_output_bpp() we have this comment: * for SST -> TimeSlotsPerMTP is 1, and there's a bunch of math used with this timeslots, but none of them is a direct division by 64. So I wonder if a refactor to reflect the "perMTP" is not needed there. Or the reverse math instead of passing the 64 directly. > This caused some issues in the tests. could you also expand on what tests? any "References:" link to cibuglog or so? Oh, any "Fixes:" tag as well? Thanks for the patch, Rodrigo. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 13baf3cb5f934..362fb394d613c 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1696,7 +1696,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, > str_yes_no(ret), str_yes_no(joiner_needs_dsc), > str_yes_no(intel_dp->force_dsc_en)); > ret = intel_dp_dsc_compute_config(intel_dp, pipe_config, > - conn_state, &limits, 1, true); > + conn_state, &limits, 64, true); > if (ret < 0) > return ret; > } > -- > 2.37.3 >
On Mon, Jan 02, 2023 at 03:23:06PM +0200, Stanislav Lisovskiy wrote: > We now accept timeslots param exactly how the variable > sounds: amount of timeslots, but not ratio timeslots/64. > So for SST case(when we have all timeslots for use), it > should be 64, but not 1. > This caused some issues in the tests. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Looks good to me, Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Manasi > --- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 13baf3cb5f934..362fb394d613c 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1696,7 +1696,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, > str_yes_no(ret), str_yes_no(joiner_needs_dsc), > str_yes_no(intel_dp->force_dsc_en)); > ret = intel_dp_dsc_compute_config(intel_dp, pipe_config, > - conn_state, &limits, 1, true); > + conn_state, &limits, 64, true); > if (ret < 0) > return ret; > } > -- > 2.37.3 >
On Tue, Jan 03, 2023 at 11:14:16AM -0500, Rodrigo Vivi wrote: > On Mon, Jan 02, 2023 at 03:23:06PM +0200, Stanislav Lisovskiy wrote: > > We now accept timeslots param exactly how the variable > > sounds: amount of timeslots, but not ratio timeslots/64. > > So for SST case(when we have all timeslots for use), it > > should be 64, but not 1. > > I noticed that at intel_dp_dsc_get_output_bpp() we have this comment: > > * for SST -> TimeSlotsPerMTP is 1, > > and there's a bunch of math used with this timeslots, but none of them > is a direct division by 64. Yep "TimeSlotsPerMTP is 1" already means that this is a ratio. We have 64 timeslots alltogether and obviously for SST we have all of them. To be honest that comment above isn't way too informative, I've had to dig into the spec and calculate everything by myself before I could figure out what is going on here. For the Link Total Bw can be calculated like this: Link Total Bw = link_symbol_clock * lane_count * 8 Amount of Bw we need for the current mode is: Required bpp = mode_clock * bpp For generic MST case, amount BW we have is split to 64 timeslots, i.e each 1 timeslot has Bw: 1 Timeslot Bw = Link Total Bw / 64 = (link_symbol_clock * lane_count * 8) / 64 Given amount if timeslots the bw which we will get is: Link Total Bw * (timeslots / 64) = (link_symbol_clock * lane_count * 8 * timeslots) / 64 However required Bw is as said above: Required Bw = mode_clock * bpp Thus in order to figure out the max bpp we can afford, we need to solve equation: Link Total Bw * (timeslots / 64) = mode_clock * bpp i.e bpp = (Link Total Bw * timeslots) / (mode_clock * 64) => bpp = (link_symbol_clock * lane_count * 8 * timeslots) / (mode_clock * 64) which can be simplified to: bpp = (link_symbol_clock * lane_count * timeslots) / (mode_clock * 8) In order for this to work timeslots should be actual amount of timeslots but not the ratio. > > So I wonder if a refactor to reflect the "perMTP" is not needed there. > Or the reverse math instead of passing the 64 directly. > > > This caused some issues in the tests. > > could you also expand on what tests? > any "References:" link to cibuglog or so? Asked from CI, but as I understand quite a lot of tests are affected, like _many_ :)) Will add the link. Stan > > Oh, any "Fixes:" tag as well? > > Thanks for the patch, > Rodrigo. > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 13baf3cb5f934..362fb394d613c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -1696,7 +1696,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, > > str_yes_no(ret), str_yes_no(joiner_needs_dsc), > > str_yes_no(intel_dp->force_dsc_en)); > > ret = intel_dp_dsc_compute_config(intel_dp, pipe_config, > > - conn_state, &limits, 1, true); > > + conn_state, &limits, 64, true); > > if (ret < 0) > > return ret; > > } > > -- > > 2.37.3 > >
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 13baf3cb5f934..362fb394d613c 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1696,7 +1696,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, str_yes_no(ret), str_yes_no(joiner_needs_dsc), str_yes_no(intel_dp->force_dsc_en)); ret = intel_dp_dsc_compute_config(intel_dp, pipe_config, - conn_state, &limits, 1, true); + conn_state, &limits, 64, true); if (ret < 0) return ret; }
We now accept timeslots param exactly how the variable sounds: amount of timeslots, but not ratio timeslots/64. So for SST case(when we have all timeslots for use), it should be 64, but not 1. This caused some issues in the tests. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)