| Message ID | 1442184669-30990-9-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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, 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. Cheers, Daniel > }; > > u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg); > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > index 78ca353bfcf0..c7e0535c0e77 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > @@ -275,9 +275,27 @@ static void rcar_du_plane_atomic_update(struct drm_plane *plane, > struct drm_plane_state *old_state) > { > struct rcar_du_plane *rplane = to_rcar_plane(plane); > + struct rcar_du_plane_state *old_rstate; > + struct rcar_du_plane_state *new_rstate; > > - if (plane->state->crtc) > - rcar_du_plane_setup(rplane); > + if (!plane->state->crtc) > + return; > + > + rcar_du_plane_setup(rplane); > + > + /* Check whether the source has changed from memory to live source or > + * from live source to memory. The source has been 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. We thus need to restart the group if the source changes. > + */ > + old_rstate = to_rcar_plane_state(old_state); > + new_rstate = to_rcar_plane_state(plane->state); > + > + if ((old_rstate->source == RCAR_DU_PLANE_MEMORY) != > + (new_rstate->source == RCAR_DU_PLANE_MEMORY)) > + rplane->group->need_restart = true; > } > > static const struct drm_plane_helper_funcs rcar_du_plane_helper_funcs = { > -- > 2.4.6 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, 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. > > }; > > > > u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg); > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > > b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index > > 78ca353bfcf0..c7e0535c0e77 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > > @@ -275,9 +275,27 @@ static void rcar_du_plane_atomic_update(struct > > drm_plane *plane, > > struct drm_plane_state *old_state) > > { > > struct rcar_du_plane *rplane = to_rcar_plane(plane); > > + struct rcar_du_plane_state *old_rstate; > > + struct rcar_du_plane_state *new_rstate; > > > > - if (plane->state->crtc) > > - rcar_du_plane_setup(rplane); > > + if (!plane->state->crtc) > > + return; > > + > > + rcar_du_plane_setup(rplane); > > + > > + /* Check whether the source has changed from memory to live source or > > + * from live source to memory. The source has been 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. We thus need to restart the group if the source changes. > > + */ > > + old_rstate = to_rcar_plane_state(old_state); > > + new_rstate = to_rcar_plane_state(plane->state); > > + > > + if ((old_rstate->source == RCAR_DU_PLANE_MEMORY) != > > + (new_rstate->source == RCAR_DU_PLANE_MEMORY)) > > + rplane->group->need_restart = true; > > } > > > > static const struct drm_plane_helper_funcs rcar_du_plane_helper_funcs = {
On Thu, Dec 17, 2015 at 09:11:49AM +0200, Laurent Pinchart wrote: > Hi Daniel, > > 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. 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. 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 ;-) Cheers, Daniel
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 :-)
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; }; u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index 78ca353bfcf0..c7e0535c0e77 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -275,9 +275,27 @@ static void rcar_du_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { struct rcar_du_plane *rplane = to_rcar_plane(plane); + struct rcar_du_plane_state *old_rstate; + struct rcar_du_plane_state *new_rstate; - if (plane->state->crtc) - rcar_du_plane_setup(rplane); + if (!plane->state->crtc) + return; + + rcar_du_plane_setup(rplane); + + /* Check whether the source has changed from memory to live source or + * from live source to memory. The source has been 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. We thus need to restart the group if the source changes. + */ + old_rstate = to_rcar_plane_state(old_state); + new_rstate = to_rcar_plane_state(plane->state); + + if ((old_rstate->source == RCAR_DU_PLANE_MEMORY) != + (new_rstate->source == RCAR_DU_PLANE_MEMORY)) + rplane->group->need_restart = true; } static const struct drm_plane_helper_funcs rcar_du_plane_helper_funcs = {
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(-)