Re: [PATCH v2 11/17] drm/i915: Use global atomic state for staged pll config

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 18, 2015 at 06:27:51PM +0200, Maarten Lankhorst wrote:
> Op 18-05-15 om 17:45 schreef Daniel Vetter:
> > On Wed, May 13, 2015 at 10:23:41PM +0200, Maarten Lankhorst wrote:
> >> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >>
> >> Now that we can subclass drm_atomic_state we can also use it to keep
> >> track of all the pll settings. atomic_state is a better place to hold
> >> all shared state than keeping pll->new_config everywhere.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h      |   1 -
> >>  drivers/gpu/drm/i915/intel_atomic.c  |  49 ++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++------------------------
> >>  drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
> >>  4 files changed, 95 insertions(+), 79 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index a0e4868653f2..0e200018c9aa 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -319,7 +319,6 @@ struct intel_shared_dpll_config {
> >>  
> >>  struct intel_shared_dpll {
> >>  	struct intel_shared_dpll_config config;
> >> -	struct intel_shared_dpll_config *new_config;
> >>  
> >>  	int active; /* count of number of active CRTCs (i.e. DPMS on) */
> >>  	bool on; /* is the PLL actually active? Disabled during modeset */
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >> index 7ed8033aae60..aff87054406c 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -421,3 +421,52 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> >>  
> >>  	return 0;
> >>  }
> >> +
> >> +static void intel_atomic_duplicate_dpll_state(struct drm_device *dev,
> >> +					      struct intel_atomic_state *state)
> >> +{
> >> +	struct drm_i915_private *dev_priv = to_i915(dev);
> >> +	struct intel_shared_dpll *pll;
> >> +	enum intel_dpll_id i;
> >> +
> >> +	state->dpll_set = true;
> > The ww mutex locking dance here is missing. For simplicity I think we
> > could just repurpose the dev->mode_config.connection_mutex. We need that
> > anyway full modesets, and dpll changes should only be required in those.
> > Which means this wont introduce any unecessary parallelism.
> >
> > It means though that we need to wire up a proper error code to all callers
> > of duplicate/get_dpll_state, like with all the other atomic states. Might
> > be best to follow the design for connector/crtc/planes an pass around
> > pointers with error codes explicitly (instead of returning
> > state->shared_dpll below since that can only cope with NULL and not with
> > -EDEADLK).
> >
> > Sorry that I didn't spot this earlier.
> >
> The dpll state should only ever be done during a modeset in which case the connection_mutex is guaranteed to be held.
> Instead of making this return a value can we add a lockdep_assert_held ?

The problem is that the atomic core might only grab the crtc state and
crtc mutex if e.g. userspace only changes the mode. But I've forgotten
that we're using the helpers to handle modesets, and those will acquire
all the needed connector states and hence the connection_mutex.

A locking check is therefore indeed enough. But please use
WARN_ON(!mutex_is_locked) since imo a locking check which can be compiled
out is useless. And the additional correctness lockdep provides isn't
needed - we'll catch the bug since no one always hits the race by doing
modesets in parallel ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx