Re: [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA

[Luis R. Rodriguez]

On Tue, Apr 19, 2016 at 01:33:58PM +0100, Chris Wilson wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..9ef47329e8ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
>  					    old_write_domain);
>  }
>  
> +static void __i915_vma_iounmap(struct i915_vma *vma)
> +{
> +	if (vma->iomap == NULL)
> +		return;
> +
> +	io_mapping_unmap(vma->iomap);

The NULL check could just be done by io_mapping_unmap() then you
can avoid this in other drivers too.

> +	vma->iomap = NULL;

You added accounting here, by simple int and inc / dec'ing it.
I cannot confirm if it is correctly avoiding races, can you
confirm?

Also you added accounting for the custom vma pinning thing and do
GEM_BUG_ON(vma->pin_count == 0); when you unpin one instance but *you do not*
do something like GEM_BUG_ON(vma->pin_count != 0); when you do the final full
iounmap. That seems rather sloppy.

iomapping stuff has its own custom data structure, why not just use that data
structure instead of the struct i915_vma and generalize this ? Drivers can
be buggy and best if we avoid custom driver accounting and just do it in a neat
generic fashion.

Then other drivers could use this too.

> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..93f54a10042f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
>  	info->fbops = &intelfb_ops;
>  
> +	vma = i915_gem_obj_to_ggtt(obj);
> +
>  	/* setup aperture base/size for vesafb takeover */
>  	info->apertures->ranges[0].base = dev->mode_config.fb_base;
>  	info->apertures->ranges[0].size = ggtt->mappable_end;
>  
> -	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> -	info->fix.smem_len = size;
> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> +	info->fix.smem_len = vma->node.size;
>  
> -	info->screen_base =
> -		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> -			   size);
> -	if (!info->screen_base) {
> +	vaddr = i915_vma_pin_iomap(vma);
> +	if (IS_ERR(vaddr)) {
>  		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> -		ret = -ENOSPC;
> +		ret = PTR_ERR(vaddr);
>  		goto out_destroy_fbi;
>  	}
> -	info->screen_size = size;
> +	info->screen_base = vaddr;
> +	info->screen_size = vma->node.size;

some framebuffer drivers tend to use a generic start address of
iinfo->fix.smem_start and a length of info->fix.smem_len, this
driver sets the smem_start above, but its different than what
gets ioremap for a start address:

+               ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+                                       vma->node.start,
+                                       vma->node.size);

fix.smem_start is :


> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;

The smem_len matches though. Can you clarify if its correct for
the io_mapping_map_wc() should not be using info->fix.smem_start
(which is dev->mode_config.fb_base + vma->node.start)?

Reason I ask is since I noticed a while ago a lot of drivers
were using info->fix.smem_start and info->fix.smem_len consistently
for their ioremap'd areas it might make sense instead to let the
internal framebuffer (register_framebuffer()) optionally manage the
ioremap_wc() for drivers, given that this is pretty generic stuff.

  Luis