| Message ID | 1378852608-30281-17-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 10, 2013 at 07:36:45PM -0300, Rodrigo Vivi wrote: > From: Ben Widawsky <benjamin.widawsky@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@bwidawsk.net> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. Also i915_l3_read() should get the same fix. 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? ... 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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@bwidawsk.net> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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? > > Also i915_l3_read() should get the same fix. Should 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. > > ... > 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
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@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@bwidawsk.net> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ben Widawsky, Intel Open Source Technology Center
On Wed, Sep 11, 2013 at 08:44:21PM +0300, Ville Syrjälä wrote: > 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@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@bwidawsk.net> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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 :) > That's fine. I want it to be as clear as possible (still seems clear to me). What do you want me to write? I'll fix it with the below fixed as well. > > > > > > > > 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. > Now I see what I was missing. Thanks for catching 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. It's not required for userspace to write the whole thing. The operation should be a RMW, except for the initial case where zero-filled is what you want (all 0's should be no remapping). If you want to submit cleanup patches on top of this, I can review them, but I don't feel this suggestion belongs in this patch anyway. > > 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@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
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); i915_gem_l3_remap(drm_dev);