On Thu, Dec 08, 2016 at 02:45:24PM +0100, Maarten Lankhorst wrote:

When DPMS was introduced to atomic, vblanks only worked when the crtc was enabled and active. wait_for_vblanks were not converted to check for crtc_state->active, which may cause an attempt for vblank_get to fail.

This is probably harmless, but convert from enable to active anyway.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

drivers/gpu/drm/drm_atomic_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 583f47f27b36..23767df72615 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1117,7 +1117,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, * vblank wait) in the ->enable boolean. */ old_crtc_state->enable = false;

• if (!crtc->state->enable)
  

• if (!crtc->state->active)
  

Hm, with this change we /should/ be able to make the drm_crtc_vblank_get call a few lines below WARN when it fails. Not doing that was only done at first because there was no accurate active state tracking for the CRTC, and since the vblank state should match active I've used that. Care for that follow-up patch?

Applied this one here meanwhile. -Daniel

	continue;

/* Legacy cursor ioctls are completely unsynced, and userspace

-- 2.7.4

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

Hi Maarten,

Thank you for the patch.

On Monday 19 Dec 2016 12:08:16 Maarten Lankhorst wrote:

Op 13-12-16 om 18:10 schreef Daniel Vetter:

...

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:

...

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

Same patch, reworded! ---8<--- 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

---

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)

Stop relying on a per crtc_state last_vblank_count, we shouldn't touch crtc_state after commit. Move it to atomic_state->crtcs.

Also stop re-using new_crtc_state->enable, we can now simply set a bitmask with crtc_crtc_mask.

Changes since v1: - Keep last_vblank_count in __drm_crtc_state. --- diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d19563651e07..88c0986d226a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1110,19 +1110,19 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state; int i, ret; + unsigned crtc_mask = 0;

- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { - /* No one cares about the old state, so abuse it for tracking - * and store whether we hold a vblank reference (and should do a - * vblank wait) in the ->enable boolean. */ - old_crtc_state->enable = false; + /* + * Legacy cursor ioctls are completely unsynced, and userspace + * relies on that (by doing tons of cursor updates). + */ + if (old_state->legacy_cursor_update) + return;

- if (!crtc->state->active) - continue; + for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + struct drm_crtc_state *new_crtc_state = crtc->state;

- /* Legacy cursor ioctls are completely unsynced, and userspace - * relies on that (by doing tons of cursor updates). */ - if (old_state->legacy_cursor_update) + if (!new_crtc_state->active) continue;

if (!drm_atomic_helper_framebuffer_changed(dev, @@ -1133,16 +1133,16 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, if (ret != 0) continue;

- old_crtc_state->enable = true; - old_crtc_state->last_vblank_count = drm_crtc_vblank_count(crtc); + crtc_mask |= drm_crtc_mask(crtc); + old_state->crtcs[i].last_vblank_count = drm_crtc_vblank_count(crtc); }

for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { - if (!old_crtc_state->enable) + if (!(crtc_mask & drm_crtc_mask(crtc))) continue;

ret = wait_event_timeout(dev->vblank[i].queue, - old_crtc_state->last_vblank_count != + old_state->crtcs[i].last_vblank_count != drm_crtc_vblank_count(crtc), msecs_to_jiffies(50));

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index d6d241f63b9f..617f095e31ba 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -145,6 +145,7 @@ struct __drm_crtcs_state { struct drm_crtc_state *state; struct drm_crtc_commit *commit; s64 __user *out_fence_ptr; + unsigned last_vblank_count; };

struct __drm_connnectors_state { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 946672f97e1e..e03d194be086 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -93,8 +93,6 @@ struct drm_plane_helper_funcs; * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders - * @last_vblank_count: for helpers and drivers to capture the vblank of the - * update to ensure framebuffer cleanup isn't done too early * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings * @mode: current mode timings * @mode_blob: &drm_property_blob for @mode @@ -140,9 +138,6 @@ struct drm_crtc_state { u32 connector_mask; u32 encoder_mask;

- /* last_vblank_count: for vblank waits before cleanup */ - u32 last_vblank_count; - /* adjusted_mode: for use by helpers and drivers */ struct drm_display_mode adjusted_mode;

On Thu, Dec 08, 2016 at 02:45:27PM +0100, Maarten Lankhorst wrote:

The current api doesn't take into account that whenever properties like rotation or z-pos change we have to wait for vblank. To make sure that we wait correctly, err on the side of caution and wait whenever a plane is added to state.

Why do we need to wait when rotation or zpos has changed? I'm a bit lost ... So rotation makes sense since often that means a special, rotated mapping (or a different offset on omap's TILER as a different example). But z-pos I have no idad why that matters. -Daniel

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

drivers/gpu/drm/drm_atomic_helper.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ccf0bed9bf4a..fdeaea84a3b7 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1123,11 +1123,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { struct drm_crtc_state *new_crtc_state = crtc->state;

• if (!new_crtc_state->active)
  

• 	continue;
  

• if (!drm_atomic_helper_framebuffer_changed(dev,
  

• 		old_state, crtc))
  

• if (!new_crtc_state->active || !new_crtc_state->planes_changed)
  continue;
  
  

  ret = drm_crtc_vblank_get(crtc);

-- 2.7.4

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 8 December 2016 at 13:45, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:

Do something similar to vc4, only allow updating the cursor state in-place through a fastpath when the watermarks are unaffected. This will allow cursor movement to be smooth, but changing cursor size or showing/hiding cursor will still fall back so watermarks can be updated.

Only moving and changing fb is allowed.

Changes since v1:

• Set page flip to always_unused for trybot.
• Copy fence correctly, ignore plane_state->state, should be NULL.
• Check crtc_state for !active and modeset, go to slowpath if the case.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++----- drivers/gpu/drm/i915/intel_display.c | 123 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 153 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index dbe9fb41ae53..60d75ce8a989 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, drm_atomic_helper_plane_destroy_state(plane, state); }

-static int intel_plane_atomic_check(struct drm_plane *plane,

•                               struct drm_plane_state *state)
  

+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,

•                                   struct intel_plane_state *intel_state)
  

{

•   struct drm_plane *plane = intel_state->base.plane;
    struct drm_i915_private *dev_priv = to_i915(plane->dev);
  

•   struct drm_crtc *crtc = state->crtc;
  

•   struct intel_crtc *intel_crtc;
  

•   struct intel_crtc_state *crtc_state;
  

•   struct drm_plane_state *state = &intel_state->base;
    struct intel_plane *intel_plane = to_intel_plane(plane);
  

•   struct intel_plane_state *intel_state = to_intel_plane_state(state);
  

•   struct drm_crtc_state *drm_crtc_state;
    int ret;
  
  

•   crtc = crtc ? crtc : plane->state->crtc;
  

•   intel_crtc = to_intel_crtc(crtc);
  

•   /*
     * Both crtc and plane->crtc could be NULL if we're updating a
     * property while the plane is disabled.  We don't actually have
     * anything driver-specific we need to test in that case, so
     * just return success.
     */
  

•   if (!crtc)
  

•   if (!intel_state->base.crtc && !plane->state->crtc)
            return 0;
  
  

•   drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
  

•   if (WARN_ON(!drm_crtc_state))
  

•           return -EINVAL;
  

•   crtc_state = to_intel_crtc_state(drm_crtc_state);
  

•   /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
    intel_state->clip.x1 = 0;
    intel_state->clip.y1 = 0;
  

@@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, return intel_plane_atomic_calc_changes(&crtc_state->base, state); }

+static int intel_plane_atomic_check(struct drm_plane *plane,

•                               struct drm_plane_state *state)
  

+{

•   struct drm_crtc *crtc = state->crtc;
  

•   struct drm_crtc_state *drm_crtc_state;
  

•   crtc = crtc ? crtc : plane->state->crtc;
  

•   /*
  

•    * Both crtc and plane->crtc could be NULL if we're updating a
  

•    * property while the plane is disabled.  We don't actually have
  

•    * anything driver-specific we need to test in that case, so
  

•    * just return success.
  

•    */
  

•   if (!crtc)
  

•           return 0;
  

•   drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
  

•   if (WARN_ON(!drm_crtc_state))
  

•           return -EINVAL;
  

•   return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
  

•                                              to_intel_plane_state(state));
  

+}

static void intel_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9eaf1e5bdae9..a8b8663ecda7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15028,6 +15029,124 @@ const struct drm_plane_funcs intel_plane_funcs = { .atomic_destroy_state = intel_plane_destroy_state, };

+static int +intel_legacy_cursor_update(struct drm_plane *plane,

•                      struct drm_crtc *crtc,
  

•                      struct drm_framebuffer *fb,
  

•                      int crtc_x, int crtc_y,
  

•                      unsigned int crtc_w, unsigned int crtc_h,
  

•                      uint32_t src_x, uint32_t src_y,
  

•                      uint32_t src_w, uint32_t src_h)
  

+{

•   struct drm_i915_private *dev_priv = to_i915(crtc->dev);
  

•   int ret;
  

•   struct drm_plane_state *old_plane_state, *new_plane_state;
  

•   struct intel_plane *intel_plane = to_intel_plane(plane);
  

•   struct drm_framebuffer *old_fb;
  

•   struct drm_crtc_state *crtc_state = crtc->state;
  

•   if (!crtc_state->active || needs_modeset(crtc_state) ||
  

•       to_intel_crtc_state(crtc_state)->update_pipe)
  

•           goto slow;
  

•   old_plane_state = plane->state;
  

•   if (old_plane_state->crtc != crtc ||
  

•       old_plane_state->src_w != src_w ||
  

•       old_plane_state->src_h != src_h ||
  

•       old_plane_state->crtc_w != crtc_w ||
  

•       old_plane_state->crtc_h != crtc_h ||
  

•       !old_plane_state->visible ||
  

•       old_plane_state->fb->modifier != fb->modifier)
  

•           goto slow;
  

•   new_plane_state = intel_plane_duplicate_state(plane);
  

•   if (!new_plane_state)
  

•           return -ENOMEM;
  

•   drm_atomic_set_fb_for_plane(new_plane_state, fb);
  

•   new_plane_state->src_x = src_x;
  

•   new_plane_state->src_y = src_y;
  

•   new_plane_state->src_w = src_w;
  

•   new_plane_state->src_h = src_h;
  

•   new_plane_state->crtc_x = crtc_x;
  

•   new_plane_state->crtc_y = crtc_y;
  

•   new_plane_state->crtc_w = crtc_w;
  

•   new_plane_state->crtc_h = crtc_h;
  

•   ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
  

•                                             to_intel_plane_state(new_plane_state));
  

•   if (ret)
  

•           return ret;
  

•   if (!new_plane_state->visible) {
  

•           intel_plane_destroy_state(plane, new_plane_state);
  

•           goto slow;
  

•   }
  

•   ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
  

•   if (ret)
  

•           return ret;
  

•   if (INTEL_INFO(dev_priv)->cursor_needs_physical) {
  

•           int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
  

•           ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
  

•           if (ret) {
  

•                   DRM_DEBUG_KMS("failed to attach phys object\n");
  

•                   return ret;
  

Drive-by-comment. Shouldn't there be some clean up in both error paths, like mutex_unlock ?

•           }
  

•   } else {
  

•           struct i915_vma *vma;
  

•           vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
  

•           if (IS_ERR(vma)) {
  

•                   DRM_DEBUG_KMS("failed to pin object\n");
  

•                   return PTR_ERR(vma);
  

•           }
  

•   }
  

•   old_fb = old_plane_state->fb;
  

•   i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
  

•                     intel_plane->frontbuffer_bit);
  

•   /* Swap plane state */
  

•   new_plane_state->fence = old_plane_state->fence;
  

•   *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
  

•   new_plane_state->fence = NULL;
  

•   new_plane_state->fb = old_fb;
  

•   intel_plane->update_plane(plane,
  

•                             to_intel_crtc_state(crtc->state),
  

•                             to_intel_plane_state(plane->state));
  

•   intel_cleanup_plane_fb(plane, new_plane_state);
  

•   mutex_unlock(&dev_priv->drm.struct_mutex);
  

•   intel_plane_destroy_state(plane, new_plane_state);
  

•   return 0;
  

+slow:

•   return drm_atomic_helper_update_plane(plane, crtc, fb,
  

•                                         crtc_x, crtc_y, crtc_w, crtc_h,
  

•                                         src_x, src_y, src_w, src_h);
  

+}

+static const struct drm_plane_funcs intel_cursor_plane_funcs = {

•   .update_plane = intel_legacy_cursor_update,
  

•   .disable_plane = drm_atomic_helper_disable_plane,
  

•   .destroy = intel_plane_destroy,
  

•   .set_property = drm_atomic_helper_plane_set_property,
  

•   .atomic_get_property = intel_plane_atomic_get_property,
  

•   .atomic_set_property = intel_plane_atomic_set_property,
  

•   .atomic_duplicate_state = intel_plane_duplicate_state,
  

•   .atomic_destroy_state = intel_plane_destroy_state,
  

+};

static struct intel_plane * intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) { @@ -15274,7 +15393,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) cursor->disable_plane = intel_disable_cursor_plane;

    ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,

•                                  0, &intel_plane_funcs,
  

•                                  0, &intel_cursor_plane_funcs,
                                   intel_cursor_formats,
                                   ARRAY_SIZE(intel_cursor_formats),
                                   DRM_PLANE_TYPE_CURSOR,
  

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8f4ddca0f521..8b6f39506d45 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1844,6 +1844,8 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,

•                                   struct intel_plane_state *intel_state);
  
  

/* intel_color.c */ void intel_color_init(struct drm_crtc *crtc); -- 2.7.4

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Hi,

On 12/12/2016 4:04 PM, Maarten Lankhorst wrote:

Do something similar to vc4, only allow updating the cursor state in-place through a fastpath when the watermarks are unaffected. This will allow cursor movement to be smooth, but changing cursor size or showing/hiding cursor will still fall back so watermarks can be updated.

Only moving and changing fb is allowed.

I was implementing something similar for msm kms. I have a comment/query below.

Changes since v1:

• Set page flip to always_unused for trybot.
• Copy fence correctly, ignore plane_state->state, should be NULL.
• Check crtc_state for !active and modeset, go to slowpath if the case.

Changes since v2:

• Make error handling work correctly. (Matthew Auld)

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 163 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index dbe9fb41ae53..60d75ce8a989 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, drm_atomic_helper_plane_destroy_state(plane, state); }

-static int intel_plane_atomic_check(struct drm_plane *plane,

• 		    struct drm_plane_state *state)
  

+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,

• 			struct intel_plane_state *intel_state)
  

{

• struct drm_plane *plane = intel_state->base.plane; struct drm_i915_private *dev_priv = to_i915(plane->dev);

• struct drm_crtc *crtc = state->crtc;
• struct intel_crtc *intel_crtc;
• struct intel_crtc_state *crtc_state;

• struct drm_plane_state *state = &intel_state->base; struct intel_plane *intel_plane = to_intel_plane(plane);

• struct intel_plane_state *intel_state = to_intel_plane_state(state);

• struct drm_crtc_state *drm_crtc_state; int ret;

• crtc = crtc ? crtc : plane->state->crtc;

• intel_crtc = to_intel_crtc(crtc);

• /*

  • Both crtc and plane->crtc could be NULL if we're updating a
  • property while the plane is disabled. We don't actually have
  • anything driver-specific we need to test in that case, so
  • just return success.

  */

• if (!crtc)

• if (!intel_state->base.crtc && !plane->state->crtc) return 0;

• drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
• if (WARN_ON(!drm_crtc_state))

• return -EINVAL;
  

• crtc_state = to_intel_crtc_state(drm_crtc_state);
• /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ intel_state->clip.x1 = 0; intel_state->clip.y1 = 0;

@@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, return intel_plane_atomic_calc_changes(&crtc_state->base, state); }

+static int intel_plane_atomic_check(struct drm_plane *plane,

• 		    struct drm_plane_state *state)
  

+{

• struct drm_crtc *crtc = state->crtc;
• struct drm_crtc_state *drm_crtc_state;
• crtc = crtc ? crtc : plane->state->crtc;
• /*

• * Both crtc and plane->crtc could be NULL if we're updating a
  

• * property while the plane is disabled.  We don't actually have
  

• * anything driver-specific we need to test in that case, so
  

• * just return success.
  

• */
  

• if (!crtc)

• return 0;
  

• drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
• if (WARN_ON(!drm_crtc_state))

• return -EINVAL;
  

• return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),

• 				   to_intel_plane_state(state));
  

+}

static void intel_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9eaf1e5bdae9..5568ecac2edc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { .atomic_destroy_state = intel_plane_destroy_state, };

+static int +intel_legacy_cursor_update(struct drm_plane *plane,

• 	   struct drm_crtc *crtc,
  

• 	   struct drm_framebuffer *fb,
  

• 	   int crtc_x, int crtc_y,
  

• 	   unsigned int crtc_w, unsigned int crtc_h,
  

• 	   uint32_t src_x, uint32_t src_y,
  

• 	   uint32_t src_w, uint32_t src_h)
  

+{

• struct drm_i915_private *dev_priv = to_i915(crtc->dev);
• int ret;
• struct drm_plane_state *old_plane_state, *new_plane_state;
• struct intel_plane *intel_plane = to_intel_plane(plane);
• struct drm_framebuffer *old_fb;
• struct drm_crtc_state *crtc_state = crtc->state;
• /*

• * When crtc is inactive or there is a modeset pending,
  

• * wait for it to complete in the slowpath
  

• */
  

• if (!crtc_state->active || needs_modeset(crtc_state) ||

•    to_intel_crtc_state(crtc_state)->update_pipe)
  

• goto slow;
  

• old_plane_state = plane->state;
• /*

• * If any parameters change that may affect watermarks,
  

• * take the slowpath. Only changing fb or position should be
  

• * in the fastpath.
  

• */
  

• if (old_plane_state->crtc != crtc ||

•    old_plane_state->src_w != src_w ||
  

•    old_plane_state->src_h != src_h ||
  

•    old_plane_state->crtc_w != crtc_w ||
  

•    old_plane_state->crtc_h != crtc_h ||
  

•    !old_plane_state->visible ||
  

•    old_plane_state->fb->modifier != fb->modifier)
  

• goto slow;
  

• new_plane_state = intel_plane_duplicate_state(plane);
• if (!new_plane_state)

• return -ENOMEM;
  

About creating a new plane state here and swapping it later, I guess it's more for the sake of intel_plane_atomic_check_with_state() to go through cleanly, right? Rather than directly modifying the old plane state?

I didn't see anything equivalent in vc4's implementation of update_plane, hence was wondering why exactly it is needed.

Also, about updating fb in the fast path, my assumption was that it was only crtc_x/y was the thing most likely to be changed really fast. Have you seen use cases where the fb changes at a faster than vsync rate?

Thanks, Archit

• drm_atomic_set_fb_for_plane(new_plane_state, fb);
• new_plane_state->src_x = src_x;
• new_plane_state->src_y = src_y;
• new_plane_state->src_w = src_w;
• new_plane_state->src_h = src_h;
• new_plane_state->crtc_x = crtc_x;
• new_plane_state->crtc_y = crtc_y;
• new_plane_state->crtc_w = crtc_w;
• new_plane_state->crtc_h = crtc_h;
• ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),

• 				  to_intel_plane_state(new_plane_state));
  

• if (ret)

• goto out_free;
  

• /* Visibility changed, must take slowpath. */
• if (!new_plane_state->visible)

• goto slow_free;
  

• ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
• if (ret)

• goto out_free;
  

• if (INTEL_INFO(dev_priv)->cursor_needs_physical) {

• int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
  

• ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to attach phys object\n");
  

• 	goto out_unlock;
  

• }
  

• } else {

• struct i915_vma *vma;
  

• vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
  

• if (IS_ERR(vma)) {
  

• 	DRM_DEBUG_KMS("failed to pin object\n");
  

• 	ret = PTR_ERR(vma);
  

• 	goto out_unlock;
  

• }
  

• }
• old_fb = old_plane_state->fb;
• i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),

• 	  intel_plane->frontbuffer_bit);
  

• /* Swap plane state */
• new_plane_state->fence = old_plane_state->fence;
• *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
• new_plane_state->fence = NULL;
• new_plane_state->fb = old_fb;
• intel_plane->update_plane(plane,

• 		  to_intel_crtc_state(crtc->state),
  

• 		  to_intel_plane_state(plane->state));
  

• intel_cleanup_plane_fb(plane, new_plane_state);

+out_unlock:

• mutex_unlock(&dev_priv->drm.struct_mutex);

+out_free:

• intel_plane_destroy_state(plane, new_plane_state);
• return ret;

+slow_free:

• intel_plane_destroy_state(plane, new_plane_state);

+slow:

• return drm_atomic_helper_update_plane(plane, crtc, fb,

• 			      crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			      src_x, src_y, src_w, src_h);
  

+}

+static const struct drm_plane_funcs intel_cursor_plane_funcs = {

• .update_plane = intel_legacy_cursor_update,
• .disable_plane = drm_atomic_helper_disable_plane,
• .destroy = intel_plane_destroy,
• .set_property = drm_atomic_helper_plane_set_property,
• .atomic_get_property = intel_plane_atomic_get_property,
• .atomic_set_property = intel_plane_atomic_set_property,
• .atomic_duplicate_state = intel_plane_duplicate_state,
• .atomic_destroy_state = intel_plane_destroy_state,

+};

static struct intel_plane * intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) { @@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) cursor->disable_plane = intel_disable_cursor_plane;

ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,

• 		       0, &intel_plane_funcs,
  

• 		       0, &intel_cursor_plane_funcs,
  	       intel_cursor_formats,
  	       ARRAY_SIZE(intel_cursor_formats),
  	       DRM_PLANE_TYPE_CURSOR,
  

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8f4ddca0f521..8b6f39506d45 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1844,6 +1844,8 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,

• 			struct intel_plane_state *intel_state);
  
  

/* intel_color.c */ void intel_color_init(struct drm_crtc *crtc);

On 12/13/2016 07:22 PM, Maarten Lankhorst wrote:

Op 13-12-16 om 14:01 schreef Archit Taneja:

...

Hi,

On 12/12/2016 4:04 PM, Maarten Lankhorst wrote:

...

Do something similar to vc4, only allow updating the cursor state in-place through a fastpath when the watermarks are unaffected. This will allow cursor movement to be smooth, but changing cursor size or showing/hiding cursor will still fall back so watermarks can be updated.

Only moving and changing fb is allowed.

I was implementing something similar for msm kms. I have a comment/query below.

...

Changes since v1:

• Set page flip to always_unused for trybot.
• Copy fence correctly, ignore plane_state->state, should be NULL.
• Check crtc_state for !active and modeset, go to slowpath if the case.

Changes since v2:

• Make error handling work correctly. (Matthew Auld)

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 163 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index dbe9fb41ae53..60d75ce8a989 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, drm_atomic_helper_plane_destroy_state(plane, state); }

-static int intel_plane_atomic_check(struct drm_plane *plane,

•                struct drm_plane_state *state)
  

+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,

•                struct intel_plane_state *intel_state)
  

{

• struct drm_plane *plane = intel_state->base.plane; struct drm_i915_private *dev_priv = to_i915(plane->dev);

• struct drm_crtc *crtc = state->crtc;
• struct intel_crtc *intel_crtc;
• struct intel_crtc_state *crtc_state;

• struct drm_plane_state *state = &intel_state->base; struct intel_plane *intel_plane = to_intel_plane(plane);

• struct intel_plane_state *intel_state = to_intel_plane_state(state);

• struct drm_crtc_state *drm_crtc_state; int ret;

• crtc = crtc ? crtc : plane->state->crtc;

• intel_crtc = to_intel_crtc(crtc);

• /* * Both crtc and plane->crtc could be NULL if we're updating a * property while the plane is disabled. We don't actually have * anything driver-specific we need to test in that case, so * just return success. */

• if (!crtc)

• if (!intel_state->base.crtc && !plane->state->crtc) return 0;

• drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
• if (WARN_ON(!drm_crtc_state))

•    return -EINVAL;
  

• crtc_state = to_intel_crtc_state(drm_crtc_state);
• /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ intel_state->clip.x1 = 0; intel_state->clip.y1 = 0;

@@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, return intel_plane_atomic_calc_changes(&crtc_state->base, state); }

+static int intel_plane_atomic_check(struct drm_plane *plane,

•                struct drm_plane_state *state)
  

+{

• struct drm_crtc *crtc = state->crtc;
• struct drm_crtc_state *drm_crtc_state;
• crtc = crtc ? crtc : plane->state->crtc;
• /*

• * Both crtc and plane->crtc could be NULL if we're updating a
  

• * property while the plane is disabled.  We don't actually have
  

• * anything driver-specific we need to test in that case, so
  

• * just return success.
  

• */
  

• if (!crtc)

•    return 0;
  

• drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
• if (WARN_ON(!drm_crtc_state))

•    return -EINVAL;
  

• return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),

•                       to_intel_plane_state(state));
  

+}

static void intel_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9eaf1e5bdae9..5568ecac2edc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { .atomic_destroy_state = intel_plane_destroy_state, };

+static int +intel_legacy_cursor_update(struct drm_plane *plane,

•           struct drm_crtc *crtc,
  

•           struct drm_framebuffer *fb,
  

•           int crtc_x, int crtc_y,
  

•           unsigned int crtc_w, unsigned int crtc_h,
  

•           uint32_t src_x, uint32_t src_y,
  

•           uint32_t src_w, uint32_t src_h)
  

+{

• struct drm_i915_private *dev_priv = to_i915(crtc->dev);
• int ret;
• struct drm_plane_state *old_plane_state, *new_plane_state;
• struct intel_plane *intel_plane = to_intel_plane(plane);
• struct drm_framebuffer *old_fb;
• struct drm_crtc_state *crtc_state = crtc->state;
• /*

• * When crtc is inactive or there is a modeset pending,
  

• * wait for it to complete in the slowpath
  

• */
  

• if (!crtc_state->active || needs_modeset(crtc_state) ||

•    to_intel_crtc_state(crtc_state)->update_pipe)
  

•    goto slow;
  

• old_plane_state = plane->state;
• /*

• * If any parameters change that may affect watermarks,
  

• * take the slowpath. Only changing fb or position should be
  

• * in the fastpath.
  

• */
  

• if (old_plane_state->crtc != crtc ||

•    old_plane_state->src_w != src_w ||
  

•    old_plane_state->src_h != src_h ||
  

•    old_plane_state->crtc_w != crtc_w ||
  

•    old_plane_state->crtc_h != crtc_h ||
  

•    !old_plane_state->visible ||
  

•    old_plane_state->fb->modifier != fb->modifier)
  

•    goto slow;
  

• new_plane_state = intel_plane_duplicate_state(plane);
• if (!new_plane_state)

•    return -ENOMEM;
  

About creating a new plane state here and swapping it later, I guess it's more for the sake of intel_plane_atomic_check_with_state() to go through cleanly, right? Rather than directly modifying the old plane state?

i915 keeps some derived state in intel_plane_state, we copy the full state to make sure the full derived state is updated as well.

...

I didn't see anything equivalent in vc4's implementation of update_plane, hence was wondering why exactly it is needed.

Also, about updating fb in the fast path, my assumption was that it was only crtc_x/y was the thing most likely to be changed really fast. Have you seen use cases where the fb changes at a faster than vsync rate?

It didn't cost much extra effort to implement fb changing too since that is handled in intel's update_plane callback too. This is why I did that as well. I'm not sure it's needed in general. Any changed fb still has to have the same size or we go to slowpath.

Okay, that make sense.

Thanks, Archit

~Maarten