From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Drop the overzealous warning from i915_gem_set_cache_level Date: Tue, 13 Aug 2013 15:12:59 +0300 Message-ID: <20130813121259.GE7159@intel.com> References: <1376304377-11695-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 9F3A943683 for ; Tue, 13 Aug 2013 05:13:02 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1376304377-11695-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Aug 12, 2013 at 11:46:17AM +0100, Chris Wilson wrote: > By our earlier reckoning, move from a snooped/llc setting to an uncached > setting, leaves the CPU cache in a consistent state irrespective of our > domain tracking - so we can forgo the warning about the lack of > invalidation. Similarly for any writes posted to the snooped CPU domain, > we know will be safely clflushed to the uncached PTEs after forcing the > domain change. > = > Signed-off-by: Chris Wilson > Cc: Ville Syrj=E4l=E4 > --- > drivers/gpu/drm/i915/i915_gem.c | 1 - > 1 file changed, 1 deletion(-) > = > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_= gem.c > index 925c77d..1d3e57e 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3520,7 +3520,6 @@ int i915_gem_object_set_cache_level(struct drm_i915= _gem_object *obj, > * Just set it to the CPU cache for now. > */ > WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU); > - WARN_ON(obj->base.read_domains & ~I915_GEM_DOMAIN_CPU); AFAICS this can only be reached by stolen objs starting in GTT read domain. Normally set_cache_level checks if the object is bound and then calls finish_gtt, and unbind also calls finish_gtt, and GPU domain is handled in a similar way. So I don't see that we can end up here any other way. Based on that, both WARNs seem rather pointless actually. Then again I'm not really sure what we gain from setting stolen objs to GTT read domain initially. The write domain check might make a bit of sense, except for the fact that finish_gtt/gpu clears it just before. Thinking about this stuff a bit, I think I actually came up with a scenario where we would currently fail to invalidate the CPU cache between non-snooped GPU/GTT access and CPU access: 1. make bo non-snooped w/ pin_display=3Dtrue (wd=3D0, rd|=3Dgtt) 2. set to CPU read domain (wd=3D0 rd|=3Dcpu) 3. set to GTT (or GPU) write domain (wd=3Dgtt, rd=3Dgtt) -> CPU cache is st= ale after this point 4. make bo snooped -> pin_display=3Dtrue still so we directly set (wd=3Dcpu= , rd=3Dcpu) 5. set to CPU domain -> CPU cache is still stale > old_read_domains =3D obj->base.read_domains; > old_write_domain =3D obj->base.write_domain; > -- = > 1.8.4.rc2 -- = Ville Syrj=E4l=E4 Intel OTC