Re: [PATCH 6/9] drm/atomic: better doc for implicit vs explicit fencing

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 20, 2018 at 03:04:58PM -0700, Eric Anholt wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> 
> > Note that a pile of drivers don't seem to take implicit fencing into
> > account, or at least don't call drm_atoimc_set_fence_for_plane().
> 
>                                      ^atomic
> 
> > Cc'ing relevant people, or at least some. Some drivers also look like
> > they don't disable implicit fencing (e.g. amdgpu) because the explicit
> > fences and implicit fences are handled by entirely independent code
> > paths.
> >
> > I also wonder whether we shouldn't just make the recommended helpers
> > the default ones, since a lot of drivers don't bother to handle the
> > implicit fences at all it seems. The helpers won't blow up even for
> > non-GEM drivers or GEM drivers which don't fill out the gem bo
> > pointers in struct drm_framebuffer.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Sinclair Yeh <syeh@vmware.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c             | 8 ++++++++
> >  include/drm/drm_modeset_helper_vtables.h | 5 ++++-
> >  include/drm/drm_plane.h                  | 7 ++++++-
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42f22db..ec77afbda0c3 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1492,6 +1492,14 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
> >   * Otherwise, if &drm_plane_state.fence is not set this function we just set it
> >   * with the received implicit fence. In both cases this function consumes a
> >   * reference for @fence.
> > + *
> > + * This way explicit fencing can be used to overrule implicit fencing, which is
> > + * important to make explicit fencing use-cases work: One example is using one
> > + * buffer for 2 screens with different refresh rates. Implicit fencing will
> > + * clamp rendering to the refresh rate of the slower screen, whereas explicit
> > + * fence allows 2 independent render and display loops on a single buffer. If a
> > + * driver allows obeys both implicit and explicit fences for plane updates, then
> > + * it will break all the benefits of explicit fencing.
> >   */
> 
> Thanks for explaining why we should care about explicit fencing for
> display!  I'd been trying and failing to generate a usecase.
> 
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index d6da26d66a4b..1e2622e33208 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -80,7 +80,12 @@ struct drm_plane_state {
> >  	 * @fence:
> >  	 *
> >  	 * Optional fence to wait for before scanning out @fb. Do not write this
> > -	 * directly, use drm_atomic_set_fence_for_plane()
> > +	 * directly, use drm_atomic_set_fence_for_plane(). The core atomic code
> > +	 * will set this when userspace is using explicit fencing.
> 
> Optional suggestion:
> 
> 	 * Optional fence to wait for before scanning out @fb. The core
> 	 * atomic code will set this when userspace is using explicit
> 	 * fencing. Do not write this directly for a driver's implicit
> 	 * fence, use drm_atomic_set_fence_for_plane() to ensure that
> 	 * an explicit fence is preserved.

Yeah, that's better.
> 
> > +	 *
> > +	 * Drivers should store any implicit fences in this from their
> 
> Maybe s/fences/fence/ to make it more obvious that you can only attach
> one?

Both suggestions implemented while applying, thanks very much for your
review.
-Daniel

> 
> > +	 * &drm_plane_helper.prepare_fb callback. See drm_gem_fb_prepare_fb()
> > +	 * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers.
> >  	 */
> >  	struct dma_fence *fence;
> 
> Regardless,
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>



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