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

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

Op 10-11-16 om 11:53 schreef Ville Syrjälä:
> 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?
Indeed, they come out as zero so it can't fail, regardless of the value of active_crtcs. :)
>> 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.
Parallel enables are handled by connection_mutex, so only 1 can happen at the same time.
It can race with a parallel crtc update to another crtc, but either way the ddb value is the same,
so it's harmless as long as it grabs the crtc_state at some point before committing.

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