| Message ID | 1464546923-13439-7-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@anholt.net> > Cc: Rob Clark <robdclark@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > 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)); Also use drm_atomic_get_existing_plane_state? ~Maarten
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@anholt.net> > > Cc: Rob Clark <robdclark@gmail.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > 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
Op 30-05-16 om 17:05 schreef Daniel Vetter: > 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@anholt.net> >>> Cc: Rob Clark <robdclark@gmail.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> 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. Wrong! If crtc == NULL, you can set for example a different source rectangle, and plane->state would change from underneath you since you don't hold the plane mutex. >> 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. Ok, but that's an oversight.. Looking at drm_atomic.c it seems to me that drm_atomic_state_default_clear should use for_each_xx_in too.. drm_atomic_get_{crtc,plane}_state uses get_existing_state, drm_atomic_get_connector_state does not, since it duplicated some checks in drm_atomic_get_connector_state. It is probably worth it to convert drm_atomic_get_connector_state too. Apart from that I don't see what you mean.. ~Maarten
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)]; + + return plane->state; +} + int __must_check drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, struct drm_display_mode *mode);
... 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@anholt.net> Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- 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(-)