* [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates
2014-10-24 13:51 [PATCH v4 1/5] drm/i915: create a prepare step for primary planes updates Gustavo Padovan
@ 2014-10-24 13:51 ` Gustavo Padovan
2014-10-30 13:32 ` Paulo Zanoni
2014-10-24 13:51 ` [PATCH v4 3/5] drm/i915: use intel_fb_obj() macros to assign gem objects Gustavo Padovan
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Gustavo Padovan @ 2014-10-24 13:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Gustavo Padovan, dri-devel
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
take out pin_fb code so the commit phase can't fail anymore.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
1 file changed, 40 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 2c060ad..3631b0e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
}
static int
-intel_commit_sprite_plane(struct drm_plane *plane,
- struct intel_plane_state *state)
+intel_prepare_sprite_plane(struct drm_plane *plane,
+ struct intel_plane_state *state)
{
struct drm_device *dev = plane->dev;
struct drm_crtc *crtc = state->crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct intel_plane *intel_plane = to_intel_plane(plane);
enum pipe pipe = intel_crtc->pipe;
struct drm_framebuffer *fb = state->fb;
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
- struct drm_i915_gem_object *old_obj = intel_plane->obj;
- int crtc_x, crtc_y;
- unsigned int crtc_w, crtc_h;
- uint32_t src_x, src_y, src_w, src_h;
- struct drm_rect *dst = &state->dst;
- const struct drm_rect *clip = &state->clip;
- bool primary_enabled;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+ struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
int ret;
- /*
- * If the sprite is completely covering the primary plane,
- * we can disable the primary and save power.
- */
- primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
- WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
-
-
if (old_obj != obj) {
mutex_lock(&dev->struct_mutex);
@@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
return ret;
}
+ return 0;
+}
+
+static void
+intel_commit_sprite_plane(struct drm_plane *plane,
+ struct intel_plane_state *state)
+{
+ struct drm_device *dev = plane->dev;
+ struct drm_crtc *crtc = state->crtc;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_plane *intel_plane = to_intel_plane(plane);
+ enum pipe pipe = intel_crtc->pipe;
+ struct drm_framebuffer *fb = state->fb;
+ struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+ struct drm_i915_gem_object *obj = intel_fb->obj;
+ struct drm_i915_gem_object *old_obj = intel_plane->obj;
+ int crtc_x, crtc_y;
+ unsigned int crtc_w, crtc_h;
+ uint32_t src_x, src_y, src_w, src_h;
+ struct drm_rect *dst = &state->dst;
+ const struct drm_rect *clip = &state->clip;
+ bool primary_enabled;
+
+ /*
+ * If the sprite is completely covering the primary plane,
+ * we can disable the primary and save power.
+ */
+ primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
+ WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
+
intel_plane->crtc_x = state->orig_dst.x1;
intel_plane->crtc_y = state->orig_dst.y1;
intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
@@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
intel_unpin_fb_obj(old_obj);
mutex_unlock(&dev->struct_mutex);
}
-
- return 0;
}
static int
@@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
if (ret)
return ret;
- return intel_commit_sprite_plane(plane, &state);
+ ret = intel_prepare_sprite_plane(plane, &state);
+ if (ret)
+ return ret;
+
+ intel_commit_sprite_plane(plane, &state);
+ return 0;
}
static int
--
1.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates
2014-10-24 13:51 ` [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates Gustavo Padovan
@ 2014-10-30 13:32 ` Paulo Zanoni
2014-10-30 14:08 ` [Intel-gfx] " Imre Deak
2014-10-30 14:34 ` Ville Syrjälä
0 siblings, 2 replies; 13+ messages in thread
From: Paulo Zanoni @ 2014-10-30 13:32 UTC (permalink / raw)
To: Gustavo Padovan
Cc: Intel Graphics Development, Gustavo Padovan, DRI Development
2014-10-24 11:51 GMT-02:00 Gustavo Padovan <gustavo@padovan.org>:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> take out pin_fb code so the commit phase can't fail anymore.
>
According to my bisection results, this is the first bad commit of
https://bugs.freedesktop.org/show_bug.cgi?id=85634.
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
> 1 file changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 2c060ad..3631b0e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
> }
>
> static int
> -intel_commit_sprite_plane(struct drm_plane *plane,
> - struct intel_plane_state *state)
> +intel_prepare_sprite_plane(struct drm_plane *plane,
> + struct intel_plane_state *state)
> {
> struct drm_device *dev = plane->dev;
> struct drm_crtc *crtc = state->crtc;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_plane *intel_plane = to_intel_plane(plane);
> enum pipe pipe = intel_crtc->pipe;
> struct drm_framebuffer *fb = state->fb;
> - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> - struct drm_i915_gem_object *obj = intel_fb->obj;
> - struct drm_i915_gem_object *old_obj = intel_plane->obj;
> - int crtc_x, crtc_y;
> - unsigned int crtc_w, crtc_h;
> - uint32_t src_x, src_y, src_w, src_h;
> - struct drm_rect *dst = &state->dst;
> - const struct drm_rect *clip = &state->clip;
> - bool primary_enabled;
> + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> int ret;
>
> - /*
> - * If the sprite is completely covering the primary plane,
> - * we can disable the primary and save power.
> - */
> - primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> - WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> -
> -
> if (old_obj != obj) {
> mutex_lock(&dev->struct_mutex);
>
> @@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> return ret;
> }
>
> + return 0;
> +}
> +
> +static void
> +intel_commit_sprite_plane(struct drm_plane *plane,
> + struct intel_plane_state *state)
> +{
> + struct drm_device *dev = plane->dev;
> + struct drm_crtc *crtc = state->crtc;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_plane *intel_plane = to_intel_plane(plane);
> + enum pipe pipe = intel_crtc->pipe;
> + struct drm_framebuffer *fb = state->fb;
> + struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> + struct drm_i915_gem_object *obj = intel_fb->obj;
> + struct drm_i915_gem_object *old_obj = intel_plane->obj;
> + int crtc_x, crtc_y;
> + unsigned int crtc_w, crtc_h;
> + uint32_t src_x, src_y, src_w, src_h;
> + struct drm_rect *dst = &state->dst;
> + const struct drm_rect *clip = &state->clip;
> + bool primary_enabled;
> +
> + /*
> + * If the sprite is completely covering the primary plane,
> + * we can disable the primary and save power.
> + */
> + primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> + WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> +
> intel_plane->crtc_x = state->orig_dst.x1;
> intel_plane->crtc_y = state->orig_dst.y1;
> intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> @@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> intel_unpin_fb_obj(old_obj);
> mutex_unlock(&dev->struct_mutex);
> }
> -
> - return 0;
> }
>
> static int
> @@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> if (ret)
> return ret;
>
> - return intel_commit_sprite_plane(plane, &state);
> + ret = intel_prepare_sprite_plane(plane, &state);
> + if (ret)
> + return ret;
> +
> + intel_commit_sprite_plane(plane, &state);
> + return 0;
> }
>
> static int
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates
2014-10-30 13:32 ` Paulo Zanoni
@ 2014-10-30 14:08 ` Imre Deak
2014-10-30 14:34 ` Ville Syrjälä
1 sibling, 0 replies; 13+ messages in thread
From: Imre Deak @ 2014-10-30 14:08 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Gustavo Padovan, DRI Development
On Thu, 2014-10-30 at 11:32 -0200, Paulo Zanoni wrote:
> 2014-10-24 11:51 GMT-02:00 Gustavo Padovan <gustavo@padovan.org>:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > take out pin_fb code so the commit phase can't fail anymore.
> >
>
> According to my bisection results, this is the first bad commit of
> https://bugs.freedesktop.org/show_bug.cgi?id=85634.
I saw the same issue, when checking #82939. AFAICS when disabling the
crtc intel_plane->obj will be set to NULL and the object will be
unpinned, but plane->fb remains set. Next if drm_set_plane with the same
fb is called intel_prepare_sprite_plane() won't pin the object since it
sees that same fb is still set, but sets intel_plane->obj. Next if
drm_set_plane called with a NULL fb, it will try unpin the object, and
the unbalanced refcount throws a WARN. The following got rid of the
problem for me:
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 8b80d68..36762b5 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1198,9 +1198,10 @@ intel_prepare_sprite_plane(struct drm_plane
*plane,
struct drm_crtc *crtc = state->crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
enum pipe pipe = intel_crtc->pipe;
+ struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_framebuffer *fb = state->fb;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
- struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+ struct drm_i915_gem_object *old_obj = intel_plane->obj;
int ret;
if (old_obj != obj) {
>
>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
> > 1 file changed, 40 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 2c060ad..3631b0e 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
> > }
> >
> > static int
> > -intel_commit_sprite_plane(struct drm_plane *plane,
> > - struct intel_plane_state *state)
> > +intel_prepare_sprite_plane(struct drm_plane *plane,
> > + struct intel_plane_state *state)
> > {
> > struct drm_device *dev = plane->dev;
> > struct drm_crtc *crtc = state->crtc;
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - struct intel_plane *intel_plane = to_intel_plane(plane);
> > enum pipe pipe = intel_crtc->pipe;
> > struct drm_framebuffer *fb = state->fb;
> > - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > - struct drm_i915_gem_object *obj = intel_fb->obj;
> > - struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > - int crtc_x, crtc_y;
> > - unsigned int crtc_w, crtc_h;
> > - uint32_t src_x, src_y, src_w, src_h;
> > - struct drm_rect *dst = &state->dst;
> > - const struct drm_rect *clip = &state->clip;
> > - bool primary_enabled;
> > + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> > int ret;
> >
> > - /*
> > - * If the sprite is completely covering the primary plane,
> > - * we can disable the primary and save power.
> > - */
> > - primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> > - WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> > -
> > -
> > if (old_obj != obj) {
> > mutex_lock(&dev->struct_mutex);
> >
> > @@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> > return ret;
> > }
> >
> > + return 0;
> > +}
> > +
> > +static void
> > +intel_commit_sprite_plane(struct drm_plane *plane,
> > + struct intel_plane_state *state)
> > +{
> > + struct drm_device *dev = plane->dev;
> > + struct drm_crtc *crtc = state->crtc;
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > + struct intel_plane *intel_plane = to_intel_plane(plane);
> > + enum pipe pipe = intel_crtc->pipe;
> > + struct drm_framebuffer *fb = state->fb;
> > + struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > + struct drm_i915_gem_object *obj = intel_fb->obj;
> > + struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > + int crtc_x, crtc_y;
> > + unsigned int crtc_w, crtc_h;
> > + uint32_t src_x, src_y, src_w, src_h;
> > + struct drm_rect *dst = &state->dst;
> > + const struct drm_rect *clip = &state->clip;
> > + bool primary_enabled;
> > +
> > + /*
> > + * If the sprite is completely covering the primary plane,
> > + * we can disable the primary and save power.
> > + */
> > + primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> > + WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> > +
> > intel_plane->crtc_x = state->orig_dst.x1;
> > intel_plane->crtc_y = state->orig_dst.y1;
> > intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> > @@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> > intel_unpin_fb_obj(old_obj);
> > mutex_unlock(&dev->struct_mutex);
> > }
> > -
> > - return 0;
> > }
> >
> > static int
> > @@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > if (ret)
> > return ret;
> >
> > - return intel_commit_sprite_plane(plane, &state);
> > + ret = intel_prepare_sprite_plane(plane, &state);
> > + if (ret)
> > + return ret;
> > +
> > + intel_commit_sprite_plane(plane, &state);
> > + return 0;
> > }
> >
> > static int
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates
2014-10-30 13:32 ` Paulo Zanoni
2014-10-30 14:08 ` [Intel-gfx] " Imre Deak
@ 2014-10-30 14:34 ` Ville Syrjälä
1 sibling, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2014-10-30 14:34 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Gustavo Padovan, DRI Development
On Thu, Oct 30, 2014 at 11:32:42AM -0200, Paulo Zanoni wrote:
> 2014-10-24 11:51 GMT-02:00 Gustavo Padovan <gustavo@padovan.org>:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > take out pin_fb code so the commit phase can't fail anymore.
> >
>
> According to my bisection results, this is the first bad commit of
> https://bugs.freedesktop.org/show_bug.cgi?id=85634.
>
>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++--------------
> > 1 file changed, 40 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 2c060ad..3631b0e 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1192,34 +1192,18 @@ intel_check_sprite_plane(struct drm_plane *plane,
> > }
> >
> > static int
> > -intel_commit_sprite_plane(struct drm_plane *plane,
> > - struct intel_plane_state *state)
> > +intel_prepare_sprite_plane(struct drm_plane *plane,
> > + struct intel_plane_state *state)
<snip>
> > - struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
I think this change is the culprit here. The problem is that during a
modeset we call intel_plane_disable()->intel_disable_plane() (yeah, yeah
the names suck big time :) which will unpin the current buffer and clear
intel_plane->obj, but plane->fb will still be set, so the next time
.update_plane() is called it will think the buffer was already pinned,
and not attempt to pin it again.
So just using intel_plane->obj to figure out the old obj should fix it.
But my plan sort of was that even if userspace "enables" a plane with
the crtc off, we'd still pin the buffer. Then we can be more sure that
when the crtc does get enabled the plane can actually be enabled as well
(ie. the pinning can't fail at that point anymore). So if we follow that
design I think we need to split intel_disable_plane() into two parts,
one which just does the plane disable, and the second part just does
unpinning. And then we need to just call the non-unpin variant from
intel_plane_disable() so that we don't unpin the buffer during crtc
disable. And then we should also kill intel_plane->obj.
> > int ret;
> >
> > - /*
> > - * If the sprite is completely covering the primary plane,
> > - * we can disable the primary and save power.
> > - */
> > - primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> > - WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> > -
> > -
> > if (old_obj != obj) {
> > mutex_lock(&dev->struct_mutex);
> >
> > @@ -1238,6 +1222,36 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> > return ret;
> > }
> >
> > + return 0;
> > +}
> > +
> > +static void
> > +intel_commit_sprite_plane(struct drm_plane *plane,
> > + struct intel_plane_state *state)
> > +{
> > + struct drm_device *dev = plane->dev;
> > + struct drm_crtc *crtc = state->crtc;
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > + struct intel_plane *intel_plane = to_intel_plane(plane);
> > + enum pipe pipe = intel_crtc->pipe;
> > + struct drm_framebuffer *fb = state->fb;
> > + struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > + struct drm_i915_gem_object *obj = intel_fb->obj;
> > + struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > + int crtc_x, crtc_y;
> > + unsigned int crtc_w, crtc_h;
> > + uint32_t src_x, src_y, src_w, src_h;
> > + struct drm_rect *dst = &state->dst;
> > + const struct drm_rect *clip = &state->clip;
> > + bool primary_enabled;
> > +
> > + /*
> > + * If the sprite is completely covering the primary plane,
> > + * we can disable the primary and save power.
> > + */
> > + primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> > + WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
> > +
> > intel_plane->crtc_x = state->orig_dst.x1;
> > intel_plane->crtc_y = state->orig_dst.y1;
> > intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> > @@ -1298,8 +1312,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> > intel_unpin_fb_obj(old_obj);
> > mutex_unlock(&dev->struct_mutex);
> > }
> > -
> > - return 0;
> > }
> >
> > static int
> > @@ -1339,7 +1351,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > if (ret)
> > return ret;
> >
> > - return intel_commit_sprite_plane(plane, &state);
> > + ret = intel_prepare_sprite_plane(plane, &state);
> > + if (ret)
> > + return ret;
> > +
> > + intel_commit_sprite_plane(plane, &state);
> > + return 0;
> > }
> >
> > static int
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 3/5] drm/i915: use intel_fb_obj() macros to assign gem objects
2014-10-24 13:51 [PATCH v4 1/5] drm/i915: create a prepare step for primary planes updates Gustavo Padovan
2014-10-24 13:51 ` [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates Gustavo Padovan
@ 2014-10-24 13:51 ` Gustavo Padovan
2014-10-24 13:51 ` [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active Gustavo Padovan
2014-10-24 13:51 ` [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj() Gustavo Padovan
3 siblings, 0 replies; 13+ messages in thread
From: Gustavo Padovan @ 2014-10-24 13:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Gustavo Padovan, dri-devel
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Use the macros makes the code cleaner and it also checks for a NULL fb.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_sprite.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 3631b0e..8b80d68 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1032,8 +1032,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_framebuffer *fb = state->fb;
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
int crtc_x, crtc_y;
unsigned int crtc_w, crtc_h;
uint32_t src_x, src_y, src_w, src_h;
@@ -1235,9 +1234,8 @@ intel_commit_sprite_plane(struct drm_plane *plane,
struct intel_plane *intel_plane = to_intel_plane(plane);
enum pipe pipe = intel_crtc->pipe;
struct drm_framebuffer *fb = state->fb;
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
- struct drm_i915_gem_object *old_obj = intel_plane->obj;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+ struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
int crtc_x, crtc_y;
unsigned int crtc_w, crtc_h;
uint32_t src_x, src_y, src_w, src_h;
--
1.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active
2014-10-24 13:51 [PATCH v4 1/5] drm/i915: create a prepare step for primary planes updates Gustavo Padovan
2014-10-24 13:51 ` [PATCH v4 2/5] drm/i915: create a prepare phase for sprite plane updates Gustavo Padovan
2014-10-24 13:51 ` [PATCH v4 3/5] drm/i915: use intel_fb_obj() macros to assign gem objects Gustavo Padovan
@ 2014-10-24 13:51 ` Gustavo Padovan
2014-10-24 15:07 ` Ville Syrjälä
2014-10-24 13:51 ` [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj() Gustavo Padovan
3 siblings, 1 reply; 13+ messages in thread
From: Gustavo Padovan @ 2014-10-24 13:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Gustavo Padovan, dri-devel
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
There is no point in flipping a buffer for a disabled crtc.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8530401..9a913f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8544,9 +8544,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
if (old_width != width)
intel_update_watermarks(crtc);
intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
- }
- intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+ intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+ }
return 0;
fail_unpin:
--
1.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active
2014-10-24 13:51 ` [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active Gustavo Padovan
@ 2014-10-24 15:07 ` Ville Syrjälä
2014-10-27 9:10 ` Daniel Vetter
0 siblings, 1 reply; 13+ messages in thread
From: Ville Syrjälä @ 2014-10-24 15:07 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Fri, Oct 24, 2014 at 02:51:34PM +0100, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> There is no point in flipping a buffer for a disabled crtc.
That thing doesn't actually flip but just signal the frontbuffer
tracking code that either has just flipped or is going to real soon now
(tm). But yeah, still makes no sense when the entire pipe is off, so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8530401..9a913f5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8544,9 +8544,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> if (old_width != width)
> intel_update_watermarks(crtc);
> intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> - }
>
> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> + intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> + }
>
> return 0;
> fail_unpin:
> --
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active
2014-10-24 15:07 ` Ville Syrjälä
@ 2014-10-27 9:10 ` Daniel Vetter
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2014-10-27 9:10 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Fri, Oct 24, 2014 at 06:07:15PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 24, 2014 at 02:51:34PM +0100, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > There is no point in flipping a buffer for a disabled crtc.
>
> That thing doesn't actually flip but just signal the frontbuffer
> tracking code that either has just flipped or is going to real soon now
> (tm). But yeah, still makes no sense when the entire pipe is off, so:
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()
2014-10-24 13:51 [PATCH v4 1/5] drm/i915: create a prepare step for primary planes updates Gustavo Padovan
` (2 preceding siblings ...)
2014-10-24 13:51 ` [PATCH v4 4/5] drm/i915: only flip frontbuffer if crtc is active Gustavo Padovan
@ 2014-10-24 13:51 ` Gustavo Padovan
2014-10-24 15:16 ` Ville Syrjälä
2014-10-28 12:35 ` [Intel-gfx] " Ville Syrjälä
3 siblings, 2 replies; 13+ messages in thread
From: Gustavo Padovan @ 2014-10-24 13:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Gustavo Padovan, dri-devel
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Merge it into the plane update_plane() callback and make other
users use the update_plane() functions instead.
The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
and merge both paths into one.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
1 file changed, 100 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9a913f5..60ec165 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
return true;
}
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
- struct drm_i915_gem_object *obj,
- uint32_t width, uint32_t height)
-{
- struct drm_device *dev = crtc->dev;
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- enum pipe pipe = intel_crtc->pipe;
- unsigned old_width;
- uint32_t addr;
- int ret;
-
- /* if we want to turn off the cursor ignore width and height */
- if (!obj) {
- DRM_DEBUG_KMS("cursor off\n");
- addr = 0;
- mutex_lock(&dev->struct_mutex);
- goto finish;
- }
-
- /* we only need to pin inside GTT if cursor is non-phy */
- mutex_lock(&dev->struct_mutex);
- if (!INTEL_INFO(dev)->cursor_needs_physical) {
- unsigned alignment;
-
- /*
- * Global gtt pte registers are special registers which actually
- * forward writes to a chunk of system memory. Which means that
- * there is no risk that the register values disappear as soon
- * as we call intel_runtime_pm_put(), so it is correct to wrap
- * only the pin/unpin/fence and not more.
- */
- intel_runtime_pm_get(dev_priv);
-
- /* Note that the w/a also requires 2 PTE of padding following
- * the bo. We currently fill all unused PTE with the shadow
- * page and so we should always have valid PTE following the
- * cursor preventing the VT-d warning.
- */
- alignment = 0;
- if (need_vtd_wa(dev))
- alignment = 64*1024;
-
- ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
- if (ret) {
- DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
- intel_runtime_pm_put(dev_priv);
- goto fail_locked;
- }
-
- ret = i915_gem_object_put_fence(obj);
- if (ret) {
- DRM_DEBUG_KMS("failed to release fence for cursor");
- intel_runtime_pm_put(dev_priv);
- goto fail_unpin;
- }
-
- addr = i915_gem_obj_ggtt_offset(obj);
-
- intel_runtime_pm_put(dev_priv);
- } else {
- int align = IS_I830(dev) ? 16 * 1024 : 256;
- ret = i915_gem_object_attach_phys(obj, align);
- if (ret) {
- DRM_DEBUG_KMS("failed to attach phys object\n");
- goto fail_locked;
- }
- addr = obj->phys_handle->busaddr;
- }
-
- finish:
- if (intel_crtc->cursor_bo) {
- if (!INTEL_INFO(dev)->cursor_needs_physical)
- i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
- }
-
- i915_gem_track_fb(intel_crtc->cursor_bo, obj,
- INTEL_FRONTBUFFER_CURSOR(pipe));
- mutex_unlock(&dev->struct_mutex);
-
- old_width = intel_crtc->cursor_width;
-
- intel_crtc->cursor_addr = addr;
- intel_crtc->cursor_bo = obj;
- intel_crtc->cursor_width = width;
- intel_crtc->cursor_height = height;
-
- if (intel_crtc->active) {
- if (old_width != width)
- intel_update_watermarks(crtc);
- intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-
- intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
- }
-
- return 0;
-fail_unpin:
- i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
- mutex_unlock(&dev->struct_mutex);
- return ret;
-}
-
static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
u16 *blue, uint32_t start, uint32_t size)
{
@@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
BUG_ON(!plane->crtc);
- return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+ return plane->funcs->update_plane(plane, plane->crtc, NULL,
+ 0, 0, 0, 0, 0, 0, 0, 0);
}
static int
@@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
struct intel_plane_state *state)
{
struct drm_crtc *crtc = state->crtc;
+ struct drm_device *dev = crtc->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_framebuffer *fb = state->fb;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
- int crtc_w, crtc_h;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+ enum pipe pipe = intel_crtc->pipe;
+ unsigned old_width;
+ uint32_t addr;
+ int ret;
crtc->cursor_x = state->orig_dst.x1;
crtc->cursor_y = state->orig_dst.y1;
- if (fb != crtc->cursor->fb) {
- crtc_w = drm_rect_width(&state->orig_dst);
- crtc_h = drm_rect_height(&state->orig_dst);
- return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+
+ if (intel_crtc->cursor_bo == obj)
+ goto update;
+
+ /* if we want to turn off the cursor ignore width and height */
+ if (!obj) {
+ DRM_DEBUG_KMS("cursor off\n");
+ addr = 0;
+ mutex_lock(&dev->struct_mutex);
+ goto finish;
+ }
+
+ /* we only need to pin inside GTT if cursor is non-phy */
+ mutex_lock(&dev->struct_mutex);
+ if (!INTEL_INFO(dev)->cursor_needs_physical) {
+ unsigned alignment;
+
+ /*
+ * Global gtt pte registers are special registers which actually
+ * forward writes to a chunk of system memory. Which means that
+ * there is no risk that the register values disappear as soon
+ * as we call intel_runtime_pm_put(), so it is correct to wrap
+ * only the pin/unpin/fence and not more.
+ */
+ intel_runtime_pm_get(dev_priv);
+
+ /* Note that the w/a also requires 2 PTE of padding following
+ * the bo. We currently fill all unused PTE with the shadow
+ * page and so we should always have valid PTE following the
+ * cursor preventing the VT-d warning.
+ */
+ alignment = 0;
+ if (need_vtd_wa(dev))
+ alignment = 64*1024;
+
+ ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+ if (ret) {
+ DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+ intel_runtime_pm_put(dev_priv);
+ goto fail_locked;
+ }
+
+ ret = i915_gem_object_put_fence(obj);
+ if (ret) {
+ DRM_DEBUG_KMS("failed to release fence for cursor");
+ intel_runtime_pm_put(dev_priv);
+ goto fail_unpin;
+ }
+
+ addr = i915_gem_obj_ggtt_offset(obj);
+
+ intel_runtime_pm_put(dev_priv);
} else {
- intel_crtc_update_cursor(crtc, state->visible);
+ int align = IS_I830(dev) ? 16 * 1024 : 256;
+ ret = i915_gem_object_attach_phys(obj, align);
+ if (ret) {
+ DRM_DEBUG_KMS("failed to attach phys object\n");
+ goto fail_locked;
+ }
+ addr = obj->phys_handle->busaddr;
+ }
- intel_frontbuffer_flip(crtc->dev,
- INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+finish:
+ if (intel_crtc->cursor_bo) {
+ if (!INTEL_INFO(dev)->cursor_needs_physical)
+ i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+ }
- return 0;
+ i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+ INTEL_FRONTBUFFER_CURSOR(pipe));
+ mutex_unlock(&dev->struct_mutex);
+
+ intel_crtc->cursor_addr = addr;
+ intel_crtc->cursor_bo = obj;
+update:
+ old_width = intel_crtc->cursor_width;
+
+ intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
+ intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
+
+ if (intel_crtc->active) {
+ if (old_width != intel_crtc->cursor_width)
+ intel_update_watermarks(crtc);
+ intel_crtc_update_cursor(crtc, state->visible);
+
+ intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
}
+
+ return 0;
+fail_unpin:
+ i915_gem_object_unpin_from_display_plane(obj);
+fail_locked:
+ mutex_unlock(&dev->struct_mutex);
+ drm_gem_object_unreference_unlocked(&obj->base);
+ return ret;
}
static int
--
1.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()
2014-10-24 13:51 ` [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj() Gustavo Padovan
@ 2014-10-24 15:16 ` Ville Syrjälä
2014-10-28 12:02 ` Gustavo Padovan
2014-10-28 12:35 ` [Intel-gfx] " Ville Syrjälä
1 sibling, 1 reply; 13+ messages in thread
From: Ville Syrjälä @ 2014-10-24 15:16 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Fri, Oct 24, 2014 at 02:51:35PM +0100, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> Merge it into the plane update_plane() callback and make other
> users use the update_plane() functions instead.
>
> The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
> so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
> and merge both paths into one.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
> 1 file changed, 100 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9a913f5..60ec165 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
> return true;
> }
>
> -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> - struct drm_i915_gem_object *obj,
> - uint32_t width, uint32_t height)
> -{
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - enum pipe pipe = intel_crtc->pipe;
> - unsigned old_width;
> - uint32_t addr;
> - int ret;
> -
> - /* if we want to turn off the cursor ignore width and height */
> - if (!obj) {
> - DRM_DEBUG_KMS("cursor off\n");
> - addr = 0;
> - mutex_lock(&dev->struct_mutex);
> - goto finish;
> - }
> -
> - /* we only need to pin inside GTT if cursor is non-phy */
> - mutex_lock(&dev->struct_mutex);
> - if (!INTEL_INFO(dev)->cursor_needs_physical) {
> - unsigned alignment;
> -
> - /*
> - * Global gtt pte registers are special registers which actually
> - * forward writes to a chunk of system memory. Which means that
> - * there is no risk that the register values disappear as soon
> - * as we call intel_runtime_pm_put(), so it is correct to wrap
> - * only the pin/unpin/fence and not more.
> - */
> - intel_runtime_pm_get(dev_priv);
> -
> - /* Note that the w/a also requires 2 PTE of padding following
> - * the bo. We currently fill all unused PTE with the shadow
> - * page and so we should always have valid PTE following the
> - * cursor preventing the VT-d warning.
> - */
> - alignment = 0;
> - if (need_vtd_wa(dev))
> - alignment = 64*1024;
> -
> - ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> - if (ret) {
> - DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> - intel_runtime_pm_put(dev_priv);
> - goto fail_locked;
> - }
> -
> - ret = i915_gem_object_put_fence(obj);
> - if (ret) {
> - DRM_DEBUG_KMS("failed to release fence for cursor");
> - intel_runtime_pm_put(dev_priv);
> - goto fail_unpin;
> - }
> -
> - addr = i915_gem_obj_ggtt_offset(obj);
> -
> - intel_runtime_pm_put(dev_priv);
> - } else {
> - int align = IS_I830(dev) ? 16 * 1024 : 256;
> - ret = i915_gem_object_attach_phys(obj, align);
> - if (ret) {
> - DRM_DEBUG_KMS("failed to attach phys object\n");
> - goto fail_locked;
> - }
> - addr = obj->phys_handle->busaddr;
> - }
> -
> - finish:
> - if (intel_crtc->cursor_bo) {
> - if (!INTEL_INFO(dev)->cursor_needs_physical)
> - i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> - }
> -
> - i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> - INTEL_FRONTBUFFER_CURSOR(pipe));
> - mutex_unlock(&dev->struct_mutex);
> -
> - old_width = intel_crtc->cursor_width;
> -
> - intel_crtc->cursor_addr = addr;
> - intel_crtc->cursor_bo = obj;
> - intel_crtc->cursor_width = width;
> - intel_crtc->cursor_height = height;
> -
> - if (intel_crtc->active) {
> - if (old_width != width)
> - intel_update_watermarks(crtc);
> - intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> -
> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> - }
> -
> - return 0;
> -fail_unpin:
> - i915_gem_object_unpin_from_display_plane(obj);
> -fail_locked:
> - mutex_unlock(&dev->struct_mutex);
> - return ret;
> -}
> -
> static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> u16 *blue, uint32_t start, uint32_t size)
> {
> @@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
>
> BUG_ON(!plane->crtc);
>
> - return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> + return plane->funcs->update_plane(plane, plane->crtc, NULL,
> + 0, 0, 0, 0, 0, 0, 0, 0);
> }
>
> static int
> @@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> struct intel_plane_state *state)
> {
> struct drm_crtc *crtc = state->crtc;
> + struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_framebuffer *fb = state->fb;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> - struct drm_i915_gem_object *obj = intel_fb->obj;
> - int crtc_w, crtc_h;
> + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> + enum pipe pipe = intel_crtc->pipe;
> + unsigned old_width;
> + uint32_t addr;
> + int ret;
>
> crtc->cursor_x = state->orig_dst.x1;
> crtc->cursor_y = state->orig_dst.y1;
> - if (fb != crtc->cursor->fb) {
> - crtc_w = drm_rect_width(&state->orig_dst);
> - crtc_h = drm_rect_height(&state->orig_dst);
> - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> +
> + if (intel_crtc->cursor_bo == obj)
> + goto update;
> +
> + /* if we want to turn off the cursor ignore width and height */
> + if (!obj) {
> + DRM_DEBUG_KMS("cursor off\n");
> + addr = 0;
> + mutex_lock(&dev->struct_mutex);
> + goto finish;
> + }
> +
> + /* we only need to pin inside GTT if cursor is non-phy */
> + mutex_lock(&dev->struct_mutex);
> + if (!INTEL_INFO(dev)->cursor_needs_physical) {
> + unsigned alignment;
> +
> + /*
> + * Global gtt pte registers are special registers which actually
> + * forward writes to a chunk of system memory. Which means that
> + * there is no risk that the register values disappear as soon
> + * as we call intel_runtime_pm_put(), so it is correct to wrap
> + * only the pin/unpin/fence and not more.
> + */
> + intel_runtime_pm_get(dev_priv);
> +
> + /* Note that the w/a also requires 2 PTE of padding following
> + * the bo. We currently fill all unused PTE with the shadow
> + * page and so we should always have valid PTE following the
> + * cursor preventing the VT-d warning.
> + */
> + alignment = 0;
> + if (need_vtd_wa(dev))
> + alignment = 64*1024;
> +
> + ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> + if (ret) {
> + DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> + intel_runtime_pm_put(dev_priv);
> + goto fail_locked;
> + }
> +
> + ret = i915_gem_object_put_fence(obj);
> + if (ret) {
> + DRM_DEBUG_KMS("failed to release fence for cursor");
> + intel_runtime_pm_put(dev_priv);
> + goto fail_unpin;
> + }
> +
> + addr = i915_gem_obj_ggtt_offset(obj);
> +
> + intel_runtime_pm_put(dev_priv);
> } else {
> - intel_crtc_update_cursor(crtc, state->visible);
> + int align = IS_I830(dev) ? 16 * 1024 : 256;
> + ret = i915_gem_object_attach_phys(obj, align);
> + if (ret) {
> + DRM_DEBUG_KMS("failed to attach phys object\n");
> + goto fail_locked;
> + }
> + addr = obj->phys_handle->busaddr;
> + }
>
> - intel_frontbuffer_flip(crtc->dev,
> - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> +finish:
> + if (intel_crtc->cursor_bo) {
> + if (!INTEL_INFO(dev)->cursor_needs_physical)
> + i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> + }
>
> - return 0;
> + i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> + INTEL_FRONTBUFFER_CURSOR(pipe));
> + mutex_unlock(&dev->struct_mutex);
> +
> + intel_crtc->cursor_addr = addr;
> + intel_crtc->cursor_bo = obj;
> +update:
> + old_width = intel_crtc->cursor_width;
> +
> + intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> + intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> +
> + if (intel_crtc->active) {
> + if (old_width != intel_crtc->cursor_width)
> + intel_update_watermarks(crtc);
> + intel_crtc_update_cursor(crtc, state->visible);
So we need to make sure state->visible==false when there's no fb. I
suppose we should just do that in drm_plane_helper_check_update().
> +
> + intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> }
> +
> + return 0;
> +fail_unpin:
> + i915_gem_object_unpin_from_display_plane(obj);
> +fail_locked:
> + mutex_unlock(&dev->struct_mutex);
> + drm_gem_object_unreference_unlocked(&obj->base);
> + return ret;
> }
>
> static int
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()
2014-10-24 15:16 ` Ville Syrjälä
@ 2014-10-28 12:02 ` Gustavo Padovan
0 siblings, 0 replies; 13+ messages in thread
From: Gustavo Padovan @ 2014-10-28 12:02 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, dri-devel
Hi Ville,
2014-10-24 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Fri, Oct 24, 2014 at 02:51:35PM +0100, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > Merge it into the plane update_plane() callback and make other
> > users use the update_plane() functions instead.
> >
> > The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
> > so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
> > and merge both paths into one.
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
> > 1 file changed, 100 insertions(+), 115 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9a913f5..60ec165 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
> > return true;
> > }
> >
> > -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> > - struct drm_i915_gem_object *obj,
> > - uint32_t width, uint32_t height)
> > -{
> > - struct drm_device *dev = crtc->dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - enum pipe pipe = intel_crtc->pipe;
> > - unsigned old_width;
> > - uint32_t addr;
> > - int ret;
> > -
> > - /* if we want to turn off the cursor ignore width and height */
> > - if (!obj) {
> > - DRM_DEBUG_KMS("cursor off\n");
> > - addr = 0;
> > - mutex_lock(&dev->struct_mutex);
> > - goto finish;
> > - }
> > -
> > - /* we only need to pin inside GTT if cursor is non-phy */
> > - mutex_lock(&dev->struct_mutex);
> > - if (!INTEL_INFO(dev)->cursor_needs_physical) {
> > - unsigned alignment;
> > -
> > - /*
> > - * Global gtt pte registers are special registers which actually
> > - * forward writes to a chunk of system memory. Which means that
> > - * there is no risk that the register values disappear as soon
> > - * as we call intel_runtime_pm_put(), so it is correct to wrap
> > - * only the pin/unpin/fence and not more.
> > - */
> > - intel_runtime_pm_get(dev_priv);
> > -
> > - /* Note that the w/a also requires 2 PTE of padding following
> > - * the bo. We currently fill all unused PTE with the shadow
> > - * page and so we should always have valid PTE following the
> > - * cursor preventing the VT-d warning.
> > - */
> > - alignment = 0;
> > - if (need_vtd_wa(dev))
> > - alignment = 64*1024;
> > -
> > - ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> > - if (ret) {
> > - DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> > - intel_runtime_pm_put(dev_priv);
> > - goto fail_locked;
> > - }
> > -
> > - ret = i915_gem_object_put_fence(obj);
> > - if (ret) {
> > - DRM_DEBUG_KMS("failed to release fence for cursor");
> > - intel_runtime_pm_put(dev_priv);
> > - goto fail_unpin;
> > - }
> > -
> > - addr = i915_gem_obj_ggtt_offset(obj);
> > -
> > - intel_runtime_pm_put(dev_priv);
> > - } else {
> > - int align = IS_I830(dev) ? 16 * 1024 : 256;
> > - ret = i915_gem_object_attach_phys(obj, align);
> > - if (ret) {
> > - DRM_DEBUG_KMS("failed to attach phys object\n");
> > - goto fail_locked;
> > - }
> > - addr = obj->phys_handle->busaddr;
> > - }
> > -
> > - finish:
> > - if (intel_crtc->cursor_bo) {
> > - if (!INTEL_INFO(dev)->cursor_needs_physical)
> > - i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > - }
> > -
> > - i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > - INTEL_FRONTBUFFER_CURSOR(pipe));
> > - mutex_unlock(&dev->struct_mutex);
> > -
> > - old_width = intel_crtc->cursor_width;
> > -
> > - intel_crtc->cursor_addr = addr;
> > - intel_crtc->cursor_bo = obj;
> > - intel_crtc->cursor_width = width;
> > - intel_crtc->cursor_height = height;
> > -
> > - if (intel_crtc->active) {
> > - if (old_width != width)
> > - intel_update_watermarks(crtc);
> > - intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> > -
> > - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> > - }
> > -
> > - return 0;
> > -fail_unpin:
> > - i915_gem_object_unpin_from_display_plane(obj);
> > -fail_locked:
> > - mutex_unlock(&dev->struct_mutex);
> > - return ret;
> > -}
> > -
> > static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> > u16 *blue, uint32_t start, uint32_t size)
> > {
> > @@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
> >
> > BUG_ON(!plane->crtc);
> >
> > - return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> > + return plane->funcs->update_plane(plane, plane->crtc, NULL,
> > + 0, 0, 0, 0, 0, 0, 0, 0);
> > }
> >
> > static int
> > @@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> > struct intel_plane_state *state)
> > {
> > struct drm_crtc *crtc = state->crtc;
> > + struct drm_device *dev = crtc->dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_framebuffer *fb = state->fb;
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > - struct drm_i915_gem_object *obj = intel_fb->obj;
> > - int crtc_w, crtc_h;
> > + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > + enum pipe pipe = intel_crtc->pipe;
> > + unsigned old_width;
> > + uint32_t addr;
> > + int ret;
> >
> > crtc->cursor_x = state->orig_dst.x1;
> > crtc->cursor_y = state->orig_dst.y1;
> > - if (fb != crtc->cursor->fb) {
> > - crtc_w = drm_rect_width(&state->orig_dst);
> > - crtc_h = drm_rect_height(&state->orig_dst);
> > - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> > +
> > + if (intel_crtc->cursor_bo == obj)
> > + goto update;
> > +
> > + /* if we want to turn off the cursor ignore width and height */
> > + if (!obj) {
> > + DRM_DEBUG_KMS("cursor off\n");
> > + addr = 0;
> > + mutex_lock(&dev->struct_mutex);
> > + goto finish;
> > + }
> > +
> > + /* we only need to pin inside GTT if cursor is non-phy */
> > + mutex_lock(&dev->struct_mutex);
> > + if (!INTEL_INFO(dev)->cursor_needs_physical) {
> > + unsigned alignment;
> > +
> > + /*
> > + * Global gtt pte registers are special registers which actually
> > + * forward writes to a chunk of system memory. Which means that
> > + * there is no risk that the register values disappear as soon
> > + * as we call intel_runtime_pm_put(), so it is correct to wrap
> > + * only the pin/unpin/fence and not more.
> > + */
> > + intel_runtime_pm_get(dev_priv);
> > +
> > + /* Note that the w/a also requires 2 PTE of padding following
> > + * the bo. We currently fill all unused PTE with the shadow
> > + * page and so we should always have valid PTE following the
> > + * cursor preventing the VT-d warning.
> > + */
> > + alignment = 0;
> > + if (need_vtd_wa(dev))
> > + alignment = 64*1024;
> > +
> > + ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> > + if (ret) {
> > + DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> > + intel_runtime_pm_put(dev_priv);
> > + goto fail_locked;
> > + }
> > +
> > + ret = i915_gem_object_put_fence(obj);
> > + if (ret) {
> > + DRM_DEBUG_KMS("failed to release fence for cursor");
> > + intel_runtime_pm_put(dev_priv);
> > + goto fail_unpin;
> > + }
> > +
> > + addr = i915_gem_obj_ggtt_offset(obj);
> > +
> > + intel_runtime_pm_put(dev_priv);
> > } else {
> > - intel_crtc_update_cursor(crtc, state->visible);
> > + int align = IS_I830(dev) ? 16 * 1024 : 256;
> > + ret = i915_gem_object_attach_phys(obj, align);
> > + if (ret) {
> > + DRM_DEBUG_KMS("failed to attach phys object\n");
> > + goto fail_locked;
> > + }
> > + addr = obj->phys_handle->busaddr;
> > + }
> >
> > - intel_frontbuffer_flip(crtc->dev,
> > - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> > +finish:
> > + if (intel_crtc->cursor_bo) {
> > + if (!INTEL_INFO(dev)->cursor_needs_physical)
> > + i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > + }
> >
> > - return 0;
> > + i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > + INTEL_FRONTBUFFER_CURSOR(pipe));
> > + mutex_unlock(&dev->struct_mutex);
> > +
> > + intel_crtc->cursor_addr = addr;
> > + intel_crtc->cursor_bo = obj;
> > +update:
> > + old_width = intel_crtc->cursor_width;
> > +
> > + intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> > + intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> > +
> > + if (intel_crtc->active) {
> > + if (old_width != intel_crtc->cursor_width)
> > + intel_update_watermarks(crtc);
> > + intel_crtc_update_cursor(crtc, state->visible);
>
> So we need to make sure state->visible==false when there's no fb. I
> suppose we should just do that in drm_plane_helper_check_update().
The code to check that is merged already, do you have any other comment on
this patch?
Gustavo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Intel-gfx] [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj()
2014-10-24 13:51 ` [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj() Gustavo Padovan
2014-10-24 15:16 ` Ville Syrjälä
@ 2014-10-28 12:35 ` Ville Syrjälä
1 sibling, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2014-10-28 12:35 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Fri, Oct 24, 2014 at 02:51:35PM +0100, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> Merge it into the plane update_plane() callback and make other
> users use the update_plane() functions instead.
>
> The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
> so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
> and merge both paths into one.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
> 1 file changed, 100 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9a913f5..60ec165 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8453,109 +8453,6 @@ static bool cursor_size_ok(struct drm_device *dev,
> return true;
> }
>
> -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> - struct drm_i915_gem_object *obj,
> - uint32_t width, uint32_t height)
> -{
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - enum pipe pipe = intel_crtc->pipe;
> - unsigned old_width;
> - uint32_t addr;
> - int ret;
> -
> - /* if we want to turn off the cursor ignore width and height */
> - if (!obj) {
> - DRM_DEBUG_KMS("cursor off\n");
> - addr = 0;
> - mutex_lock(&dev->struct_mutex);
> - goto finish;
> - }
> -
> - /* we only need to pin inside GTT if cursor is non-phy */
> - mutex_lock(&dev->struct_mutex);
> - if (!INTEL_INFO(dev)->cursor_needs_physical) {
> - unsigned alignment;
> -
> - /*
> - * Global gtt pte registers are special registers which actually
> - * forward writes to a chunk of system memory. Which means that
> - * there is no risk that the register values disappear as soon
> - * as we call intel_runtime_pm_put(), so it is correct to wrap
> - * only the pin/unpin/fence and not more.
> - */
> - intel_runtime_pm_get(dev_priv);
> -
> - /* Note that the w/a also requires 2 PTE of padding following
> - * the bo. We currently fill all unused PTE with the shadow
> - * page and so we should always have valid PTE following the
> - * cursor preventing the VT-d warning.
> - */
> - alignment = 0;
> - if (need_vtd_wa(dev))
> - alignment = 64*1024;
> -
> - ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> - if (ret) {
> - DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> - intel_runtime_pm_put(dev_priv);
> - goto fail_locked;
> - }
> -
> - ret = i915_gem_object_put_fence(obj);
> - if (ret) {
> - DRM_DEBUG_KMS("failed to release fence for cursor");
> - intel_runtime_pm_put(dev_priv);
> - goto fail_unpin;
> - }
> -
> - addr = i915_gem_obj_ggtt_offset(obj);
> -
> - intel_runtime_pm_put(dev_priv);
> - } else {
> - int align = IS_I830(dev) ? 16 * 1024 : 256;
> - ret = i915_gem_object_attach_phys(obj, align);
> - if (ret) {
> - DRM_DEBUG_KMS("failed to attach phys object\n");
> - goto fail_locked;
> - }
> - addr = obj->phys_handle->busaddr;
> - }
> -
> - finish:
> - if (intel_crtc->cursor_bo) {
> - if (!INTEL_INFO(dev)->cursor_needs_physical)
> - i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> - }
> -
> - i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> - INTEL_FRONTBUFFER_CURSOR(pipe));
> - mutex_unlock(&dev->struct_mutex);
> -
> - old_width = intel_crtc->cursor_width;
> -
> - intel_crtc->cursor_addr = addr;
> - intel_crtc->cursor_bo = obj;
> - intel_crtc->cursor_width = width;
> - intel_crtc->cursor_height = height;
> -
> - if (intel_crtc->active) {
> - if (old_width != width)
> - intel_update_watermarks(crtc);
> - intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> -
> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> - }
> -
> - return 0;
> -fail_unpin:
> - i915_gem_object_unpin_from_display_plane(obj);
> -fail_locked:
> - mutex_unlock(&dev->struct_mutex);
> - return ret;
> -}
> -
> static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> u16 *blue, uint32_t start, uint32_t size)
> {
> @@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
>
> BUG_ON(!plane->crtc);
>
> - return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> + return plane->funcs->update_plane(plane, plane->crtc, NULL,
> + 0, 0, 0, 0, 0, 0, 0, 0);
> }
>
> static int
> @@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> struct intel_plane_state *state)
> {
> struct drm_crtc *crtc = state->crtc;
> + struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_framebuffer *fb = state->fb;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> - struct drm_i915_gem_object *obj = intel_fb->obj;
> - int crtc_w, crtc_h;
> + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> + enum pipe pipe = intel_crtc->pipe;
> + unsigned old_width;
> + uint32_t addr;
> + int ret;
>
> crtc->cursor_x = state->orig_dst.x1;
> crtc->cursor_y = state->orig_dst.y1;
> - if (fb != crtc->cursor->fb) {
> - crtc_w = drm_rect_width(&state->orig_dst);
> - crtc_h = drm_rect_height(&state->orig_dst);
> - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> +
> + if (intel_crtc->cursor_bo == obj)
> + goto update;
> +
> + /* if we want to turn off the cursor ignore width and height */
> + if (!obj) {
> + DRM_DEBUG_KMS("cursor off\n");
> + addr = 0;
> + mutex_lock(&dev->struct_mutex);
> + goto finish;
> + }
> +
> + /* we only need to pin inside GTT if cursor is non-phy */
> + mutex_lock(&dev->struct_mutex);
> + if (!INTEL_INFO(dev)->cursor_needs_physical) {
> + unsigned alignment;
> +
> + /*
> + * Global gtt pte registers are special registers which actually
> + * forward writes to a chunk of system memory. Which means that
> + * there is no risk that the register values disappear as soon
> + * as we call intel_runtime_pm_put(), so it is correct to wrap
> + * only the pin/unpin/fence and not more.
> + */
> + intel_runtime_pm_get(dev_priv);
> +
> + /* Note that the w/a also requires 2 PTE of padding following
> + * the bo. We currently fill all unused PTE with the shadow
> + * page and so we should always have valid PTE following the
> + * cursor preventing the VT-d warning.
> + */
> + alignment = 0;
> + if (need_vtd_wa(dev))
> + alignment = 64*1024;
> +
> + ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> + if (ret) {
> + DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> + intel_runtime_pm_put(dev_priv);
> + goto fail_locked;
> + }
> +
> + ret = i915_gem_object_put_fence(obj);
> + if (ret) {
> + DRM_DEBUG_KMS("failed to release fence for cursor");
> + intel_runtime_pm_put(dev_priv);
> + goto fail_unpin;
> + }
> +
> + addr = i915_gem_obj_ggtt_offset(obj);
> +
> + intel_runtime_pm_put(dev_priv);
> } else {
> - intel_crtc_update_cursor(crtc, state->visible);
> + int align = IS_I830(dev) ? 16 * 1024 : 256;
> + ret = i915_gem_object_attach_phys(obj, align);
> + if (ret) {
> + DRM_DEBUG_KMS("failed to attach phys object\n");
> + goto fail_locked;
> + }
> + addr = obj->phys_handle->busaddr;
> + }
>
> - intel_frontbuffer_flip(crtc->dev,
> - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> +finish:
> + if (intel_crtc->cursor_bo) {
> + if (!INTEL_INFO(dev)->cursor_needs_physical)
> + i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> + }
>
> - return 0;
> + i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> + INTEL_FRONTBUFFER_CURSOR(pipe));
> + mutex_unlock(&dev->struct_mutex);
> +
> + intel_crtc->cursor_addr = addr;
> + intel_crtc->cursor_bo = obj;
> +update:
> + old_width = intel_crtc->cursor_width;
> +
> + intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> + intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> +
> + if (intel_crtc->active) {
> + if (old_width != intel_crtc->cursor_width)
> + intel_update_watermarks(crtc);
> + intel_crtc_update_cursor(crtc, state->visible);
> +
> + intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> }
> +
> + return 0;
> +fail_unpin:
> + i915_gem_object_unpin_from_display_plane(obj);
> +fail_locked:
> + mutex_unlock(&dev->struct_mutex);
> + drm_gem_object_unreference_unlocked(&obj->base);
That unref needs to be dropped. With that change the patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> + return ret;
> }
>
> static int
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread