| Message ID | 1484581498-32309-2-git-send-email-Andrey.Grodzovsky@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andrey, 2017-01-16 Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>: > 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@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) Did you build this patch? It is changing the signature of page_flip_common() but no changes to the callers. Gustavo
> -----Original Message----- > From: Gustavo Padovan [mailto:gustavo@padovan.org] > Sent: Monday, January 16, 2017 3:22 PM > To: Grodzovsky, Andrey > Cc: dri-devel@lists.freedesktop.org; nouveau@lists.freedesktop.org; amd- > gfx@lists.freedesktop.org; Deucher, Alexander; daniel.vetter@intel.com > Subject: Re: [PATCH 1/4] drm/atomic: Save flip flags in drm_plane_state > > Hi Andrey, > > 2017-01-16 Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>: > > > 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@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) > > Did you build this patch? It is changing the signature of > page_flip_common() but no changes to the callers. > > Gustavo Thanks for spotting this, I am afraid I've sent not the final version of the patch. I will resend the latest version later today. Thanks Andrey
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@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; > };
> -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] > Sent: Monday, January 16, 2017 5:18 PM > To: dri-devel@lists.freedesktop.org > Cc: Grodzovsky, Andrey; nouveau@lists.freedesktop.org; amd- > gfx@lists.freedesktop.org; Deucher, Alexander; daniel.vetter@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@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
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@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; "pflip" looks off. Better just spell it out. These flags need to be reset in duplicate_state otherwise they leak into subsequent operations. This is why I don't really like the concept of "state" containing flags and stuff that are not really part of the state but rather part of the operation of moving from the old state to the new state. But maybe we don't have a better place for this sort of stuff? I have suggested at some point that we might rename drm_atomic_state to drm_atomic_transaction or something. Stuffing the flags (or just a bool perhaps?) in there might be less confusing. > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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.
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@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> -----Original Message----- > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf > Of Daniel Vetter > Sent: Monday, January 23, 2017 3:55 AM > To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; > nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri- > devel@lists.freedesktop.org; daniel.vetter@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@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@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@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@lists.freedesktop.org; amd- > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > daniel.vetter@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@lists.freedesktop.org] On > > Behalf Of Daniel Vetter > > Sent: Monday, January 23, 2017 3:55 AM > > To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; > > nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri- > > devel@lists.freedesktop.org; daniel.vetter@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@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@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@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@lists.freedesktop.org] On Behalf > > Of Daniel Vetter > > Sent: Monday, January 23, 2017 3:55 AM > > To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; > > nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri- > > devel@lists.freedesktop.org; daniel.vetter@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@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
On Thu, Jan 26, 2017 at 05:01:01AM +0000, Grodzovsky, Andrey wrote: > > > > -----Original Message----- > > From: Cheng, Tony > > Sent: Monday, January 23, 2017 2:49 PM > > To: Daniel Vetter; Grodzovsky, Andrey > > Cc: Deucher, Alexander; nouveau@lists.freedesktop.org; amd- > > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > > daniel.vetter@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@lists.freedesktop.org] On > > > Behalf Of Daniel Vetter > > > Sent: Monday, January 23, 2017 3:55 AM > > > To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com> > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; > > > nouveau@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; dri- > > > devel@lists.freedesktop.org; daniel.vetter@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@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 ? Yes. > > 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 ... It's pretty easy to go from plane_state to crtc_state. No need to loop at all. And the implementation shouldn't justify where we put it, the important part is the semantics. -Daniel
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; };
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@amd.com> --- drivers/gpu/drm/drm_atomic_helper.c | 10 +++------- include/drm/drm_plane.h | 8 ++++++++ 2 files changed, 11 insertions(+), 7 deletions(-)