| Message ID | 1416955805-31832-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 25, 2014 at 5:50 PM, Daniel Vetter <daniel.vetter@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@gmail.com> > Cc: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> My canary is still alive. Reviewed-by: Sean Paul <seanpaul@chromium.org> Lightly-Tested-by: Sean Paul <seanpaul@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 >
On Wed, Nov 26, 2014 at 10:52:24AM -0500, Sean Paul wrote: > On Tue, Nov 25, 2014 at 5:50 PM, Daniel Vetter <daniel.vetter@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@gmail.com> > > Cc: Sean Paul <seanpaul@chromium.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > My canary is still alive. > > Reviewed-by: Sean Paul <seanpaul@chromium.org> > Lightly-Tested-by: Sean Paul <seanpaul@chromium.org> Put onto my atomic fixes pile, thanks for review&testing. -Daniel
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;
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@gmail.com> Cc: Sean Paul <seanpaul@chromium.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_crtc.c | 49 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-)