Nouveau - Jan 2017 - [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state

On Mon, Jan 16, 2017 at 10:44:55AM -0500, 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_plane_state
> to be used in the low level drivers.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
It's mostly guesswork, but I think we should have the flip flags in the
crtc, not in each plane. Similar to how we move the event from planes to
crtc.
-Daniel
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
>  include/drm/drm_plane.h             |  8 ++++++++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
> index a4e5477..f83dc43 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2737,7 +2737,8 @@ static int page_flip_common(
>  				struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event)
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_plane_state *plane_state;
> @@ -2754,6 +2755,7 @@ static int page_flip_common(
>  	if (IS_ERR(plane_state))
>  		return PTR_ERR(plane_state);
>  
> +	plane_state->pflip_flags = flags;
>  
>  	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
>  	if (ret != 0)
> @@ -2800,9 +2802,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
*crtc,
>  	struct drm_atomic_state *state;
>  	int ret = 0;
>  
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -2865,9 +2864,6 @@ int drm_atomic_helper_page_flip_target(
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
> -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> -		return -EINVAL;
> -
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
>  		return -ENOMEM;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index db3bbde..86d8ffc 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -122,6 +122,14 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
>  
> +
> +	/**
> +	 * @pflip_flags:
> +	 *
> +	 * Flip related config options
> +	 */
> +	u32 pflip_flags;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On
Behalf
> Of Daniel Vetter
> Sent: Monday, January 23, 2017 3:55 AM
> To: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>;
> nouveau at lists.freedesktop.org; amd-gfx at lists.freedesktop.org; dri-
> devel at lists.freedesktop.org; daniel.vetter at intel.com
> Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> 
> On Mon, Jan 16, 2017 at 10:44:55AM -0500, 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_plane_state to be used in the low level drivers.
> >
> > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> 
> It's mostly guesswork, but I think we should have the flip flags in the
crtc, not
> in each plane. Similar to how we move the event from planes to crtc.
> -Daniel
What does ASYNC flip mean?  HW flip as soon as possible and result in tearing on
screen?  If so I could imaging some use case where you have some UI control/menu
overlay on top, and some game running on a underlay plane, and the game want to
be able to flip as soon as possible.  Or Daniel do you think ASYNC property will
apply to all planes in CRTC?
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
> >  include/drm/drm_plane.h             |  8 ++++++++
> >  2 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index a4e5477..f83dc43 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2737,7 +2737,8 @@ static int page_flip_common(
> >  				struct drm_atomic_state *state,
> >  				struct drm_crtc *crtc,
> >  				struct drm_framebuffer *fb,
> > -				struct drm_pending_vblank_event *event)
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags)
> >  {
> >  	struct drm_plane *plane = crtc->primary;
> >  	struct drm_plane_state *plane_state; @@ -2754,6 +2755,7 @@ static
> > int page_flip_common(
> >  	if (IS_ERR(plane_state))
> >  		return PTR_ERR(plane_state);
> >
> > +	plane_state->pflip_flags = flags;
> >
> >  	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> >  	if (ret != 0)
> > @@ -2800,9 +2802,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc
> *crtc,
> >  	struct drm_atomic_state *state;
> >  	int ret = 0;
> >
> > -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> > -		return -EINVAL;
> > -
> >  	state = drm_atomic_state_alloc(plane->dev);
> >  	if (!state)
> >  		return -ENOMEM;
> > @@ -2865,9 +2864,6 @@ int drm_atomic_helper_page_flip_target(
> >  	struct drm_crtc_state *crtc_state;
> >  	int ret = 0;
> >
> > -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> > -		return -EINVAL;
> > -
> >  	state = drm_atomic_state_alloc(plane->dev);
> >  	if (!state)
> >  		return -ENOMEM;
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
> > db3bbde..86d8ffc 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -122,6 +122,14 @@ struct drm_plane_state {
> >  	 */
> >  	bool visible;
> >
> > +
> > +	/**
> > +	 * @pflip_flags:
> > +	 *
> > +	 * Flip related config options
> > +	 */
> > +	u32 pflip_flags;
> > +
> >  	struct drm_atomic_state *state;
> >  };
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

> -----Original Message-----
> From: Cheng, Tony
> Sent: Monday, January 23, 2017 2:49 PM
> To: Daniel Vetter; Grodzovsky, Andrey
> Cc: Deucher, Alexander; nouveau at lists.freedesktop.org; amd-
> gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> daniel.vetter at intel.com; dc_upstream
> Subject: RE: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> 
> 
> 
> > -----Original Message-----
> > From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On
> > Behalf Of Daniel Vetter
> > Sent: Monday, January 23, 2017 3:55 AM
> > To: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>
> > Cc: Deucher, Alexander <Alexander.Deucher at amd.com>;
> > nouveau at lists.freedesktop.org; amd-gfx at lists.freedesktop.org;
dri-
> > devel at lists.freedesktop.org; daniel.vetter at intel.com
> > Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in
> > drm_plane_state
> >
> > On Mon, Jan 16, 2017 at 10:44:55AM -0500, 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_plane_state to be used in the low level drivers.
> > >
> > > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at
amd.com>
> >
> > It's mostly guesswork, but I think we should have the flip flags
in
> > the crtc, not in each plane. Similar to how we move the event from
planes
> to crtc.
> > -Daniel
> 
Do you mean here crtc or crtc_state ?
> What does ASYNC flip mean?  HW flip as soon as possible and result in
tearing
> on screen?  If so I could imaging some use case where you have some UI
> control/menu overlay on top, and some game running on a underlay plane,
> and the game want to be able to flip as soon as possible.  Or Daniel do you
> think ASYNC property will apply to all planes in CRTC?
> 
Also the client code both in nouveau and AMD/DAL iterates over new planes and
their new states
so it seems to me easier to retrieve the parameter from the plane_state and not
adding loop
to find matching crtc or its  state, especially if it's before
drm_atomic_helper_swap_state is called,
 when crtc->state still points to the old state ...
[Grodzovsky, Andrey] 
> >
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 10 +++-------
> > >  include/drm/drm_plane.h             |  8 ++++++++
> > >  2 files changed, 11 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index a4e5477..f83dc43 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -2737,7 +2737,8 @@ static int page_flip_common(
> > >  				struct drm_atomic_state *state,
> > >  				struct drm_crtc *crtc,
> > >  				struct drm_framebuffer *fb,
> > > -				struct drm_pending_vblank_event *event)
> > > +				struct drm_pending_vblank_event *event,
> > > +				uint32_t flags)
> > >  {
> > >  	struct drm_plane *plane = crtc->primary;
> > >  	struct drm_plane_state *plane_state; @@ -2754,6 +2755,7 @@
static
> > > int page_flip_common(
> > >  	if (IS_ERR(plane_state))
> > >  		return PTR_ERR(plane_state);
> > >
> > > +	plane_state->pflip_flags = flags;
> > >
> > >  	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> > >  	if (ret != 0)
> > > @@ -2800,9 +2802,6 @@ int drm_atomic_helper_page_flip(struct
> > > drm_crtc
> > *crtc,
> > >  	struct drm_atomic_state *state;
> > >  	int ret = 0;
> > >
> > > -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> > > -		return -EINVAL;
> > > -
> > >  	state = drm_atomic_state_alloc(plane->dev);
> > >  	if (!state)
> > >  		return -ENOMEM;
> > > @@ -2865,9 +2864,6 @@ int drm_atomic_helper_page_flip_target(
> > >  	struct drm_crtc_state *crtc_state;
> > >  	int ret = 0;
> > >
> > > -	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> > > -		return -EINVAL;
> > > -
> > >  	state = drm_atomic_state_alloc(plane->dev);
> > >  	if (!state)
> > >  		return -ENOMEM;
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index
> > > db3bbde..86d8ffc 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -122,6 +122,14 @@ struct drm_plane_state {
> > >  	 */
> > >  	bool visible;
> > >
> > > +
> > > +	/**
> > > +	 * @pflip_flags:
> > > +	 *
> > > +	 * Flip related config options
> > > +	 */
> > > +	u32 pflip_flags;
> > > +
> > >  	struct drm_atomic_state *state;
> > >  };
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Jan 23, 2017 at 07:48:54PM +0000, Cheng, Tony
wrote:
> 
> 
> > -----Original Message-----
> > From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On
Behalf
> > Of Daniel Vetter
> > Sent: Monday, January 23, 2017 3:55 AM
> > To: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>
> > Cc: Deucher, Alexander <Alexander.Deucher at amd.com>;
> > nouveau at lists.freedesktop.org; amd-gfx at lists.freedesktop.org;
dri-
> > devel at lists.freedesktop.org; daniel.vetter at intel.com
> > Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in
drm_plane_state
> > 
> > On Mon, Jan 16, 2017 at 10:44:55AM -0500, 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_plane_state to be used in the low level drivers.
> > >
> > > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at
amd.com>
> > 
> > It's mostly guesswork, but I think we should have the flip flags
in the crtc, not
> > in each plane. Similar to how we move the event from planes to crtc.
> > -Daniel
> 
> What does ASYNC flip mean?  HW flip as soon as possible and result in
> tearing on screen?  If so I could imaging some use case where you have
> some UI control/menu overlay on top, and some game running on a underlay
> plane, and the game want to be able to flip as soon as possible.  Or
> Daniel do you think ASYNC property will apply to all planes in CRTC?
Those kind of questions are exactly why I think we should wait with
exposing async through the atomic ioctl until someone needs it. And yes
async means "as fast as possible, with tearing".
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch