Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 11 Dec 2019 13:38:35 +0100
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Gerd Hoffmann <kraxel@redhat.com>
Cc:     dri-devel@lists.freedesktop.org, tzimmermann@suse.de,
        gurchetansingh@chromium.org,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/4] drm: add pgprot callback to drm_gem_object_funcs
Message-ID: <20191211123835.GZ624164@phenom.ffwll.local>
Mail-Followup-To: Gerd Hoffmann <kraxel@redhat.com>,
        dri-devel@lists.freedesktop.org, tzimmermann@suse.de,
        gurchetansingh@chromium.org,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>, David Airlie <airlied@linux.ie>,
        open list <linux-kernel@vger.kernel.org>
References: <20191211121957.18637-1-kraxel@redhat.com>
 <20191211121957.18637-2-kraxel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191211121957.18637-2-kraxel@redhat.com>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Dec 11, 2019 at 01:19:53PM +0100, Gerd Hoffmann wrote:
> The callback allows drivers and helpers to tweak pgprot for mappings.
> This is especially helpful when using shmem helpers.  It allows drivers
> to switch mappings from writecombine (default) to something else (cached
> for example) on a per-object base without having to supply their own
> mmap() and vmap() functions.
> 
> The patch also adds two implementations for the callback, for cached and
> writecombine mappings, and the drm_gem_pgprot() function to update
> pgprot for a given object, using the new &drm_gem_object_funcs.pgprot
> callback if available.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem.h     | 15 +++++++++++++
>  drivers/gpu/drm/drm_gem.c | 46 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 0b375069cd48..5beef7226e69 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -163,6 +163,17 @@ struct drm_gem_object_funcs {
>  	 */
>  	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
>  
> +	/**
> +	 * @pgprot:
> +	 *
> +	 * Tweak pgprot as needed, typically used to set cache bits.
> +	 *
> +	 * This callback is optional.
> +	 *
> +	 * If unset drm_gem_pgprot_wc() will be used.
> +	 */
> +	pgprot_t (*pgprot)(struct drm_gem_object *obj, pgprot_t prot);

I kinda prefer v1, mostly because this is a huge can of worms, and solving
this properly is going to be real hard (and will necessarily involve
dma-buf and dma-api and probably more). Charging ahead here just risks
that we dig ourselves into a corner. You're v1 is maybe not the most
clean, but just a few code bits here&there should be more flexible and
easier to hack on and experiment around with.
-Daniel

> +
>  	/**
>  	 * @vm_ops:
>  	 *
> @@ -350,6 +361,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  		     struct vm_area_struct *vma);
>  int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>  
> +pgprot_t drm_gem_pgprot_cached(struct drm_gem_object *obj, pgprot_t prot);
> +pgprot_t drm_gem_pgprot_wc(struct drm_gem_object *obj, pgprot_t prot);
> +pgprot_t drm_gem_pgprot(struct drm_gem_object *obj, pgprot_t prot);
> +
>  /**
>   * drm_gem_object_get - acquire a GEM buffer object reference
>   * @obj: GEM buffer object
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 56f42e0f2584..1c468fe8e342 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1119,7 +1119,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  			return -EINVAL;
>  
>  		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> -		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +		vma->vm_page_prot = drm_gem_pgprot(obj, vma->vm_page_prot);
>  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>  	}
>  
> @@ -1210,6 +1211,49 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  }
>  EXPORT_SYMBOL(drm_gem_mmap);
>  
> +/**
> + * drm_gem_mmap - update pgprot for objects needing a cachable mapping.
> + * @obj: the GEM object.
> + * @prot: page attributes.
> + *
> + * This function can be used as &drm_gem_object_funcs.pgprot callback.
> + */
> +pgprot_t drm_gem_pgprot_cached(struct drm_gem_object *obj, pgprot_t prot)
> +{
> +	return prot;
> +}
> +EXPORT_SYMBOL(drm_gem_pgprot_cached);
> +
> +/**
> + * drm_gem_mmap - update pgprot for objects needing a wc mapping.
> + * @obj: the GEM object.
> + * @prot: page attributes.
> + *
> + * This function can be used as &drm_gem_object_funcs.pgprot callback.
> + */
> +pgprot_t drm_gem_pgprot_wc(struct drm_gem_object *obj, pgprot_t prot)
> +{
> +	return pgprot_writecombine(prot);
> +}
> +EXPORT_SYMBOL(drm_gem_pgprot_wc);
> +
> +/**
> + * drm_gem_mmap - update pgprot for a given gem object.
> + * @obj: the GEM object.
> + * @prot: page attributes.
> + *
> + * This function updates pgprot according to the needs of the given
> + * object.  If present &drm_gem_object_funcs.pgprot callback will be
> + * used, otherwise drm_gem_pgprot_wc() is called.
> + */
> +pgprot_t drm_gem_pgprot(struct drm_gem_object *obj, pgprot_t prot)
> +{
> +	if (obj->funcs->pgprot)
> +		return obj->funcs->pgprot(obj, prot);
> +	return drm_gem_pgprot_wc(obj, prot);
> +}
> +EXPORT_SYMBOL(drm_gem_pgprot);
> +
>  void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>  			const struct drm_gem_object *obj)
>  {
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch