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;
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;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