diff mbox

[1/6] drm/i915: Fail if we can't get a fence for gen2/3 tiled scanout

Message ID 20180221160235.11134-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Feb. 21, 2018, 4:02 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Gen2/3 display engine depends on the fence for tiled scanout. So if we
fail to get a fence fail the entire operation.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Chris Wilson Feb. 21, 2018, 9:52 p.m. UTC | #1 
Quoting Ville Syrjala (2018-02-21 16:02:30)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Gen2/3 display engine depends on the fence for tiled scanout. So if we
> fail to get a fence fail the entire operation.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5d46771d58f6..66b269bc24b9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2123,6 +2123,8 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>                 goto err;
>  
>         if (i915_vma_is_map_and_fenceable(vma)) {
> +               int ret;
> +
>                 /* Install a fence for tiled scan-out. Pre-i965 always needs a
>                  * fence, whereas 965+ only requires a fence if using
>                  * framebuffer compression.  For simplicity, we always, when
> @@ -2139,7 +2141,13 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>                  * something and try to run the system in a "less than optimal"
>                  * mode that matches the user configuration.
>                  */
> -               if (i915_vma_pin_fence(vma) == 0 && vma->fence)
> +               ret = i915_vma_pin_fence(vma);
> +               if (ret != 0 && INTEL_GEN(dev_priv) < 4) {
> +                       vma = ERR_PTR(ret);
> +                       goto err;
> +               }
> +
> +               if (ret == 0 && vma->fence)
>                         *out_flags |= PLANE_HAS_FENCE;
>         }

Ok, I'd like to see INTEL_GEN(dev_priv) < 4 be replaced with say
needs_fence (and may be passed in from the caller like wants_fence?).
Then I'm wondering if a 
	if (WARN_ON(needs_fence && !(*flags & PLANE_HAS_FENCE))
makes sense.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä Feb. 22, 2018, 2:13 p.m. UTC | #2 
On Wed, Feb 21, 2018 at 09:52:04PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-02-21 16:02:30)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Gen2/3 display engine depends on the fence for tiled scanout. So if we
> > fail to get a fence fail the entire operation.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5d46771d58f6..66b269bc24b9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2123,6 +2123,8 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> >                 goto err;
> >  
> >         if (i915_vma_is_map_and_fenceable(vma)) {
> > +               int ret;
> > +
> >                 /* Install a fence for tiled scan-out. Pre-i965 always needs a
> >                  * fence, whereas 965+ only requires a fence if using
> >                  * framebuffer compression.  For simplicity, we always, when
> > @@ -2139,7 +2141,13 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> >                  * something and try to run the system in a "less than optimal"
> >                  * mode that matches the user configuration.
> >                  */
> > -               if (i915_vma_pin_fence(vma) == 0 && vma->fence)
> > +               ret = i915_vma_pin_fence(vma);
> > +               if (ret != 0 && INTEL_GEN(dev_priv) < 4) {
> > +                       vma = ERR_PTR(ret);
> > +                       goto err;
> > +               }
> > +
> > +               if (ret == 0 && vma->fence)
> >                         *out_flags |= PLANE_HAS_FENCE;
> >         }
> 
> Ok, I'd like to see INTEL_GEN(dev_priv) < 4 be replaced with say
> needs_fence (and may be passed in from the caller like wants_fence?).

I had that earlier, but then I didn't have the uses_fence. Maybe I
cook up some kind of input flags thing here with PLANE_NEEDS_FENCE
and PLANE_WANTS_FENCE (maybe with a better naming scheme to
distinguish from the output flags, or should we just share the
same namespace?).

And should we then move the gmch check out and instead have something
like PLANE_NEEDS_MAPPABLE?

> Then I'm wondering if a 
> 	if (WARN_ON(needs_fence && !(*flags & PLANE_HAS_FENCE))
> makes sense.

Just to make sure i915_vma_pin_fence() did its job correctly?

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
Chris Wilson Feb. 22, 2018, 2:21 p.m. UTC | #3 
Quoting Ville Syrjälä (2018-02-22 14:13:34)
> On Wed, Feb 21, 2018 at 09:52:04PM +0000, Chris Wilson wrote:
> > Ok, I'd like to see INTEL_GEN(dev_priv) < 4 be replaced with say
> > needs_fence (and may be passed in from the caller like wants_fence?).
> 
> I had that earlier, but then I didn't have the uses_fence. Maybe I
> cook up some kind of input flags thing here with PLANE_NEEDS_FENCE
> and PLANE_WANTS_FENCE (maybe with a better naming scheme to
> distinguish from the output flags, or should we just share the
> same namespace?).

It's probably not worth it unless we want some more flexibility in
future.

> And should we then move the gmch check out and instead have something
> like PLANE_NEEDS_MAPPABLE?
> 
> > Then I'm wondering if a 
> >       if (WARN_ON(needs_fence && !(*flags & PLANE_HAS_FENCE))
> > makes sense.
> 
> Just to make sure i915_vma_pin_fence() did its job correctly?

and i915_vma_pin() etc, yes.

At the end we would have something like:

	i915_vma_pin();
	if (uses_fence && i915_vma_pin_fence())
		flags |= HAS_FENCE;
	if (WARN_ON(needs_fence && !(flags & HAS_FENCE))
		...

(with the error checking along the way, it will be even less clear).
I expect the controlling logic to only get more complicated, so having a
few sanity checks between wants and gets seems useful.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5d46771d58f6..66b269bc24b9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2123,6 +2123,8 @@  intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
 		goto err;
 
 	if (i915_vma_is_map_and_fenceable(vma)) {
+		int ret;
+
 		/* Install a fence for tiled scan-out. Pre-i965 always needs a
 		 * fence, whereas 965+ only requires a fence if using
 		 * framebuffer compression.  For simplicity, we always, when
@@ -2139,7 +2141,13 @@  intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
 		 * something and try to run the system in a "less than optimal"
 		 * mode that matches the user configuration.
 		 */
-		if (i915_vma_pin_fence(vma) == 0 && vma->fence)
+		ret = i915_vma_pin_fence(vma);
+		if (ret != 0 && INTEL_GEN(dev_priv) < 4) {
+			vma = ERR_PTR(ret);
+			goto err;
+		}
+
+		if (ret == 0 && vma->fence)
 			*out_flags |= PLANE_HAS_FENCE;
 	}