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

[Ying Liu]

On Wed, May 25, 2016 at 6:30 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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 at 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 at 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.

Again, please take a look at the kdoc of drm_helper_crtc_mode_set.
It reads 'Besides the atomic plane helper functions for the primary
plane the driver must also provide the ->mode_set_nofb callback
to set up the CRTC.  This is a transitional helper useful for converting
drivers to the atomic interfaces.'

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

drm_helper_crtc_mode_set calls drm_helper_crtc_mode_set_base.
Both drm_helper_crtc_mode_set_base and drm_plane_helper_disable
call drm_plane_helper_commit, so they are somewhat counterparts,
that is to say,  drm_helper_crtc_mode_set may call
drm_plane_helper_disable technically.

Regards,
Liu Ying

> -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 at 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