[PATCH 23/27] drm: Replace fb_helper->atomic with mode_config->atomic_commit
Ying Liu
gnuiyl at gmail.com
Mon Jun 13 09:26:28 UTC 2016
On Mon, Jun 13, 2016 at 3:58 PM, Daniel Vetter <daniel at 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 at 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 at linux.intel.com>
>> > Signed-off-by: Daniel Vetter <daniel.vetter at 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 at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
More information about the dri-devel
mailing list