Re: [PATCH] drm/i915: add a LLC feature flag in device description

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 13, 2011 at 09:20:40AM -0800, Eric Anholt wrote:
> On Tue, 13 Dec 2011 17:09:37 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Dec 13, 2011 at 11:05:15AM -0200, Eugeni Dodonov wrote:
> > > From: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > > 
> > > LLC is not SNB-specific, so we should check for it in a more generic way.
> > > 
> > > v2: export LLC support status via debugfs and DRM GETPARAM.
> > > 
> > > Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > 
> > Nice patch and would get an r-b from me safe for the new GETPARAM. I
> > really think we need to export this on a per-bo basis (and with the caveat
> > that the kernel is free to change the caching on every ioctl that uses
> > it). I.e. without forcing userspace to check the caching bits before any
> > bo access I fear that we won't be able to change the kernel's behaviour in
> > this area, which surely results in backwards-compat hell when the first
> > w/a that needs such changes comes around. Hence in its current from
> > 
> > Nacked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > So please drop the GETPARAM. For the per-bo get_cache_flags ioctl there's
> > already a patch by Ben floating around.
> 
> The way the getparam would be useful is that right now we're taking some
> different paths for performance reasons in Mesa on gen6, assuming that
> LLC is present.  Knowing whether or not we expect BOs in general to be
> LLC for performance would be nice for that -- without that, I'll just
> make assumptions based on chipset generation.

Ok, I'll reconsider: In the mesa example (and any other use-case for llc
accelarated up/download) we don't depend upon llc for correctness and
we're using the caching on a newly created buffer, so the per-bo ioctls
aren't much use. So I think the backwards-compat mess is manageable if
people promise to use the HAS_LLC getparam only for such optimizations ...

I still think we want to full get/set_cache_level in additions to this.
But for this patch:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48