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