Re: [PATCH] drm/i915: Limit Valleyview and earlier to only using mappable scanout

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	"# v4 . 9-rc1+" <drm-intel-fixes@lists.freedesktop.org>,
	Akash Goel <akash.goel@intel.com>
Subject: Re: [PATCH] drm/i915: Limit Valleyview and earlier to only using mappable scanout
Date: Fri, 04 Nov 2016 16:17:43 +0200	[thread overview]
Message-ID: <8737j7ib20.fsf@intel.com> (raw)
In-Reply-To: <20161104130630.GA20322@nuc-i3427.alporthouse.com>

On Fri, 04 Nov 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Nov 04, 2016 at 12:59:08PM +0000, Tvrtko Ursulin wrote:
>> 
>> On 04/11/2016 11:08, Chris Wilson wrote:
>> >Valleyview and Cherryview are definitely limited to only scanning out
>> >from the first 256MiB and 512MiB of the Global GTT respectively. Lets
>> >presume that this behaviour was inherited from the display block copied
>> >from g4x (not Ironlake) and all earlier generations are similarly
>> >affected. For simplicity, impose that these platforms must scanout from
>> >the mappable region.
>> >
>> >Reported-by: Luis Botello <luis.botello.ortega@intel.com>
>> >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98036
>> >Fixes: 2efb813d5388 ("drm/i915: Fallback to using unmappable memory for scanout")
>> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >Cc: Akash Goel <akash.goel@intel.com>
>> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.9-rc1+
>> >---
>> >This leaves Ironlake -> Haswell with a bit of uncertainity. It is also
>> >not clear if the scanout accessible region is similarly limited on all
>> >gen8+, and so whether we need to similarly curtain the upper range for
>> >their scanouts.
>> >---
>> > drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++--
>> > 1 file changed, 16 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> >index 269e2487c104..408875fbec66 100644
>> >--- a/drivers/gpu/drm/i915/i915_gem.c
>> >+++ b/drivers/gpu/drm/i915/i915_gem.c
>> >@@ -3661,8 +3661,22 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>> > 	if (view->type == I915_GGTT_VIEW_NORMAL)
>> > 		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment,
>> > 					       PIN_MAPPABLE | PIN_NONBLOCK);
>> >-	if (IS_ERR(vma))
>> >-		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, 0);
>> >+	if (IS_ERR(vma)) {
>> >+		struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> 
>> dev_priv ?
>> 
>> What do we do with i915_params being a global i915?
>
> Sssh, I'm gradually waging war against dev_priv.
> Eventually Jani won't be able to complain about i915 being the minority.
>
> The global modparams is an easy rename.

I just liked that i915.foo was the same on both the kernel command line
and in code. I kinda still do, but like Chris I'm not too fond of
dev_priv either, and i915 seems like a good replacement.

Seeing how module parameters multiply like rabbits, with all sorts of
sanitization, how the parameters are changed in kernel, and
/sys/module/i915/parameters/ not reflecting what the user did, maybe you
could come up with something nice for that while at it...

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx