| Message ID | 1464084653-16684-6-git-send-email-gnuiyl@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -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
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. 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
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
On Wed, May 25, 2016 at 6:30 PM, Daniel Vetter <daniel@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@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. 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@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
On Thu, May 26, 2016 at 11:02:55AM +0800, Ying Liu wrote: > On Wed, May 25, 2016 at 6:30 PM, Daniel Vetter <daniel@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@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. > > 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.' Oops, my apologies, I mixed it all up in my head. Sorry for all the mess, I'll start a new thread since this discussion here is a bit confusion now. -Daniel > > 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@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
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. > > 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); > + if (ret) > + goto out; > + Ok, so there's no problem with mixing up legacy and transitional helpers here. It's still not how it's supposed to work though. All the helpers (legacy, transitional & atomic) assume that when the helpers call into crtc->disable hooks, the driver will also disable all the scanout engines. The helpers will _not_ do that for you, it's the driver duties. For atomic we have a helper function which you can call at the right place, drm_atomic_helper_disable_planes_on_crtc(). Unfortunately it looks like no one ever used it, so probably a bunch of bugs in drivers because of this. Explicitly disabling the plane when the crtc is shut down might confused/break drivers which get this right, hence why we can't have this in the helper libraries. In short: You need to fix this in your driver, by manually disabling the plane at a suitable place first. Most likely that's in the crtc->prepare hook (since you're still transitioning). -Daniel > 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
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); + if (ret) + goto out; + swap(crtc->state, crtc_state); crtc_funcs->mode_set_nofb(crtc);
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. Signed-off-by: Liu Ying <gnuiyl@gmail.com> --- drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++ 1 file changed, 9 insertions(+)