[Intel-gfx] [PATCH] drm/i915: prefer vma->size for vma iomap

[Chris Wilson]

Quoting Matthew Auld (2018-03-17 17:42:07)
> It makes more sense to use vma->size, since this determines the number
> of entries we inserted into the vm, while the vma->node.size is the size
> of the vm window we reserved, which may also include padding. At the
> very least this keeps things consistent with the GTT routines.

"Being consistent with the other GTT routines"? Name them, we're talking
about how many pages does the caller expect to be able to access, is the
padding (that they established for the vma) part of the iomap?

So the padding exists to ensure that random tiling beneath the pointer
maps to real pages, but it was established during vma pinning by the
caller.  Being the kernel maybe they do want to access those pages, but
equally what can they do? It's just one page that they could access
directly if so required. I don't see a reason and can't think of an
instance where we do.

> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +-
>  drivers/gpu/drm/i915/i915_vma.c          | 2 +-
>  drivers/gpu/drm/i915/intel_fbdev.c       | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 5757fb7c4b5a..e71e999169cc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -481,7 +481,7 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
>         /* We also want to clear any cached iomaps as they wrap vmap */
>         list_for_each_entry_safe(vma, next,
>                                  &i915->ggtt.base.inactive_list, vm_link) {
> -               unsigned long count = vma->node.size >> PAGE_SHIFT;
> +               unsigned long count = vma->size >> PAGE_SHIFT;
>                 if (vma->iomap && i915_vma_unbind(vma) == 0)
>                         freed_pages += count;
>         }

Ok, as the size the vmap() is obj->base.size.

> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 4bda3bd29bf5..00fb1e5ea495 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -312,7 +312,7 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>         if (ptr == NULL) {
>                 ptr = io_mapping_map_wc(&i915_vm_to_ggtt(vma->vm)->iomap,
>                                         vma->node.start,
> -                                       vma->node.size);
> +                                       vma->size);

Debatable. Even though I can't think of a reason why, I don't see if we
need to be overly restrictive either.

>                 if (ptr == NULL) {
>                         err = -ENOMEM;
>                         goto err;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 65a3313723c9..424800c16dec 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -244,7 +244,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>         info->apertures->ranges[0].size = ggtt->mappable_end;
>  
>         info->fix.smem_start = dev->mode_config.fb_base + i915_ggtt_offset(vma);
> -       info->fix.smem_len = vma->node.size;
> +       info->fix.smem_len = vma->size;

I think this has to be node.size for the overlaps to be checked.

>         vaddr = i915_vma_pin_iomap(vma);
>         if (IS_ERR(vaddr)) {
> @@ -253,7 +253,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>                 goto out_unpin;
>         }
>         info->screen_base = vaddr;
> -       info->screen_size = vma->node.size;
> +       info->screen_size = vma->size;

I don't what this is used for; vma->size is probably correct.
-Chris