| Message ID | 20190204184523.28097-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915: Fix wm latency==0 disable on skl+ | expand |
On Mon, Feb 04, 2019 at 08:45:20PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > When adding the early latency==0 check back I neglected to > realize that we no longer have a way to return a failure > from the wm computation like we had in the past (since we > now calculate wms before ddb allocations). Also plane_en > being false doesn't actually indicate that the level is > invalid as it wil also happen when the plane is not > enabled. > > skl_allocate_pipe_ddb() starts scanning from the maximum > watermark level and it stops as soon as it finds a level > that is deemed viable. The assumption being that if level > n+1 is valid then level n is valid as well. Thus if we > now disable any watermark level by zeroing its latency > the code will think that level to be actually valid > and won't confirm whether the actually enabled lower > watermark level(s) actually fit into the allotted ddb > space. This results in hilarious watermark values that > exceed the ddb allocation of the plane. > > The way we must now indicate a failure is to assign an > unreasoanbly big value to min_ddb_alloc which will then > make skl_allocate_pipe_ddb() reject the entire level. > > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Similarly, isn't the if (res_lines > 31) return; farther down the function going to also cause a problem? If we fail the line requirement then result->min_ddb_alloc and such never get set for this plane, which screws up the block sum at block allocation time. Matt > --- > drivers/gpu/drm/i915/intel_pm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index ed9786241307..d6186c90bc10 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4694,8 +4694,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *cstate, > uint_fixed_16_16_t selected_result; > u32 res_blocks, res_lines, min_ddb_alloc = 0; > > - if (latency == 0) > + if (latency == 0) { > + /* reject it */ > + result->min_ddb_alloc = U16_MAX; > return; > + } > > /* Display WA #1141: kbl,cfl */ > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) || > -- > 2.19.2 >
On Mon, Feb 04, 2019 at 03:07:50PM -0800, Matt Roper wrote: > On Mon, Feb 04, 2019 at 08:45:20PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > When adding the early latency==0 check back I neglected to > > realize that we no longer have a way to return a failure > > from the wm computation like we had in the past (since we > > now calculate wms before ddb allocations). Also plane_en > > being false doesn't actually indicate that the level is > > invalid as it wil also happen when the plane is not > > enabled. > > > > skl_allocate_pipe_ddb() starts scanning from the maximum > > watermark level and it stops as soon as it finds a level > > that is deemed viable. The assumption being that if level > > n+1 is valid then level n is valid as well. Thus if we > > now disable any watermark level by zeroing its latency > > the code will think that level to be actually valid > > and won't confirm whether the actually enabled lower > > watermark level(s) actually fit into the allotted ddb > > space. This results in hilarious watermark values that > > exceed the ddb allocation of the plane. > > > > The way we must now indicate a failure is to assign an > > unreasoanbly big value to min_ddb_alloc which will then > > make skl_allocate_pipe_ddb() reject the entire level. > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Similarly, isn't the > > if (res_lines > 31) > return; > > farther down the function going to also cause a problem? If we fail the > line requirement then result->min_ddb_alloc and such never get set for > this plane, which screws up the block sum at block allocation time. Hmm. Yes, that does seem to be the case. I'll respin. > > > Matt > > > --- > > drivers/gpu/drm/i915/intel_pm.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index ed9786241307..d6186c90bc10 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4694,8 +4694,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *cstate, > > uint_fixed_16_16_t selected_result; > > u32 res_blocks, res_lines, min_ddb_alloc = 0; > > > > - if (latency == 0) > > + if (latency == 0) { > > + /* reject it */ > > + result->min_ddb_alloc = U16_MAX; > > return; > > + } > > > > /* Display WA #1141: kbl,cfl */ > > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) || > > -- > > 2.19.2 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index ed9786241307..d6186c90bc10 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4694,8 +4694,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *cstate, uint_fixed_16_16_t selected_result; u32 res_blocks, res_lines, min_ddb_alloc = 0; - if (latency == 0) + if (latency == 0) { + /* reject it */ + result->min_ddb_alloc = U16_MAX; return; + } /* Display WA #1141: kbl,cfl */ if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||