On Mon, Jun 13, 2016 at 3:58 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Sun, Jun 12, 2016 at 05:01:27PM +0800, Ying Liu wrote:
On Wed, Jun 8, 2016 at 8:19 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Drivers transitioning to atomic might not yet want to enable full DRIVER_ATOMIC support when it's not entirely working. But using atomic internally makes a lot more sense earlier.
Instead of spreading such flags to more places I figured it's simpler to just check for mode_config->funcs->atomic_commit, and use atomic paths if that is set. For the only driver currently transitioning (i915) this does the right thing.
Well, imx-drm is transitioning. Unfortunately, after applying this patch, legacy fbdev cannot enable displays when the imx-drm transitional patch set reaches phase 3 step 1[1].
For those transitioning drivers with fine-grained steps, this patch is likely to expose the atomic function __drm_atomic_helper_set_config to legacy fbdev support too early. They could still be using drm_crtc_helper_set_config when adding ->atomic_commit.
BTW, this patch and those for generic nonblocking commit are making the imx-drm transition a bit harder. Not good timing probably. But, I'll go on with the task anyway.
Hm, making transition harder wasn't really the intention. What exactly is blowing up for you? At least how I planned the transition the first thing you should be able to do is basic modesets (once you fill out ->atomic_commit), so I hoped that this wouldn't cause trouble.
At the imx-drm transition phase 1, ipu_plane_atomic_check checks crtc->enabled and hopes it to be true. Till phase 3 step 1, this function is not changed. This patch exposes restore_fbdev_mode_atomic and pan_display_atomic to legacy fbdev too early. Both of them call drm_atomic_commit which does plane check at atomic check stage. Then, the plane check fails for the legacy fbdev, because drm_atomic_commit sets crtc->enabled later than the legacy path drm_crtc_helper_set_mode does. Actually, drm_atomic_commit doesn't set crtc->enabled until the commit stage if you use the atomic helper.
OTOH, we fill out ->atomic_commit with drm_atomic_helper_commit at phase 3 step 1, then exposing drm_atomic_commit means that we need to handle crtc_state->event no later than phase 3 step 1... I haven't decided the right order/process to add the handling.
So, would it be an option to revert this patch...
Wrt nonblocking commit helpers breaking transitioning drivers: The most likely cause is your driver isn't handling crtc_state->event yet. Before you start using ->atomic_commit or one of the helpers to map legacy ioctl to atomic, you need to fill out some code to handle that in ->atomic_begin or ->atomic_flush. Then the transition should still work as before.
I do have confusion on the event handling in some drivers' ->atomic_flush. At least sun4i, kirin and fsl-dcu have the below snip: ========================================== if (event) { crtc->state->event = NULL;
spin_lock_irq(&crtc->dev->event_lock); if (drm_crtc_vblank_get(crtc) == 0) drm_crtc_arm_vblank_event(crtc, event); else drm_crtc_send_vblank_event(crtc, event); spin_unlock_irq(&crtc->dev->event_lock); } ========================================== It looks drm_crtc_vblank_get seldom fails? And why do we send vblank event when it fails?
Regards, Liu Ying
I'll be happy to help you out debug this, and then update my blog post with the transition plan with the latest findings.
Thanks, Daniel
[1] https://patchwork.kernel.org/patch/9144035/
Regards, Liu Ying
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_fb_helper.c | 6 ++---- drivers/gpu/drm/i915/intel_fbdev.c | 2 -- include/drm/drm_fb_helper.h | 11 ----------- 3 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index c0e0a2e78d75..ba2fcb2a68ad 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -385,7 +385,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
drm_warn_on_modeset_not_all_locked(dev);
if (fb_helper->atomic)
if (dev->mode_config.funcs->atomic_commit) return restore_fbdev_mode_atomic(fb_helper); drm_for_each_plane(plane, dev) {@@ -716,8 +716,6 @@ int drm_fb_helper_init(struct drm_device *dev, i++; }
fb_helper->atomic = !!drm_core_check_feature(dev, DRIVER_ATOMIC);return 0;out_free: drm_fb_helper_crtc_free(fb_helper); @@ -1344,7 +1342,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, return -EBUSY; }
if (fb_helper->atomic) {
if (dev->mode_config.funcs->atomic_commit) { ret = pan_display_atomic(var, info); goto unlock; }diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index ef8e67690f3d..4c725ad6fb54 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -724,8 +724,6 @@ int intel_fbdev_init(struct drm_device *dev) return ret; }
ifbdev->helper.atomic = true;dev_priv->fbdev = ifbdev; INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5b4aa35026a3..db8d4780eaa2 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -212,17 +212,6 @@ struct drm_fb_helper { * needs to be reprobe when fbdev is in control again. */ bool delayed_hotplug;
/*** @atomic:** Use atomic updates for restore_fbdev_mode(), etc. This defaults to* true if driver has DRIVER_ATOMIC feature flag, but drivers can* override it to true after drm_fb_helper_init() if they support atomic* modeset but do not yet advertise DRIVER_ATOMIC (note that fb-helper* does not require ASYNC commits).*/bool atomic;};
#ifdef CONFIG_DRM_FBDEV_EMULATION
2.8.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch