Re: [PATCH v3 05/11] drm/i915/gen9+: Do not initialise active_crtcs for !modeset

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 09, 2016 at 01:45:20PM +0100, Maarten Lankhorst wrote:
> Op 08-11-16 om 15:11 schreef Ville Syrjälä:
> > On Tue, Nov 08, 2016 at 01:55:36PM +0100, Maarten Lankhorst wrote:
> >> This is a hack and not needed. Use the right mask by checking
> >> intel_state->modeset. This works for watermark sanitization too.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
> >>  1 file changed, 15 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 02f52b52a03d..d38a46efcfed 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> >>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >>  	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 pipe_size, ddb_size, active_crtcs;
> >>  	int nth_active_pipe;
> >>  
> >> +	if (intel_state->modeset)
> >> +		active_crtcs = intel_state->active_crtcs;
> >> +	else
> >> +		active_crtcs = dev_priv->active_crtcs;
> > What's the story with the locking here?
> if !modeset, 3 things can happen:
> 1. fastset, connection_mutex held, dev_priv->active_crtcs cannot change.
> 2. crtc disabled, active_crtcs is potentially garbage, but harmless since we don't write disabled wm's when the crtc is already disabled.
> (same as what happens currently)

But we still compute them? Can the computation fail on account of
that? I guess it shouldn't because when things are disabled all wms
should come out as 0?

> 3. crtc enabled, dev_priv->active_crtcs is valid because ddb reallocation requires locking all active crtc's for reallocation,
> which requires taking this lock for this crtc.

Case 3 at least looks a little suspect to me. What the code does is:

for_each_intel_crtc_in_mask(realloc_pipes) {
	intel_atomic_get_crtc_state();
	skl_allocate_pipe_ddb() {
		skl_ddb_get_pipe_allocation_limits();
	}
	...
}

Since it starts to compute this stuff already before it's necessarily
locked all the crtcs, I'm having a hard time convincing myself that
the state will always be up to date. I guess it might since
realloc_pipes should only have the one pipe unless active_crtcs is going
to change, but I don't think it's obvious at all when looking at the
code. Might want to change the way this is done for clarity if nothing
else.

I think we need to start documenting the locking rules for this kind
of device wide state better, and maybe try to sprinkle more locking
asserts around.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx