Re: [PATCH 08/22] drm: rcar-du: Restart the DU group when a plane source changes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Thursday 17 December 2015 10:41:51 Daniel Vetter wrote:
> On Thu, Dec 17, 2015 at 09:11:49AM +0200, Laurent Pinchart wrote:
> > On Monday 14 September 2015 09:22:13 Daniel Vetter wrote:
> >> On Mon, Sep 14, 2015 at 01:50:55AM +0300, Laurent Pinchart wrote:
> >>> Plane sources are configured by the VSPS bit in the PnDDCR4 register.
> >>> Although the datasheet states that the bit is updated during vertical
> >>> blanking, it seems that updates only occur when the DU group is held
> >>> in reset through the DSYSR.DRES bit. Restart the group if the source
> >>> changes.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  4 ++++
> >>>  drivers/gpu/drm/rcar-du/rcar_du_group.c |  2 ++
> >>>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 ++
> >>>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 22 ++++++++++++++++++++--
> >>>  4 files changed, 28 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index
> >>> 48cb19949ca3..7e2f5c26d589
> >>> 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>> @@ -272,6 +272,10 @@ static void rcar_du_crtc_update_planes(struct
> >>> rcar_du_crtc *rcrtc)
> >>>  			rcar_du_group_restart(rcrtc->group);
> >>>  	}
> >>> 
> >>> +	/* Restart the group if plane sources have changed. */
> >>> +	if (rcrtc->group->need_restart)
> >>> +		rcar_du_group_restart(rcrtc->group);
> >>> +
> >>>  	mutex_unlock(&rcrtc->group->lock);
> >>>  	
> >>>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR,
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> >>> b/drivers/gpu/drm/rcar-du/rcar_du_group.c index
> >>> 4a44ddd51766..0e2b46dce563 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> >>> @@ -162,6 +162,8 @@ void rcar_du_group_start_stop(struct rcar_du_group
> >>> *rgrp, bool start)
> >>> 
> >>>  void rcar_du_group_restart(struct rcar_du_group *rgrp)
> >>>  {
> >>> +	rgrp->need_restart = false;
> >>> +
> >>>  	__rcar_du_group_start_stop(rgrp, false);
> >>>  	__rcar_du_group_start_stop(rgrp, true);
> >>>  }
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> >>> b/drivers/gpu/drm/rcar-du/rcar_du_group.h index
> >>> 4b1952fd4e7d..5e3adc6b31b5 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> >>> @@ -32,6 +32,7 @@ struct rcar_du_device;
> >>>   * @dptsr_planes: bitmask of planes driven by dot-clock and timing
> >>>   generator 1
> >>>   * @num_planes: number of planes in the group
> >>>   * @planes: planes handled by the group
> >>> + * @need_restart: the group needs to be restarted due to a
> >>> configuration change
> >>>   */
> >>>  struct rcar_du_group {
> >>>  	struct rcar_du_device *dev;
> >>> @@ -47,6 +48,7 @@ struct rcar_du_group {
> >>>  	unsigned int num_planes;
> >>>  	struct rcar_du_plane planes[RCAR_DU_NUM_KMS_PLANES];
> >>> 
> >>> +	bool need_restart;
> >> 
> >> My recommendation is to keep any of these intermediate values in state
> >> objects too. The reason is that eventually we want to also support real
> >> queues of atomic commits,
> > 
> > I like the idea :-)
> > 
> >> and then anything stored globally (well, outside of the state objects)
> >> won't work. And yes it's ok to push that kind of stuff into helper,
> >> this isn't really any different than e.g. crtc_state->active_changed
> >> and similar booleans indicating that something special needs to be done
> >> when committing.
> >
> > I agree with you. There's still a couple of state variables I need to
> > remove in the driver structures, and that's on my todo list (although not
> > very high I'm afraid).
> > 
> > The group state is a bit of an oddball. The DU groups CRTCs by two and
> > shares resources inside a group. Implementing that resource sharing
> > requires storing group state, which is very difficult without an atomic
> > state object for the group.
> > 
> > Am I missing an easy way to fix that ? And would it be fine to upstream
> > this patch as-is in the meantime ? I'd like to get it in v4.6, it's been
> > out for long enough and is blocking other people.
> 
> Involves a bit of typing, but we allow drivers to subclass
> drm_atomic_state and add whatever they want.

I hadn't noticed it had been introduced, that's very nice.

> The important part is to make sure you get the locking right, i.e. only ever
> duplicate anything when you hold the right locks. For that you have about 3
> options:
> - protect all group state with mode_config->connection_lock. Might
>   unecessarily serialize updates.
> - create a per-group ww_mutex (atomic allows you to throw as many
>   additional ww_mutex locks encapsulated within a drm_modeset_lock as you
>   want).
> - just grab the crtc states (plus locks) for all the crtcs in a group when
>   duplicating a group state
> 
> I highly recommend you follow the patterns laid out in drm_atomic.c and
> only allow your driver to get at the group state through a get_group_state
> helper, like we have for plane/crtc/connector states. That can then take
> care of the locking and everything.

Thanks for the tip, I'll give it a try.

> i915 uses this to keep track of shared resources like dpll and display
> core clock.
> 
> There should be kerneldoc for the individual functions, but I think we
> lack an overview/example section ... hint, hint ;-)

I'll first try to understand it by using it :-)

-- 
Regards,

Laurent Pinchart