[PATCH] drm: Handle atomic state properly in kms getfoo ioctl

[Sean Paul]

On Tue, Nov 25, 2014 at 5:50 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> So the problem with async commit (especially async modeset commit) is
> that the legacy pointers only get updated after the point of no
> return, in the async part of the modeset sequence. At least as
> implemented by the current helper functions. This is done in the
> set_routing_links function in drm_atomic_helper.c.
>
> Which also means that access isn't protected by locks but only
> coordinated by synchronizing with async workers. No problem thus far,
> until we lock at the getconnector/encoder ioctls.
>
> So fix this up by adding special cases for atomic drivers: For those
> we need to look at state objects. Unfortunately digging out the
> correct encoder->crtc link is a bit of work, so wrap this up in a
> helper function.
>
> Moving the assignments of connector->encoder and encoder->crtc earlier
> isn't a good idea because the point of the atomic helpers is that we
> stage the state updates. That way the disable functions can still
> inspect the links and rely upon them.
>
> v2: Extract full encoder->crtc lookup into helper (Rob).
>
> v3: Extract drm_connector_get_encoder too since - we need to always
> return state->best_encoder when there is a state otherwise we might
> return stale data if there's a pending async disable (and chase
> unlocked pointers, too). Same issue with encoder_get_crtc but there
> it's a bit more tricky to handle.
>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Sean Paul <seanpaul at chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>


My canary is still alive.

Reviewed-by: Sean Paul <seanpaul at chromium.org>
Lightly-Tested-by: Sean Paul <seanpaul at chromium.org>


> ---
>  drivers/gpu/drm/drm_crtc.c | 49 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 589a921d4313..d51b1c1f6726 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1955,6 +1955,15 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>         return true;
>  }
>
> +static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *connector)
> +{
> +       /* For atomic drivers only state objects are synchronously updated and
> +        * protected by modeset locks, so check those first. */
> +       if (connector->state)
> +               return connector->state->best_encoder;
> +       return connector->encoder;
> +}
> +
>  /**
>   * drm_mode_getconnector - get connector configuration
>   * @dev: drm device for the ioctl
> @@ -1973,6 +1982,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  {
>         struct drm_mode_get_connector *out_resp = data;
>         struct drm_connector *connector;
> +       struct drm_encoder *encoder;
>         struct drm_display_mode *mode;
>         int mode_count = 0;
>         int props_count = 0;
> @@ -2028,8 +2038,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>         out_resp->subpixel = connector->display_info.subpixel_order;
>         out_resp->connection = connector->status;
>         drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -       if (connector->encoder)
> -               out_resp->encoder_id = connector->encoder->base.id;
> +
> +       encoder = drm_connector_get_encoder(connector);
> +       if (encoder)
> +               out_resp->encoder_id = encoder->base.id;
>         else
>                 out_resp->encoder_id = 0;
>         drm_modeset_unlock(&dev->mode_config.connection_mutex);
> @@ -2099,6 +2111,33 @@ out:
>         return ret;
>  }
>
> +static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
> +{
> +       struct drm_connector *connector;
> +       struct drm_device *dev = encoder->dev;
> +       bool uses_atomic = false;
> +
> +       /* For atomic drivers only state objects are synchronously updated and
> +        * protected by modeset locks, so check those first. */
> +       list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +               if (!connector->state)
> +                       continue;
> +
> +               uses_atomic = true;
> +
> +               if (connector->state->best_encoder != encoder)
> +                       continue;
> +
> +               return connector->state->crtc;
> +       }
> +
> +       /* Don't return stale data (e.g. pending async disable). */
> +       if (uses_atomic)
> +               return NULL;
> +
> +       return encoder->crtc;
> +}
> +
>  /**
>   * drm_mode_getencoder - get encoder configuration
>   * @dev: drm device for the ioctl
> @@ -2117,6 +2156,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>  {
>         struct drm_mode_get_encoder *enc_resp = data;
>         struct drm_encoder *encoder;
> +       struct drm_crtc *crtc;
>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
> @@ -2126,7 +2166,10 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>                 return -ENOENT;
>
>         drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -       if (encoder->crtc)
> +       crtc = drm_encoder_get_crtc(encoder);
> +       if (crtc)
> +               enc_resp->crtc_id = crtc->base.id;
> +       else if (encoder->crtc)
>                 enc_resp->crtc_id = encoder->crtc->base.id;
>         else
>                 enc_resp->crtc_id = 0;
> --
> 2.1.1
>