| Message ID | 1481204729-9058-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote: > Atomic drivers may set properties like rotation on the same fb, which > may require a call to prepare_fb even when framebuffer stays identical. > > Instead of handling all the special cases in the core, let the driver > decide when prepare_fb and cleanup_fb are noops. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> I think this makes sense, but would be really good to get a pile of acks from driver maintainers on this one. Rob, Eric, Laurent, others? -Daniel > --- > drivers/gpu/drm/drm_atomic_helper.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 23767df72615..d19563651e07 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, > > funcs = plane->helper_private; > > - if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc)) > - continue; > - > if (funcs->prepare_fb) { > ret = funcs->prepare_fb(plane, plane_state); > if (ret) > @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, > if (j >= i) > continue; > > - if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc)) > - continue; > - > funcs = plane->helper_private; > > if (funcs->cleanup_fb) > @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev, > for_each_plane_in_state(old_state, plane, plane_state, i) { > const struct drm_plane_helper_funcs *funcs; > > - if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc)) > - continue; > - > funcs = plane->helper_private; > > if (funcs->cleanup_fb) > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Daniel, On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote: > On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote: > > Atomic drivers may set properties like rotation on the same fb, which > > may require a call to prepare_fb even when framebuffer stays identical. > > > > Instead of handling all the special cases in the core, let the driver > > decide when prepare_fb and cleanup_fb are noops. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > I think this makes sense, but would be really good to get a pile of acks > from driver maintainers on this one. Rob, Eric, Laurent, others? This is all very nice, but it will introduce at least a performance regression, and possibly worse, until drivers get updated. There are 7 drivers implementing the .prepare_fb() callback (plus a bunch of drivers that probably should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this patch before they get fixed. > > --- > > > > drivers/gpu/drm/drm_atomic_helper.c | 9 --------- > > 1 file changed, 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..d19563651e07 > > 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct > > drm_device *dev,> > > funcs = plane->helper_private; > > > > - if (!drm_atomic_helper_framebuffer_changed(dev, state, > > plane_state->crtc)) - continue; > > - > > > > if (funcs->prepare_fb) { > > > > ret = funcs->prepare_fb(plane, plane_state); > > if (ret) > > > > @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct > > drm_device *dev,> > > if (j >= i) > > > > continue; > > > > - if (!drm_atomic_helper_framebuffer_changed(dev, state, > > plane_state->crtc)) - continue; > > - > > > > funcs = plane->helper_private; > > > > if (funcs->cleanup_fb) > > > > @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct > > drm_device *dev,> > > for_each_plane_in_state(old_state, plane, plane_state, i) { > > > > const struct drm_plane_helper_funcs *funcs; > > > > - if (!drm_atomic_helper_framebuffer_changed(dev, old_state, > > plane_state->crtc)) - continue; > > - > > > > funcs = plane->helper_private; > > > > if (funcs->cleanup_fb)
On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote: > Hi Daniel, > > On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote: > > On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote: > > > Atomic drivers may set properties like rotation on the same fb, which > > > may require a call to prepare_fb even when framebuffer stays identical. > > > > > > Instead of handling all the special cases in the core, let the driver > > > decide when prepare_fb and cleanup_fb are noops. > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > I think this makes sense, but would be really good to get a pile of acks > > from driver maintainers on this one. Rob, Eric, Laurent, others? > > This is all very nice, but it will introduce at least a performance > regression, and possibly worse, until drivers get updated. There are 7 drivers > implementing the .prepare_fb() callback (plus a bunch of drivers that probably > should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this > patch before they get fixed. Maarten's commit message is insufficient, since this is defacto a revert of commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba Author: Keith Packard <keithp@keithp.com> Date: Sat Jun 4 01:16:22 2016 -0700 drm: Don't prepare or cleanup unchanging frame buffers [v3] because that breaks stuff. We're simply going back to where we've been a few months ago. Since this is a regression fix, back to original behaviour, can you ack (assuming Maarten updates the commit message to reflect the nature of the commit here)? -Daniel > > > > --- > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 9 --------- > > > 1 file changed, 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..d19563651e07 > > > 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct > > > drm_device *dev,> > > > funcs = plane->helper_private; > > > > > > - if (!drm_atomic_helper_framebuffer_changed(dev, state, > > > plane_state->crtc)) - continue; > > > - > > > > > > if (funcs->prepare_fb) { > > > > > > ret = funcs->prepare_fb(plane, plane_state); > > > if (ret) > > > > > > @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct > > > drm_device *dev,> > > > if (j >= i) > > > > > > continue; > > > > > > - if (!drm_atomic_helper_framebuffer_changed(dev, state, > > > plane_state->crtc)) - continue; > > > - > > > > > > funcs = plane->helper_private; > > > > > > if (funcs->cleanup_fb) > > > > > > @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct > > > drm_device *dev,> > > > for_each_plane_in_state(old_state, plane, plane_state, i) { > > > > > > const struct drm_plane_helper_funcs *funcs; > > > > > > - if (!drm_atomic_helper_framebuffer_changed(dev, old_state, > > > plane_state->crtc)) - continue; > > > - > > > > > > funcs = plane->helper_private; > > > > > > if (funcs->cleanup_fb) > > -- > Regards, > > Laurent Pinchart >
Op 09-12-16 om 09:25 schreef Daniel Vetter: > On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote: >> Hi Daniel, >> >> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote: >>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote: >>>> Atomic drivers may set properties like rotation on the same fb, which >>>> may require a call to prepare_fb even when framebuffer stays identical. >>>> >>>> Instead of handling all the special cases in the core, let the driver >>>> decide when prepare_fb and cleanup_fb are noops. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> I think this makes sense, but would be really good to get a pile of acks >>> from driver maintainers on this one. Rob, Eric, Laurent, others? >> This is all very nice, but it will introduce at least a performance >> regression, and possibly worse, until drivers get updated. There are 7 drivers >> implementing the .prepare_fb() callback (plus a bunch of drivers that probably >> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this >> patch before they get fixed. > Maarten's commit message is insufficient, since this is defacto a revert > of > > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba > Author: Keith Packard <keithp@keithp.com> > Date: Sat Jun 4 01:16:22 2016 -0700 > > drm: Don't prepare or cleanup unchanging frame buffers [v3] > > because that breaks stuff. We're simply going back to where we've been a > few months ago. Since this is a regression fix, back to original > behaviour, can you ack (assuming Maarten updates the commit message to > reflect the nature of the commit here)? Waiting on a reply, but what about this commit message for this patch? --- Atomic drivers may set properties like rotation on the same fb, which may require a call to prepare_fb even when framebuffer stays identical. Instead of handling all the special cases in the core, let the driver decide when prepare_fb and cleanup_fb are noops. This is a revert of: commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba Author: Keith Packard <keithp@keithp.com> Date: Sat Jun 4 01:16:22 2016 -0700 drm: Don't prepare or cleanup unchanging frame buffers [v3] The original commit mentions that this prevents waiting in i915 on all previous rendering during cursor updates, but there are better ways to fix this. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
On Tue, Dec 13, 2016 at 03:13:54PM +0100, Maarten Lankhorst wrote: > Op 09-12-16 om 09:25 schreef Daniel Vetter: > > On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote: > >> Hi Daniel, > >> > >> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote: > >>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote: > >>>> Atomic drivers may set properties like rotation on the same fb, which > >>>> may require a call to prepare_fb even when framebuffer stays identical. > >>>> > >>>> Instead of handling all the special cases in the core, let the driver > >>>> decide when prepare_fb and cleanup_fb are noops. > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> I think this makes sense, but would be really good to get a pile of acks > >>> from driver maintainers on this one. Rob, Eric, Laurent, others? > >> This is all very nice, but it will introduce at least a performance > >> regression, and possibly worse, until drivers get updated. There are 7 drivers > >> implementing the .prepare_fb() callback (plus a bunch of drivers that probably > >> should use drm_fb_cma_prepare_fb() but don't at the moment). I can't ack this > >> patch before they get fixed. > > Maarten's commit message is insufficient, since this is defacto a revert > > of > > > > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba > > Author: Keith Packard <keithp@keithp.com> > > Date: Sat Jun 4 01:16:22 2016 -0700 > > > > drm: Don't prepare or cleanup unchanging frame buffers [v3] > > > > because that breaks stuff. We're simply going back to where we've been a > > few months ago. Since this is a regression fix, back to original > > behaviour, can you ack (assuming Maarten updates the commit message to > > reflect the nature of the commit here)? > > Waiting on a reply, but what about this commit message for this patch? > --- > Atomic drivers may set properties like rotation on the same fb, which > may require a call to prepare_fb even when framebuffer stays identical. > > Instead of handling all the special cases in the core, let the driver > decide when prepare_fb and cleanup_fb are noops. > > This is a revert of: > > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba > Author: Keith Packard <keithp@keithp.com> > Date: Sat Jun 4 01:16:22 2016 -0700 > > drm: Don't prepare or cleanup unchanging frame buffers [v3] > > The original commit mentions that this prevents waiting in i915 on all > previous rendering during cursor updates, but there are better ways to > fix this. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Yeah sounds good to me. Since we don't want to backport all the i915 cursor patches no cc: stable on this. Also, this is only an issue for drivers which both have a cursor plane, and implement that cursor using universal planes (i.e. settting drm_crtc->cursor). Afaik the only two are vc4 and i915, and after this series both will have appropriate hacks (for now) to keep existing userspace happy. -Daniel
Hi Maarten, On Tuesday 13 Dec 2016 15:13:54 Maarten Lankhorst wrote: > Op 09-12-16 om 09:25 schreef Daniel Vetter: > > On Fri, Dec 09, 2016 at 12:42:19AM +0200, Laurent Pinchart wrote: > >> On Thursday 08 Dec 2016 16:41:04 Daniel Vetter wrote: > >>> On Thu, Dec 08, 2016 at 02:45:25PM +0100, Maarten Lankhorst wrote: > >>>> Atomic drivers may set properties like rotation on the same fb, which > >>>> may require a call to prepare_fb even when framebuffer stays identical. > >>>> > >>>> Instead of handling all the special cases in the core, let the driver > >>>> decide when prepare_fb and cleanup_fb are noops. > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> > >>> I think this makes sense, but would be really good to get a pile of acks > >>> from driver maintainers on this one. Rob, Eric, Laurent, others? > >> > >> This is all very nice, but it will introduce at least a performance > >> regression, and possibly worse, until drivers get updated. There are 7 > >> drivers implementing the .prepare_fb() callback (plus a bunch of drivers > >> that probably should use drm_fb_cma_prepare_fb() but don't at the > >> moment). I can't ack this patch before they get fixed. > > > > Maarten's commit message is insufficient, since this is defacto a revert > > of > > > > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba > > Author: Keith Packard <keithp@keithp.com> > > Date: Sat Jun 4 01:16:22 2016 -0700 > > > > drm: Don't prepare or cleanup unchanging frame buffers [v3] > > > > because that breaks stuff. We're simply going back to where we've been a > > few months ago. Since this is a regression fix, back to original > > behaviour, can you ack (assuming Maarten updates the commit message to > > reflect the nature of the commit here)? > > Waiting on a reply, but what about this commit message for this patch? > --- > Atomic drivers may set properties like rotation on the same fb, which > may require a call to prepare_fb even when framebuffer stays identical. > > Instead of handling all the special cases in the core, let the driver > decide when prepare_fb and cleanup_fb are noops. > > This is a revert of: > > commit fcc60b413d14dd06ddbd79ec50e83c4fb2a097ba > Author: Keith Packard <keithp@keithp.com> > Date: Sat Jun 4 01:16:22 2016 -0700 > > drm: Don't prepare or cleanup unchanging frame buffers [v3] > > The original commit mentions that this prevents waiting in i915 on all > previous rendering during cursor updates, but there are better ways to > fix this. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 23767df72615..d19563651e07 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1664,9 +1664,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, funcs = plane->helper_private; - if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc)) - continue; - if (funcs->prepare_fb) { ret = funcs->prepare_fb(plane, plane_state); if (ret) @@ -1683,9 +1680,6 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, if (j >= i) continue; - if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc)) - continue; - funcs = plane->helper_private; if (funcs->cleanup_fb) @@ -1952,9 +1946,6 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev, for_each_plane_in_state(old_state, plane, plane_state, i) { const struct drm_plane_helper_funcs *funcs; - if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc)) - continue; - funcs = plane->helper_private; if (funcs->cleanup_fb)
Atomic drivers may set properties like rotation on the same fb, which may require a call to prepare_fb even when framebuffer stays identical. Instead of handling all the special cases in the core, let the driver decide when prepare_fb and cleanup_fb are noops. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 9 --------- 1 file changed, 9 deletions(-)