Re: [v3 PATCH 1/3] drm/atomic: Save flip flags in drm_crtct_state

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Andrey,

(by the way there's a typo in the subject)

On Monday 30 Jan 2017 19:42:23 Grodzovsky, Andrey wrote:
> On Monday, January 30, 2017 6:05 AM Laurent Pinchart wrote:
> > On Saturday 28 Jan 2017 21:26:49 Andrey Grodzovsky wrote:
> >> Allows using atomic flip helpers for drivers using ASYNC flip.
> >> Remove ASYNC_FLIP restriction in helpers and caches the page flip
> >> flags in drm_crtc_state to be used in the low level drivers.
> >> 
> >> v2:
> >> Resending the patch since the original was broken.
> >> 
> >> v3:
> >> Save flag in crtc_state instead of plane_state
> >> 
> >> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_atomic_helper.c | 19 +++++--------------
> >>  include/drm/drm_crtc.h              |  8 +++++++-
> >>  include/drm/drm_plane.h             |  1 +
> >>  3 files changed, 13 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> b/drivers/gpu/drm/drm_atomic_helper.c index a4e5477..28065ee 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c

[snip]

> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> >> 5c77c3f..76457a4 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -162,10 +162,16 @@ struct drm_crtc_state {
> >>  	 * Target vertical blank period when a page flip
> >>  	 * should take effect.
> >>  	 */
> >>  	u32 target_vblank;
> >>  	
> >>  	/**
> >> +	 * @pflip_flags:
> >> +	 *
> >> +	 * Flip related config options
> > 
> > This isn't detailed enough. I propose something along the lines of
> > 
> > "DRM_MODE_PAGE_FLIP_* page flip flags, as passed to the page flip ioctl.
> > Always zero for atomic commits that don't originate from a page flip
> > ioctl."
> > 
> > You will then also need to reset the field to 0 at an appropriate point,
> > as it's more an atomic commit transaction information than a state. Apart
> > from that this patch looks good to me.
> 
> Thanks for your comments, i am not sure I understand why the reset is
> needed, for any future commit on same crtc the new state will have the
> field empty

It won't, __drm_atomic_helper_crtc_duplicate_state() memcpy's the state, so 
the page flip flags will be copied to the new state until another legacy page 
flip overwrites them.

> and if it's a flip IOCTL the field will be filled as needed in
> page_flip_common, otherwise it will stay empty. If the last commit on that
> crtc was a flip then why not keep this field with the bits that the user
> mode set ?

The page flip flags are not state information. They describe an operation, not 
a state. They're needed when performing the operation, but if we don't reset 
them when it completes (at the latest when duplicating the state for the next 
atomic commit, but possibly earlier) there's a risk that a driver will 
mistakenly perform an async page flip during a later operation.

> >> +	 */
> >> +	u32 pflip_flags;

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau