Message ID | 20170112213533.16614-6-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 12/01/2017 21:35, Chris Wilson wrote: > Save a lot of characters by making the union anonymous, with the > side-effect of ignoring unset bits when comparing views. Side-effect is not happening in this patch. Always tricky what to do with commit messages for split patches. :) Maybe just rewrite the commit message. > > v2: Roll up the memcmps back into one. > v3: And split again as Ville points out we can't trust the compiler. Not sure what is split, memcmps? But there is only one. Needs v4? Or I am missing something? > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 20 ++++++++++---------- > drivers/gpu/drm/i915/i915_gem.c | 8 ++++---- > drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++----- > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- > drivers/gpu/drm/i915/i915_vma.c | 9 ++++----- > drivers/gpu/drm/i915/i915_vma.h | 4 +++- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 7 files changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index e367f06f5883..da13c4c3aa6b 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -167,20 +167,20 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) > > case I915_GGTT_VIEW_PARTIAL: > seq_printf(m, ", partial [%08llx+%x]", > - vma->ggtt_view.params.partial.offset << PAGE_SHIFT, > - vma->ggtt_view.params.partial.size << PAGE_SHIFT); > + vma->ggtt_view.partial.offset << PAGE_SHIFT, > + vma->ggtt_view.partial.size << PAGE_SHIFT); > break; > > case I915_GGTT_VIEW_ROTATED: > seq_printf(m, ", rotated [(%ux%u, stride=%u, offset=%u), (%ux%u, stride=%u, offset=%u)]", > - vma->ggtt_view.params.rotated.plane[0].width, > - vma->ggtt_view.params.rotated.plane[0].height, > - vma->ggtt_view.params.rotated.plane[0].stride, > - vma->ggtt_view.params.rotated.plane[0].offset, > - vma->ggtt_view.params.rotated.plane[1].width, > - vma->ggtt_view.params.rotated.plane[1].height, > - vma->ggtt_view.params.rotated.plane[1].stride, > - vma->ggtt_view.params.rotated.plane[1].offset); > + vma->ggtt_view.rotated.plane[0].width, > + vma->ggtt_view.rotated.plane[0].height, > + vma->ggtt_view.rotated.plane[0].stride, > + vma->ggtt_view.rotated.plane[0].offset, > + vma->ggtt_view.rotated.plane[1].width, > + vma->ggtt_view.rotated.plane[1].height, > + vma->ggtt_view.rotated.plane[1].stride, > + vma->ggtt_view.rotated.plane[1].offset); > break; > > default: > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f034d8d2dd4c..d8622fd23f5d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1760,10 +1760,10 @@ compute_partial_view(struct drm_i915_gem_object *obj, > chunk = roundup(chunk, tile_row_pages(obj)); > > view.type = I915_GGTT_VIEW_PARTIAL; > - view.params.partial.offset = rounddown(page_offset, chunk); > - view.params.partial.size = > + view.partial.offset = rounddown(page_offset, chunk); > + view.partial.size = > min_t(unsigned int, chunk, > - (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset); > + (obj->base.size >> PAGE_SHIFT) - view.partial.offset); > > /* If the partial covers the entire object, just create a normal VMA. */ > if (chunk >= obj->base.size >> PAGE_SHIFT) > @@ -1879,7 +1879,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf) > > /* Finally, remap it using the new GTT offset */ > ret = remap_io_mapping(area, > - area->vm_start + (vma->ggtt_view.params.partial.offset << PAGE_SHIFT), > + area->vm_start + (vma->ggtt_view.partial.offset << PAGE_SHIFT), > (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT, > min_t(u64, vma->size, area->vm_end - area->vm_start), > &ggtt->mappable); > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 6d2ff20ec973..06cfd6951a5e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3490,7 +3490,7 @@ intel_partial_pages(const struct i915_ggtt_view *view, > { > struct sg_table *st; > struct scatterlist *sg, *iter; > - unsigned int count = view->params.partial.size; > + unsigned int count = view->partial.size; > unsigned int offset; > int ret = -ENOMEM; > > @@ -3502,9 +3502,7 @@ intel_partial_pages(const struct i915_ggtt_view *view, > if (ret) > goto err_sg_alloc; > > - iter = i915_gem_object_get_sg(obj, > - view->params.partial.offset, > - &offset); > + iter = i915_gem_object_get_sg(obj, view->partial.offset, &offset); > GEM_BUG_ON(!iter); > > sg = st->sgl; > @@ -3556,7 +3554,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma) > vma->pages = vma->obj->mm.pages; > else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED) > vma->pages = > - intel_rotate_fb_obj_pages(&vma->ggtt_view.params.rotated, vma->obj); > + intel_rotate_fb_obj_pages(&vma->ggtt_view.rotated, > + vma->obj); > else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL) > vma->pages = intel_partial_pages(&vma->ggtt_view, vma->obj); > else > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 20c03c30ce4e..560fd8ddaf2c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -179,7 +179,7 @@ struct i915_ggtt_view { > /* Members need to contain no holes/padding */ > struct intel_partial_info partial; > struct intel_rotation_info rotated; > - } params; > + }; > }; > > extern const struct i915_ggtt_view i915_ggtt_view_normal; > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index b74eeb73ae41..e6c339b0ea37 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -97,15 +97,14 @@ __i915_vma_create(struct drm_i915_gem_object *obj, > vma->ggtt_view = *view; > if (view->type == I915_GGTT_VIEW_PARTIAL) { > GEM_BUG_ON(range_overflows_t(u64, > - view->params.partial.offset, > - view->params.partial.size, > + view->partial.offset, > + view->partial.size, > obj->base.size >> PAGE_SHIFT)); > - vma->size = view->params.partial.size; > + vma->size = view->partial.size; > vma->size <<= PAGE_SHIFT; > GEM_BUG_ON(vma->size >= obj->base.size); > } else if (view->type == I915_GGTT_VIEW_ROTATED) { > - vma->size = > - intel_rotation_info_size(&view->params.rotated); > + vma->size = intel_rotation_info_size(&view->rotated); > vma->size <<= PAGE_SHIFT; > } > } > diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h > index 19f049cef9e3..9d6913b10f30 100644 > --- a/drivers/gpu/drm/i915/i915_vma.h > +++ b/drivers/gpu/drm/i915/i915_vma.h > @@ -209,8 +209,10 @@ i915_vma_compare(struct i915_vma *vma, > return cmp; > > BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL == I915_GGTT_VIEW_ROTATED); > + BUILD_BUG_ON(offsetof(typeof(*view), rotated) != > + offsetof(typeof(*view), partial)); Nice! > > - return memcmp(&vma->ggtt_view.params, &view->params, view->type); > + return memcmp(&vma->ggtt_view.partial, &view->partial, view->type); Here you could expand on the comment from the earlier patch explaining the above BUILD_BUG_ON in words. > } > > int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fd5fbc83c69e..f4be20f0198a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2139,7 +2139,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, > { > if (drm_rotation_90_or_270(rotation)) { > *view = i915_ggtt_view_rotated; > - view->params.rotated = to_intel_framebuffer(fb)->rot_info; > + view->rotated = to_intel_framebuffer(fb)->rot_info; > } else { > *view = i915_ggtt_view_normal; > } > Regards, Tvrtko
On Fri, Jan 13, 2017 at 09:04:46AM +0000, Tvrtko Ursulin wrote: > > On 12/01/2017 21:35, Chris Wilson wrote: > >Save a lot of characters by making the union anonymous, with the > >side-effect of ignoring unset bits when comparing views. > > Side-effect is not happening in this patch. Always tricky what to do > with commit messages for split patches. :) Maybe just rewrite the > commit message. > > > > >v2: Roll up the memcmps back into one. > >v3: And split again as Ville points out we can't trust the compiler. > > Not sure what is split, memcmps? But there is only one. Needs v4? Or > I am missing something? The changelog just needs rewriting so that the preconditions for this patch (wanting to able to use a single memcmp and so no uninitialised bits within the memcmp) are clear. In a sense, it's why I liked having this in one patch, since the patches are not really standalone and only make sense together :| Revert one, we have to revert them all. -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index e367f06f5883..da13c4c3aa6b 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -167,20 +167,20 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) case I915_GGTT_VIEW_PARTIAL: seq_printf(m, ", partial [%08llx+%x]", - vma->ggtt_view.params.partial.offset << PAGE_SHIFT, - vma->ggtt_view.params.partial.size << PAGE_SHIFT); + vma->ggtt_view.partial.offset << PAGE_SHIFT, + vma->ggtt_view.partial.size << PAGE_SHIFT); break; case I915_GGTT_VIEW_ROTATED: seq_printf(m, ", rotated [(%ux%u, stride=%u, offset=%u), (%ux%u, stride=%u, offset=%u)]", - vma->ggtt_view.params.rotated.plane[0].width, - vma->ggtt_view.params.rotated.plane[0].height, - vma->ggtt_view.params.rotated.plane[0].stride, - vma->ggtt_view.params.rotated.plane[0].offset, - vma->ggtt_view.params.rotated.plane[1].width, - vma->ggtt_view.params.rotated.plane[1].height, - vma->ggtt_view.params.rotated.plane[1].stride, - vma->ggtt_view.params.rotated.plane[1].offset); + vma->ggtt_view.rotated.plane[0].width, + vma->ggtt_view.rotated.plane[0].height, + vma->ggtt_view.rotated.plane[0].stride, + vma->ggtt_view.rotated.plane[0].offset, + vma->ggtt_view.rotated.plane[1].width, + vma->ggtt_view.rotated.plane[1].height, + vma->ggtt_view.rotated.plane[1].stride, + vma->ggtt_view.rotated.plane[1].offset); break; default: diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f034d8d2dd4c..d8622fd23f5d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1760,10 +1760,10 @@ compute_partial_view(struct drm_i915_gem_object *obj, chunk = roundup(chunk, tile_row_pages(obj)); view.type = I915_GGTT_VIEW_PARTIAL; - view.params.partial.offset = rounddown(page_offset, chunk); - view.params.partial.size = + view.partial.offset = rounddown(page_offset, chunk); + view.partial.size = min_t(unsigned int, chunk, - (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset); + (obj->base.size >> PAGE_SHIFT) - view.partial.offset); /* If the partial covers the entire object, just create a normal VMA. */ if (chunk >= obj->base.size >> PAGE_SHIFT) @@ -1879,7 +1879,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf) /* Finally, remap it using the new GTT offset */ ret = remap_io_mapping(area, - area->vm_start + (vma->ggtt_view.params.partial.offset << PAGE_SHIFT), + area->vm_start + (vma->ggtt_view.partial.offset << PAGE_SHIFT), (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT, min_t(u64, vma->size, area->vm_end - area->vm_start), &ggtt->mappable); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 6d2ff20ec973..06cfd6951a5e 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3490,7 +3490,7 @@ intel_partial_pages(const struct i915_ggtt_view *view, { struct sg_table *st; struct scatterlist *sg, *iter; - unsigned int count = view->params.partial.size; + unsigned int count = view->partial.size; unsigned int offset; int ret = -ENOMEM; @@ -3502,9 +3502,7 @@ intel_partial_pages(const struct i915_ggtt_view *view, if (ret) goto err_sg_alloc; - iter = i915_gem_object_get_sg(obj, - view->params.partial.offset, - &offset); + iter = i915_gem_object_get_sg(obj, view->partial.offset, &offset); GEM_BUG_ON(!iter); sg = st->sgl; @@ -3556,7 +3554,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma) vma->pages = vma->obj->mm.pages; else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED) vma->pages = - intel_rotate_fb_obj_pages(&vma->ggtt_view.params.rotated, vma->obj); + intel_rotate_fb_obj_pages(&vma->ggtt_view.rotated, + vma->obj); else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL) vma->pages = intel_partial_pages(&vma->ggtt_view, vma->obj); else diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 20c03c30ce4e..560fd8ddaf2c 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -179,7 +179,7 @@ struct i915_ggtt_view { /* Members need to contain no holes/padding */ struct intel_partial_info partial; struct intel_rotation_info rotated; - } params; + }; }; extern const struct i915_ggtt_view i915_ggtt_view_normal; diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index b74eeb73ae41..e6c339b0ea37 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -97,15 +97,14 @@ __i915_vma_create(struct drm_i915_gem_object *obj, vma->ggtt_view = *view; if (view->type == I915_GGTT_VIEW_PARTIAL) { GEM_BUG_ON(range_overflows_t(u64, - view->params.partial.offset, - view->params.partial.size, + view->partial.offset, + view->partial.size, obj->base.size >> PAGE_SHIFT)); - vma->size = view->params.partial.size; + vma->size = view->partial.size; vma->size <<= PAGE_SHIFT; GEM_BUG_ON(vma->size >= obj->base.size); } else if (view->type == I915_GGTT_VIEW_ROTATED) { - vma->size = - intel_rotation_info_size(&view->params.rotated); + vma->size = intel_rotation_info_size(&view->rotated); vma->size <<= PAGE_SHIFT; } } diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 19f049cef9e3..9d6913b10f30 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -209,8 +209,10 @@ i915_vma_compare(struct i915_vma *vma, return cmp; BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL == I915_GGTT_VIEW_ROTATED); + BUILD_BUG_ON(offsetof(typeof(*view), rotated) != + offsetof(typeof(*view), partial)); - return memcmp(&vma->ggtt_view.params, &view->params, view->type); + return memcmp(&vma->ggtt_view.partial, &view->partial, view->type); } int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fd5fbc83c69e..f4be20f0198a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2139,7 +2139,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, { if (drm_rotation_90_or_270(rotation)) { *view = i915_ggtt_view_rotated; - view->params.rotated = to_intel_framebuffer(fb)->rot_info; + view->rotated = to_intel_framebuffer(fb)->rot_info; } else { *view = i915_ggtt_view_normal; }
Save a lot of characters by making the union anonymous, with the side-effect of ignoring unset bits when comparing views. v2: Roll up the memcmps back into one. v3: And split again as Ville points out we can't trust the compiler. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_debugfs.c | 20 ++++++++++---------- drivers/gpu/drm/i915/i915_gem.c | 8 ++++---- drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++----- drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- drivers/gpu/drm/i915/i915_vma.c | 9 ++++----- drivers/gpu/drm/i915/i915_vma.h | 4 +++- drivers/gpu/drm/i915/intel_display.c | 2 +- 7 files changed, 27 insertions(+), 27 deletions(-)