Re: [PATCH 23/27] drm: Replace fb_helper->atomic with mode_config->atomic_commit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 13, 2016 at 3:58 PM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxxxxxxxxxxxxxx
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux