On Mon, Jun 13, 2016 at 10:01 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Mon, Jun 13, 2016 at 05:26:28PM +0800, Ying Liu wrote:
>> 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.
>
> Hm, my idea was that in phase 2 (when you roll out state structures and
> checks), you'd change your atomic_check functions from looking at legacy
> stuff like crtc->enabled, to instead look at new state like
> crtc_state->enabled.
>
>> 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.
>
> Yes, this is a change: Before you can start with phase 3 you need to
> handle crtc_state->event in atomic_flush (or a similar place).
>
>> So, would it be an option to revert this patch...
>
> I'd really like to avoid that - the old hack of adding a knob for every
> place we should use atomic was getting complicated :( And right now
> there's many more atomic drivers than legacy ones, and only 2 drivers in
> transition (i915 and imx). Given that I think we should optimize more for
> atomic drivers.
>
> But like I said, of course transition shouldn't be too painful.
>
>> > 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?
>
> If it never fails for you, then you haven't put drm_crtc_vblank_on/off()
> into your crtc enable/disable callbacks. Ofc this assumes you have real
> vblank support.
>
>> And why do we send vblank event when it fails?
>
> This is just the most failsafe way to make sure the event gets out. It's
> rather inaccurate though, so would be much better if you can make that
> decision by instead looking at crtc->state->active, like i915 does.
> drm_crtc_vblank_get always works in-betwen drm_crtc_vblank_on/off() calls,
> i.e. when the crtc is supposed to be on. And when you disable the crtc and
> it stays disable then you should send out the vblank event directly, after
> the pipe is off. But that's really the only case where you should call
> drm_crtc_send_vblank_event directly.
>
> The kerneldoc for these functions try to explain this a bit ...
>
> Anyway, pls don't look at these drivers - I just hacked them up because
> they didn't implement event handling at all. Which is breaking atomic. For
> a good example of how it should be done, look at rockchip (already merged
> in drm-misc) or i915 (patches on the m-l).
It looks calling drm_vblank_off in crtc_helper->disable will set vblank->enabled
to false and increase vblank->refcount by one. The next enable operation
could call crtc_helper->atomic_begin or atomic_flush and then
crtc_helper->enable. In ->atomic_begin or atomic_flush, it looks
drm_crtc_vblank_get could be called. The problem is that
drm_crtc_vblank_get finds vblank->refcount is not 1 after an add-1-operation
and vblank->enabled is false, which finally makes the function return
-EINVAL. How to handle this problem?
Regards,
Liu Ying
> -Daniel
>
>>
>> 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
>
> --
> 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
