Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

From: Daniel Vetter <daniel@ffwll.ch>
To: Ying Liu <gnuiyl@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set
Date: Wed, 25 May 2016 12:30:05 +0200	[thread overview]
Message-ID: <20160525103004.GH27098@phenom.ffwll.local> (raw)
In-Reply-To: <CAOcKUNUWVct4tsMkV0r4aiTyahQ9-SsR3nBe6oQSh9Bf1jnnhQ@mail.gmail.com>

On Wed, May 25, 2016 at 05:37:41PM +0800, Ying Liu wrote:
> On Tue, May 24, 2016 at 6:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 24, 2016 at 06:10:44PM +0800, Liu Ying wrote:
> >> Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
> >> to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
> >> in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
> >> update in plane_funcs->atomic_check() happy.
> >
> > How/where exactly do you blow up?
> >>
> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >> index 79555d2..7fabcd7 100644
> >> --- a/drivers/gpu/drm/drm_crtc_helper.c
> >> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> >> @@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
> >>                       goto out;
> >>       }
> >>
> >> +     /*
> >> +      * It doesn't hurt to disable primary plane here since crtc is off
> >> +      * and we'll enable it again in drm_helper_crtc_mode_set_base()
> >> +      * below soon.
> >> +      */
> >> +     ret = drm_plane_helper_disable(crtc->primary);
> >
> > You can't call this helper from crtc helpers. drm_plane_helper_disable
> > assumes that a) you've already extracted universal plane support for the
> > primary plane by registering a proper drm_plane for it b) that you're
> > driver is at least partially using atomic hooks already.
> >
> > Both assumptions are wrong for all current users of this function.
> > drm_helper_crtc_mode_set() is purely a legacy helper for legecy drivers.
> 
> Isn't the function a transitional helper, just as the kdoc claims?
> It looks all current users are sort of atomic users already.

drm_helper_crtc_mode_set is exclusively used by legacy drivers.
drm_plane_helper_disable can be used for transitioning to atomic.

Calling the latter from the former means you break every non-transition
legacy driver.
-Daniel

> 
> Regards,
> Liu Ying
> 
> > -Daniel
> >
> >> +     if (ret)
> >> +             goto out;
> >> +
> >>       swap(crtc->state, crtc_state);
> >>
> >>       crtc_funcs->mode_set_nofb(crtc);
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel