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 at anholt.net>
>>> Cc: Rob Clark <robdclark at gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter at 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
