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

Hi Andrey,

Thank you for the patch.

On Monday 16 Jan 2017 10:44:55 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>
> ---
>  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;
> -
With this change all drivers using the helper will not reject that async flag, 
even if they don't implement support for async page flip. You need to either
patch them all to reject the flag, or implement async page flip support for 
all of them (preferable in the helpers, and then remove the

 * Note that for now so called async page flips (i.e. updates which are not
 * synchronized to vblank) are not supported, since the atomic interfaces have
 * no provisions for this yet.

comment).
>  	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;
>  };
-- 
Regards,

Laurent Pinchart

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart at ideasonboard.com]
> Sent: Monday, January 16, 2017 5:18 PM
> To: dri-devel at lists.freedesktop.org
> Cc: Grodzovsky, Andrey; nouveau at lists.freedesktop.org; amd-
> gfx at lists.freedesktop.org; Deucher, Alexander; daniel.vetter at
intel.com
> Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state
> 
> Hi Andrey,
> 
> Thank you for the patch.
> 
> On Monday 16 Jan 2017 10:44:55 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>
> > ---
> >  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;
> > -
> 
> With this change all drivers using the helper will not reject that async
flag,
> even if they don't implement support for async page flip. You need to
either
> patch them all to reject the flag, or implement async page flip support for
all
> of them (preferable in the helpers, and then remove the
Please check drm_mode_page_flip_ioctl, one of the checks in the beginning is

if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) &&
!dev->mode_config.async_page_flip)
		return -EINVAL;

We in DC explicitly set dev->mode_config.async_page_flip = true, any driver
which is
Not supporting ASYNC flip will fail the IOCTL at this point.
Same in drm_mode_atomic_ioctl
> 
>  * Note that for now so called async page flips (i.e. updates which are not
>  * synchronized to vblank) are not supported, since the atomic interfaces
> have
>  * no provisions for this yet.
> 
> comment).
Thanks, that a good catch, will remove.

Andrey
> 
> >  	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;
> >  };
> 
> --
> Regards,
> 
> Laurent Pinchart

Hi Andrey,

On Tuesday 17 Jan 2017 04:03:11 Grodzovsky, Andrey
wrote:
> On Monday, January 16, 2017 5:18 PM Laurent Pinchart wrote:
> > On Monday 16 Jan 2017 10:44:55 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>
> >> ---
> >> 
> >>  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;
> >> -
> > 
> > With this change all drivers using the helper will not reject that
async
> > flag, even if they don't implement support for async page flip.
You need
> > to either patch them all to reject the flag, or implement async page
flip
> > support for all of them (preferable in the helpers, and then remove
the
> 
> Please check drm_mode_page_flip_ioctl, one of the checks in the beginning
is
> 
> if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) &&
> !dev->mode_config.async_page_flip) return -EINVAL;
I think you're right. Sorry for the noise.
> We in DC explicitly set dev->mode_config.async_page_flip = true, any
driver
> which is Not supporting ASYNC flip will fail the IOCTL at this point.
> Same in drm_mode_atomic_ioctl
> 
> >  * Note that for now so called async page flips (i.e. updates which
are
> >  not
> >  * synchronized to vblank) are not supported, since the atomic
interfaces
> > have
> >  * no provisions for this yet.
> > 
> > comment).
> 
> Thanks, that a good catch, will remove.
-- 
Regards,

Laurent Pinchart