Re: [PATCH v2 2/2] drm/nouveau: add GEM_SET_TILING staging ioctl

[Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4636 bytes --]

On Mon, Jun 15, 2015 at 04:15:36PM +0900, Alexandre Courbot wrote:
> On 06/15/2015 04:09 PM, Alexandre Courbot wrote:
> >From: Ari Hirvonen <ahirvonen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> >Add new NOUVEAU_GEM_SET_TILING ioctl to set correct tiling
> >mode for imported dma-bufs. This ioctl is staging for now
> >and enabled with the "staging_tiling" module option.
> 
> Adding Thierry to the conversation since he knows best about exported
> buffers and attributes. I wonder if this would not better be done at the
> time the buffer gets imported. It seems to me that this would be a more
> appropriate way than having an ioctl dedicated to this. Thierry, would you
> have any input based on your experience with tegradrm? In the end, it seems
> like you have settled for a similar ioctl to set the tiling mode of
> displayed buffers.

You can't do this at the time the buffer is imported because the PRIME
API doesn't have a means to communicate this type of meta-data. The data
is also very driver-specific, so you can't easily make it generic enough
to be useful in a generic import API.

That said, I have a couple of other comments. First, the commit message
doesn't mention why this needs to be protected by a module option. Also
I think that if we go for a module option it should be more generic and
provide an umbrella if ever we want to have more code guarded by the
option. It might also be useful to introduce a Kconfig symbol that in
turn depends on the STAGING symbol so that users can't accidentally
enable this.

> >diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
> >index 28860268cf38..45a2c88ebf8e 100644
> >--- a/drm/nouveau/nouveau_drm.c
> >+++ b/drm/nouveau/nouveau_drm.c
> >@@ -75,6 +75,10 @@ MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1
> >  int nouveau_runtime_pm = -1;
> >  module_param_named(runpm, nouveau_runtime_pm, int, 0400);
> >
> >+MODULE_PARM_DESC(staging_tiling, "enable staging GEM_SET_TILING ioctl");
> >+int nouveau_staging_tiling = 0;
> >+module_param_named(staging_tiling, nouveau_staging_tiling, int, 0600);

Perhaps make this a boolean parameter? And like I said, maybe make it
more general and provide a single option for potentially other staging
features.

A word of caution: let's only resort to this if absolutely necessary.
Doing this is going to get messy and if we want this merged I suspect
that we do have userspace that uses this. Hence if ever this gets
modified that userspace will break. Can't we find a way around it?

Note that the DRM_TEGRA_STAGING option is a bit of a special case as
there aren't any real users of it beyond proof of concept code. Nouveau,
on the other hand, has real users that will want to take advantage of
this code. So if we really need to do this, let's come up with a *very*
good rationale.

> >diff --git a/drm/nouveau/nouveau_gem.c b/drm/nouveau/nouveau_gem.c
> >index 0e690bf19fc9..0e69449798aa 100644
> >--- a/drm/nouveau/nouveau_gem.c
> >+++ b/drm/nouveau/nouveau_gem.c
> >@@ -172,6 +172,64 @@ nouveau_gem_object_close(struct drm_gem_object *gem, struct drm_file *file_priv)
> >  	ttm_bo_unreserve(&nvbo->bo);
> >  }
> >
> >+extern int nouveau_staging_tiling;

Put this in nouveau_drm.h along with other external declarations?

> >+int
> >+nouveau_gem_ioctl_set_tiling(struct drm_device *dev, void *data,
> >+			     struct drm_file *file_priv)
> >+{
> >+	struct nouveau_drm *drm = nouveau_drm(dev);
> >+	struct nouveau_cli *cli = nouveau_cli(file_priv);
> >+	struct nvkm_fb *pfb = nvxx_fb(&drm->device);
> >+	struct drm_nouveau_gem_set_tiling *req = data;
> >+	struct drm_gem_object *gem;
> >+	struct nouveau_bo *nvbo;
> >+	struct nvkm_vma *vma;
> >+	int ret = 0;
> >+
> >+	if (!nouveau_staging_tiling)
> >+		return -EINVAL;
> >+
> >+	if (!pfb->memtype_valid(pfb, req->tile_flags)) {
> >+		NV_PRINTK(error, cli, "bad page flags: 0x%08x\n", req->tile_flags);
> >+		return -EINVAL;
> >+	}
> >+
> >+	gem = drm_gem_object_lookup(dev, file_priv, req->handle);
> >+	if (!gem)
> >+		return -ENOENT;
> >+
> >+	nvbo = nouveau_gem_object(gem);
> >+
> >+	/* We can only change tiling on PRIME-imported buffers */
> >+	if (nvbo->bo.type != ttm_bo_type_sg) {
> >+		ret = -EINVAL;
> >+		goto out;
> >+	}

I don't think that's the right way to check for PRIME-imported buffers.
The right check is:

	if (!gem->import_attach) {
		ret = -EINVAL;
		goto out;
	}

The comment above this block should probably also say *why* we can only
change tiling on PRIME-imported buffers.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]