Re: [Intel-gfx] [PATCH 06/26] drm/atomic: Add __drm_atomic_get_current_plane_state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, May 30, 2016 at 01:42:40PM +0200, Maarten Lankhorst wrote:
> Op 29-05-16 om 20:35 schreef Daniel Vetter:
> > ... and use it in msm&vc4. Again just want to encapsulate
> > drm_atomic_state internals a bit.
> >
> > The const threading is a bit awkward in vc4 since C sucks, but I still
> > think it's worth to enforce this. Eventually I want to make all the
> > obj->state pointers const too, but that's a lot more work ...
> >
> > Cc: Eric Anholt <eric@xxxxxxxxxx>
> > Cc: Rob Clark <robdclark@xxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 10 +++------
> >  drivers/gpu/drm/vc4/vc4_crtc.c           | 11 +++-------
> >  drivers/gpu/drm/vc4/vc4_drv.h            |  2 +-
> >  drivers/gpu/drm/vc4/vc4_plane.c          |  5 +++--
> >  include/drm/drm_atomic.h                 | 36 ++++++++++++++++++++++++++++++++
> >  5 files changed, 46 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> > index 88fe256c1931..6d4086ee0503 100644
> > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> > @@ -383,19 +383,15 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
> >  	 */
> >  	hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
> >  	drm_atomic_crtc_state_for_each_plane(plane, state) {
> > -		struct drm_plane_state *pstate;
> > +		const struct drm_plane_state *pstate;
> >  		if (cnt >= (hw_cfg->lm.nb_stages)) {
> >  			dev_err(dev->dev, "too many planes!\n");
> >  			return -EINVAL;
> >  		}
> >  
> > -		pstate = state->state->plane_states[drm_plane_index(plane)];
> > +		pstate = __drm_atomic_get_current_plane_state(state->state,
> > +							      plane);
> >  
> > -		/* plane might not have changed, in which case take
> > -		 * current state:
> > -		 */
> > -		if (!pstate)
> > -			pstate = plane->state;
> >  		pstates[cnt].plane = plane;
> >  		pstates[cnt].state = to_mdp5_plane_state(pstate);
> >  
> > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> > index 904d0754ad78..703bda170105 100644
> > --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> > @@ -405,14 +405,9 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
> >  		return -EINVAL;
> >  
> >  	drm_atomic_crtc_state_for_each_plane(plane, state) {
> > -		struct drm_plane_state *plane_state =
> > -			state->state->plane_states[drm_plane_index(plane)];
> > -
> > -		/* plane might not have changed, in which case take
> > -		 * current state:
> > -		 */
> > -		if (!plane_state)
> > -			plane_state = plane->state;
> > +		const struct drm_plane_state *plane_state =
> > +			__drm_atomic_get_current_plane_state(state->state,
> > +							     plane);
> >  
> >  		dlist_count += vc4_plane_dlist_size(plane_state);
> >  	}
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index 37cac59401d7..c799baabc008 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -469,7 +469,7 @@ int vc4_kms_load(struct drm_device *dev);
> >  struct drm_plane *vc4_plane_init(struct drm_device *dev,
> >  				 enum drm_plane_type type);
> >  u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist);
> > -u32 vc4_plane_dlist_size(struct drm_plane_state *state);
> > +u32 vc4_plane_dlist_size(const struct drm_plane_state *state);
> >  void vc4_plane_async_set_fb(struct drm_plane *plane,
> >  			    struct drm_framebuffer *fb);
> >  
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index 4037b52fde31..5d2c3d9fd17a 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -690,9 +690,10 @@ u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist)
> >  	return vc4_state->dlist_count;
> >  }
> >  
> > -u32 vc4_plane_dlist_size(struct drm_plane_state *state)
> > +u32 vc4_plane_dlist_size(const struct drm_plane_state *state)
> >  {
> > -	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
> > +	const struct vc4_plane_state *vc4_state =
> > +		container_of(state, typeof(*vc4_state), base);
> >  
> >  	return vc4_state->dlist_count;
> >  }
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 92c84e9ab09a..4e97186293be 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -109,6 +109,42 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state,
> >  	return state->connector_states[index];
> >  }
> >  
> > +/**
> > + * __drm_atomic_get_current_plane_state - get current plane state
> > + * @state: global atomic state object
> > + * @plane: plane to grab
> > + *
> > + * This function returns the plane state for the given plane, either from
> > + * @state, or if the plane isn't part of the atomic state update, from @plane.
> > + * This is useful in atomic check callbacks, when drivers need to peek at, but
> > + * not change, state of other planes, since it avoids threading an error code
> > + * back up the call chain.
> > + *
> > + * WARNING:
> > + *
> > + * Note that this function is in general unsafe since it doesn't check for the
> > + * required locking for access state structures. Drivers must ensure that it is
> > + * save to access the returned state structure through other means. One commone
> > + * example is when planes are fixed to a single CRTC, and the driver knows that
> > + * the CRTC locks is held already. In that case holding the CRTC locks gives a
> > + * read-lock on all planes connected to that CRTC. But if planes can be
> > + * reassigned things get more tricky. In that case it's better to use
> > + * drm_atomic_get_plane_state and wire up full error handling.
> > + *
> > + * Returns:
> > + *
> > + * Read-only pointer to the current plane state.
> > + */
> > +static inline const struct drm_plane_state *
> > +__drm_atomic_get_current_plane_state(struct drm_atomic_state *state,
> > +				     struct drm_plane *plane)
> > +{
> > +	if (state->plane_states[drm_plane_index(plane)])
> > +		return state->plane_states[drm_plane_index(plane)];
> Could use a debug WARN_ON here. Just in case someone tries to use this for a plane that can't be safely checked:
> 
> WARN_ON(!plane->state->crtc || !drm_modeset_is_locked(&plane->state->crtc->mutex));

If you have hw like i915, where you can't move planes, this check isn't
good enough, because plane->state->crtc might be NULL, but if you hold the
right crtc mutex it's still perfectly safe. That's why I didn't add that
check, and instead typed the really long warning.

> Also use drm_atomic_get_existing_plane_state?

Hm, feels like overkill since this is a core atomic function. We don't use
get_existing_* in drm_atomic.c much either.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux