Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

["Lankhorst, Maarten" <maarten.lankhorst@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4539 bytes --]

Mahesh Kumar schreef op di 30-05-2017 om 18:26 [+0530]:
> Hi Maarten,
> 
> Thanks for review :)
> 
> 
> On Tuesday 30 May 2017 03:30 PM, Maarten Lankhorst wrote:
> > Op 26-05-17 om 17:15 schreef Mahesh Kumar:
> > > Don't trust cached DDB values. Recalculate the ddb value if
> > > cached value
> > > is zero.
> > > 
> > > If i915 is build as a module, there may be a race condition when
> > > cursor_disable call comes even before intel_fbdev_initial_config.
> > > Which may lead to cached value being 0. And further commit will
> > > fail
> > > until a modeset.
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
> > >   1 file changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 936eef1634c7..b67be1355e39 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   	struct drm_i915_private *dev_priv = to_i915(dev);
> > >   	struct drm_crtc *for_crtc = cstate->base.crtc;
> > >   	unsigned int pipe_size, ddb_size;
> > > +	unsigned int active_crtcs;
> > >   	int nth_active_pipe;
> > >   
> > >   	if (WARN_ON(!state) || !cstate->base.active) {
> > > @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   	}
> > >   
> > >   	if (intel_state->active_pipe_changes)
> > > -		*num_active = hweight32(intel_state-
> > > >active_crtcs);
> > > +		active_crtcs = intel_state->active_crtcs;
> > >   	else
> > > -		*num_active = hweight32(dev_priv->active_crtcs);
> > > +		active_crtcs = dev_priv->active_crtcs;
> > >   
> > > +	*num_active = hweight32(active_crtcs);
> > >   	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> > >   	WARN_ON(ddb_size == 0);
> > >   
> > > @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   		 * copy from old state to be sure
> > >   		 */
> > >   		*alloc = to_intel_crtc_state(for_crtc->state)-
> > > >wm.skl.ddb;
> > > -		return;
> > > +		if (!skl_ddb_entry_size(alloc))
> > > +			DRM_DEBUG_KMS("Cached pipe DDB is 0
> > > recalculate\n");
> > > +		else
> > > +			return;
> > >   	}
> > >   
> > > -	nth_active_pipe = hweight32(intel_state->active_crtcs &
> > > +	nth_active_pipe = hweight32(active_crtcs &
> > >   				    (drm_crtc_mask(for_crtc) -
> > > 1));
> > > -	pipe_size = ddb_size / hweight32(intel_state-
> > > >active_crtcs);
> > > +	pipe_size = ddb_size / hweight32(active_crtcs);
> > >   	alloc->start = nth_active_pipe * ddb_size /
> > > *num_active;
> > >   	alloc->end = alloc->start + pipe_size;
> > >   }
> > 
> > I'd love it if you also add a warning somewhere for active pipe
> > with no ddb allocation to prevent this from reoccuring in the
> > future. :)
> > But with the root cause being a invalid ddb allocation at boot,
> > won't the below one liner should fix it too, but better?
> 
> Above DEBUG message is actually warning. That will hit only if pipe
> is 
> active & DDB allocated for that pipe is "0" (we do return from caller
> if 
> !cstate->active).
> I can reword the msg to be WARN & make it something like "Pipe is
> active 
> with No DDB allocated, recompute ddb now". This will fix the issue
> as 
> well as add warning msg.
> 
> > 
> > I'l ltest it when f3-kbl becomes available again.
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index a488e068c3d6..63a47909f618 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5051,7 +5051,7 @@ skl_compute_wm(struct drm_atomic_state
> > *state)
> >   	 */
> >   	for_each_new_crtc_in_state(state, crtc, cstate, i)
> >   		changed = true;
> > -	if (!changed)
> > +	if (!changed && !to_i915(state->dev)->wm.distrust_bios_wm)
> >   		return 0;
> >   
> 
> This change should also solve the issue during boot.

Yeah I did some testing and this fixes it.
Looks like a bug in the original implementation of skl_compute_wm that
the KBL just happened to hit.

So I guess if others are hitting it too, then this needs
Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
check time (v4)")
Cc: <stable@vger.kernel.org> # v4.8+

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx