Re: [PATCH 17/25] drm/i915: ValleyView cacheability is different

[Jesse Barnes <jbarnes@virtuousgeek.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3263 bytes --]

On Wed, 21 Mar 2012 22:19:36 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Mar 21, 2012 at 12:48:38PM -0700, Jesse Barnes wrote:
> > The GT can snoop CPU writes, but doesn't snoop into the CPU cache when
> > it does writes, so we can't use the cache bits the same way.
> > 
> > So map the status and pipe control pages as uncached on ValleyView, and
> > only set the pages to cached if we're on a supported platform.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c         |    2 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |   35 ++++++++++++++++++++++++------
> >  2 files changed, 30 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index e4fa294..a636703 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -255,6 +255,7 @@ static const struct intel_device_info intel_valleyview_m_info = {
> >  	.has_bsd_ring = 1,
> >  	.has_blt_ring = 1,
> >  	.is_valleyview = 1,
> > +	.has_llc = 0,
> >  };
> >  
> >  static const struct intel_device_info intel_valleyview_d_info = {
> > @@ -264,6 +265,7 @@ static const struct intel_device_info intel_valleyview_d_info = {
> >  	.has_bsd_ring = 1,
> >  	.has_blt_ring = 1,
> >  	.is_valleyview = 1,
> > +	.has_llc = 0,
> 
> Usually we don't set feature bits to 0, please drop these 2 hunks.
> 
> >  };
> >  
> >  static const struct pci_device_id pciidlist[] = {		/* aka */
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index ca3972f..f52abc4 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -319,6 +319,8 @@ init_pipe_control(struct intel_ring_buffer *ring)
> >  {
> >  	struct pipe_control *pc;
> >  	struct drm_i915_gem_object *obj;
> > +	int cache_level = HAS_LLC(ring->dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
> > +	struct drm_device *dev;
> 
> Nope, that's not gonna work. Afaik we have 3 kinds of snoopable bus access
> from the gpu:
> - llc, i.e. snoopable access for all render operations, support if
>   intel_info->has_llc == 1
> - snoopable access to untiled mem with the blitter, supported on all
>   generations (down to mighty old i81x)
> - snoopable access to the hw status page
> 
> Please clear up the confusion here. Below you also use the IS_VLV macro,
> that seems more appropriate. Also I'm wondering whether this is supposed
> to be fixed in future silicon revisions, if so please mark this as a w/a
> that can be reaped as soon as we don't use this early silicon for testing
> any more.

Yeah I don't like this either.  I have a meeting with the hw guys on
Friday and will try to clear this up.

It may be best to just check for IS_VALLEYVIEW here instead (this is
what I had before, then I decided to get clever so I could squash in
the HAS_LLC change in i915_gem.c).  You're right there are multiple
types of cacheability we need to accommodate.

So maybe HAS_LLC for i915_gem.c and HWS_CACHEABLE or something to
easily identify the retro chips...

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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