On Mon, Jun 20, 2016 at 01:55:57PM +0800, Ying Liu wrote:
> 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?
That's how it's supposed to work: When the vblank is off, then you can't
send out vblank events. If the pipe gets (re)enabled, then you need to
move the vblank generation to where it's fully on. If it stays disabled,
then you can directly call drm_crtc_send_vblank_event.
btw much easier to discuss this on irc.
-Daniel
>
> 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
--
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
