| Message ID | 20170216164442.28704-1-krisman@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 16, 2017 at 02:44:42PM -0200, Gabriel Krisman Bertazi wrote: > Despite the documentation claim that cleanup_fb will match prior calls > to prepare_fb, in case of NULL framebuffers in the transitional helpers, > the code will skip the call to prepare_fb but not the corresponding > cleanup_fb call. This asymmetry in semantics is unnecessarily surprising > for developers transitioning drivers to atomic model, specially because > the final atomic handlers don't have the issue - the prepare_fb is > always called, despite the new state framebuffer being null. > > The only current user of the transitional helper that doesn't take care > of null framebuffers explicitly inside the prepare_fb hook is > atmel_hlcdc, so we take special care to make sure we don't break > anything there. Oh dear, I noticed all the callers to drm_plane_helper_disable :( What they really all want to do is shut down the entire display pipe on driver unload, with drm_atomic_helper_disable_all(). A bit more work to do in a simple cleanup, but care to at least do a small patch to Documentation/gpu/todo.rst to note it? Thanks, Daniel > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 +++ > drivers/gpu/drm/drm_plane_helper.c | 3 +-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > index bd2791c4b002..886ed5d8e304 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > @@ -772,6 +772,9 @@ static int atmel_hlcdc_plane_prepare_fb(struct drm_plane *p, > drm_plane_state_to_atmel_hlcdc_plane_state(s); > int ret; > > + if (!new_state->fb) > + return 0; > + > ret = atmel_hlcdc_layer_update_start(&plane->layer); > if (!ret) > state->prepared = true; > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index 148688fb920a..d8639e46bd2b 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -450,8 +450,7 @@ int drm_plane_helper_commit(struct drm_plane *plane, > goto out; > } > > - if (plane_funcs->prepare_fb && plane_state->fb && > - plane_state->fb != old_fb) { > + if (plane_funcs->prepare_fb && plane_state->fb != old_fb) { > ret = plane_funcs->prepare_fb(plane, > plane_state); > if (ret) > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, Feb 26, 2017 at 09:57:16PM +0100, Daniel Vetter wrote: > On Thu, Feb 16, 2017 at 02:44:42PM -0200, Gabriel Krisman Bertazi wrote: > > Despite the documentation claim that cleanup_fb will match prior calls > > to prepare_fb, in case of NULL framebuffers in the transitional helpers, > > the code will skip the call to prepare_fb but not the corresponding > > cleanup_fb call. This asymmetry in semantics is unnecessarily surprising > > for developers transitioning drivers to atomic model, specially because > > the final atomic handlers don't have the issue - the prepare_fb is > > always called, despite the new state framebuffer being null. > > > > The only current user of the transitional helper that doesn't take care > > of null framebuffers explicitly inside the prepare_fb hook is > > atmel_hlcdc, so we take special care to make sure we don't break > > anything there. > > Oh dear, I noticed all the callers to drm_plane_helper_disable :( What > they really all want to do is shut down the entire display pipe on driver > unload, with drm_atomic_helper_disable_all(). > > A bit more work to do in a simple cleanup, but care to at least do a small > patch to Documentation/gpu/todo.rst to note it? Patch itself applied to drm-misc, thanks. -Daniel
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index bd2791c4b002..886ed5d8e304 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -772,6 +772,9 @@ static int atmel_hlcdc_plane_prepare_fb(struct drm_plane *p, drm_plane_state_to_atmel_hlcdc_plane_state(s); int ret; + if (!new_state->fb) + return 0; + ret = atmel_hlcdc_layer_update_start(&plane->layer); if (!ret) state->prepared = true; diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 148688fb920a..d8639e46bd2b 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -450,8 +450,7 @@ int drm_plane_helper_commit(struct drm_plane *plane, goto out; } - if (plane_funcs->prepare_fb && plane_state->fb && - plane_state->fb != old_fb) { + if (plane_funcs->prepare_fb && plane_state->fb != old_fb) { ret = plane_funcs->prepare_fb(plane, plane_state); if (ret)
Despite the documentation claim that cleanup_fb will match prior calls to prepare_fb, in case of NULL framebuffers in the transitional helpers, the code will skip the call to prepare_fb but not the corresponding cleanup_fb call. This asymmetry in semantics is unnecessarily surprising for developers transitioning drivers to atomic model, specially because the final atomic handlers don't have the issue - the prepare_fb is always called, despite the new state framebuffer being null. The only current user of the transitional helper that doesn't take care of null framebuffers explicitly inside the prepare_fb hook is atmel_hlcdc, so we take special care to make sure we don't break anything there. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 +++ drivers/gpu/drm/drm_plane_helper.c | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-)