[Intel-gfx] [PATCH 16/19] drm/i915: Fix l3 parity buffer offset

[Ville Syrjälä]

On Wed, Sep 11, 2013 at 10:07:39AM -0700, Ben Widawsky wrote:
> On Wed, Sep 11, 2013 at 02:45:28PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 10, 2013 at 07:36:45PM -0300, Rodrigo Vivi wrote:
> > > From: Ben Widawsky <benjamin.widawsky at intel.com>
> > > 
> > > The buf pointer used during l3_write is just char *, therefore it does
> > > not require the silly /4.
> > > 
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_sysfs.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > > index 05195c0..70de7a9 100644
> > > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > > @@ -184,9 +184,7 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
> > >  	if (temp)
> > >  		dev_priv->l3_parity.remap_info = temp;
> > >  
> > > -	memcpy(dev_priv->l3_parity.remap_info + (offset/4),
> > > -	       buf + (offset/4),
> > > -	       count);
> > > +	memcpy(dev_priv->l3_parity.remap_info + (offset/4), buf, count);
> > 
> > The commit message doesn't really reflect the fact that you completely
> > remove the offset from 'buf', which is actually the correct thing to do,
> > but the commit message should match the patch.
> 
> I don't know, it seems pretty clear to me. Shall I just copy and paste
> what you wrote here?

The commit msg says this:
- buf + offset/4
+ buf + offset

But the patch does this:
- buf + offset/4
+ buf

Doesn't seem very clear to me :)

> 
> > 
> > Also i915_l3_read() should get the same fix.
> 
> Should it?

Yes, 'buf' is just the buffer userspace will eventually get from the
read() syscall. Seeking seeks inside the file, not inside the buffer
userspace passes to read()/write().

Well, actually 'buf' is one of two temp buffers inside the kernel. It
looks like bin.c will copy from that buffer to another temp buffer,
and then from the second temp buffer to userspace. Seems very
inefficient, but perhaps there's a reason for it.

> 
> > 
> > And while on the subject, I don't understand why we're playing weird
> > tricks with the remap_info memory allocation. Ie. why don't we simply
> > do this?
> 
> It was mostly to be able to differentiate NULL vs. 0's. The former being
> no remaps ever, the latter being a cleared remapping. That's why temp is
> used.

Hmm. I don't get it. The only difference between my idea and the current
code is if i915_gpu_idle() fails for the first write. But we could just
do the allocation after we've called i915_gpu_idle() and then there's no
longer any difference.

Also there's no guarantee that userspace would even write the whole
thing. It could just write one byte and then we'd implicitly clear the
rest to 0. Not sure if that's considered OK or not.

Actually 'temp' would make more sense if we'd do the allocation outside
struct_mutex.

> > 
> >  ...
> >  if (!dev_priv->l3_parity.remap_info) {
> >  	dev_priv->l3_parity.remap_info = kzalloc(GEN7_L3LOG_SIZE,
> >  	                                         GFP_KERNEL);
> > 
> >  	if (!dev_priv->l3_parity.remap_info) {
> >  		mutex_unlock(&drm_dev->struct_mutex);
> >  		return -ENOMEM;
> >  	}
> >  }
> >  ...
> > 
> > >  
> > >  	i915_gem_l3_remap(drm_dev);
> > >  
> > > -- 
> > > 1.8.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC