| Message ID | 20170627145936.18983-10-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 27-06-17 om 16:59 schreef Daniel Vetter: > Like with panning and modesetting, and like with those, stick with > simple drm_modeset_locking_all for the legacy path, and the full > atomic dance for atomic drivers. > > This means a bit more boilerplate since setting up the atomic state > machinery is rather verbose, but then this is shared code for 30+ > drivers or so, so meh. > > After this patch there's only the LUT/cmap path which is still using > drm_modeset_lock_all for an atomic driver. But Peter is already > locking into reworking that, so I'll leave that code as-is for now. > > Cc: Peter Rosin <peda@axentia.se> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Thierry Reding <treding@nvidia.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 88 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 76 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 59e2916471b2..bbd4c6d0378d 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -616,23 +616,13 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { > static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; > #endif > > -static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) > +static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode) > { > - struct drm_fb_helper *fb_helper = info->par; > struct drm_device *dev = fb_helper->dev; > struct drm_crtc *crtc; > struct drm_connector *connector; > int i, j; > > - /* > - * For each CRTC in this fb, turn the connectors on/off. > - */ > - mutex_lock(&fb_helper->lock); > - if (!drm_fb_helper_is_bound(fb_helper)) { > - mutex_unlock(&fb_helper->lock); > - return; > - } > - > drm_modeset_lock_all(dev); > for (i = 0; i < fb_helper->crtc_count; i++) { > crtc = fb_helper->crtc_info[i].mode_set.crtc; > @@ -649,6 +639,81 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) > } > } > drm_modeset_unlock_all(dev); > +} > + > +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode) > +{ > + struct drm_device *dev = fb_helper->dev; > + struct drm_atomic_state *state; > + int i, ret; > + > + struct drm_modeset_acquire_ctx ctx; > + > + drm_modeset_acquire_init(&ctx, 0); > + > + state = drm_atomic_state_alloc(dev); > + if (!state) { > + ret = -ENOMEM; > + goto out_ctx; > + } > + > + state->acquire_ctx = &ctx; > +retry: > + for (i = 0; i < fb_helper->crtc_count; i++) { > + struct drm_crtc_state *crtc_state; > + struct drm_crtc *crtc; > + > + if (!fb_helper->crtc_info[i].mode_set.mode) > + continue; > + > + crtc = fb_helper->crtc_info[i].mode_set.crtc; > + > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) { > + ret = PTR_ERR(crtc_state); > + goto out_state; > + } Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ? I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state, similar to how DPMS on is ignored without CRTC. > + crtc_state->active = dpms_mode == DRM_MODE_DPMS_ON; > + } > + > + ret = drm_atomic_commit(state); > +out_state: > + if (ret == -EDEADLK) > + goto backoff; > + > + drm_atomic_state_put(state); > +out_ctx: > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > + return; > + > +backoff: > + drm_atomic_state_clear(state); > + drm_modeset_backoff(&ctx); > + > + goto retry; > + > +} > + > +static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + > + /* > + * For each CRTC in this fb, turn the connectors on/off. > + */ > + mutex_lock(&fb_helper->lock); > + if (!drm_fb_helper_is_bound(fb_helper)) { > + mutex_unlock(&fb_helper->lock); > + return; > + } > + > + if (drm_drv_uses_atomic_modeset(fb_helper->dev)) > + dpms_atomic(fb_helper, dpms_mode); > + else > + dpms_legacy(fb_helper, dpms_mode); > mutex_unlock(&fb_helper->lock); > } > > @@ -2467,7 +2532,6 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > */ > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > - struct drm_device *dev = fb_helper->dev; > int err = 0; > > if (!drm_fbdev_emulation)
On Thu, Jun 29, 2017 at 12:22 PM, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: >> +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode) >> +{ >> + struct drm_device *dev = fb_helper->dev; >> + struct drm_atomic_state *state; >> + int i, ret; >> + >> + struct drm_modeset_acquire_ctx ctx; >> + >> + drm_modeset_acquire_init(&ctx, 0); >> + >> + state = drm_atomic_state_alloc(dev); >> + if (!state) { >> + ret = -ENOMEM; >> + goto out_ctx; >> + } >> + >> + state->acquire_ctx = &ctx; >> +retry: >> + for (i = 0; i < fb_helper->crtc_count; i++) { >> + struct drm_crtc_state *crtc_state; >> + struct drm_crtc *crtc; >> + >> + if (!fb_helper->crtc_info[i].mode_set.mode) >> + continue; >> + >> + crtc = fb_helper->crtc_info[i].mode_set.crtc; >> + >> + crtc_state = drm_atomic_get_crtc_state(state, crtc); >> + if (IS_ERR(crtc_state)) { >> + ret = PTR_ERR(crtc_state); >> + goto out_state; >> + } > Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ? > > I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state, > similar to how DPMS on is ignored without CRTC. I just blindly smashed the old fbdev code in with the helper and moved the locking out. Not sure what would be better here, since the continue is in a way just part of a non-existent drm_fb_helper_for_each_active_crtc loop iterator. Not sure it's worth it to overpolish this code to such an extent :-) -Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 59e2916471b2..bbd4c6d0378d 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -616,23 +616,13 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; #endif -static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) +static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode) { - struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct drm_crtc *crtc; struct drm_connector *connector; int i, j; - /* - * For each CRTC in this fb, turn the connectors on/off. - */ - mutex_lock(&fb_helper->lock); - if (!drm_fb_helper_is_bound(fb_helper)) { - mutex_unlock(&fb_helper->lock); - return; - } - drm_modeset_lock_all(dev); for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc; @@ -649,6 +639,81 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) } } drm_modeset_unlock_all(dev); +} + +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode) +{ + struct drm_device *dev = fb_helper->dev; + struct drm_atomic_state *state; + int i, ret; + + struct drm_modeset_acquire_ctx ctx; + + drm_modeset_acquire_init(&ctx, 0); + + state = drm_atomic_state_alloc(dev); + if (!state) { + ret = -ENOMEM; + goto out_ctx; + } + + state->acquire_ctx = &ctx; +retry: + for (i = 0; i < fb_helper->crtc_count; i++) { + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + + if (!fb_helper->crtc_info[i].mode_set.mode) + continue; + + crtc = fb_helper->crtc_info[i].mode_set.crtc; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) { + ret = PTR_ERR(crtc_state); + goto out_state; + } + + crtc_state->active = dpms_mode == DRM_MODE_DPMS_ON; + } + + ret = drm_atomic_commit(state); +out_state: + if (ret == -EDEADLK) + goto backoff; + + drm_atomic_state_put(state); +out_ctx: + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return; + +backoff: + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + + goto retry; + +} + +static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) +{ + struct drm_fb_helper *fb_helper = info->par; + + /* + * For each CRTC in this fb, turn the connectors on/off. + */ + mutex_lock(&fb_helper->lock); + if (!drm_fb_helper_is_bound(fb_helper)) { + mutex_unlock(&fb_helper->lock); + return; + } + + if (drm_drv_uses_atomic_modeset(fb_helper->dev)) + dpms_atomic(fb_helper, dpms_mode); + else + dpms_legacy(fb_helper, dpms_mode); mutex_unlock(&fb_helper->lock); } @@ -2467,7 +2532,6 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); */ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { - struct drm_device *dev = fb_helper->dev; int err = 0; if (!drm_fbdev_emulation)
Like with panning and modesetting, and like with those, stick with simple drm_modeset_locking_all for the legacy path, and the full atomic dance for atomic drivers. This means a bit more boilerplate since setting up the atomic state machinery is rather verbose, but then this is shared code for 30+ drivers or so, so meh. After this patch there's only the LUT/cmap path which is still using drm_modeset_lock_all for an atomic driver. But Peter is already locking into reworking that, so I'll leave that code as-is for now. Cc: Peter Rosin <peda@axentia.se> Cc: John Stultz <john.stultz@linaro.org> Cc: Thierry Reding <treding@nvidia.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_fb_helper.c | 88 +++++++++++++++++++++++++++++++++++------ 1 file changed, 76 insertions(+), 12 deletions(-)