On Tue, Nov 25, 2014 at 5:50 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxx>
> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
My canary is still alive.
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
Lightly-Tested-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
