[PATCH v2 0/2] drm/nouveau: option for staging ioctls and new GEM_SET_TILING ioctl

[PATCH v2 0/2] drm/nouveau: option for staging ioctls and new GEM_SET_TILING ioctl

[Alexandre Courbot]

Second version of this patchset addressing Ben's comments and fixing a few
extra things.

This patchset proposes to introduce a "staging" module option to dynamically
enable features (mostly ioctls) that are merged but may be refined before
they are declared "stable". The second patch illustrates the use of this
staging option with the SET_TILING ioctl, which can be used to specify the
tiling options of a PRIME-imported buffer.

The staging parameter will allow us (particularly, us at NVIDIA) to experiment
more freely with new features and avoid carrying out-of-tree patches for long
periods of time. To prevent abuse, the number of staging ioctls is limited to 8
(range 0x98 to 0xa0) that are to be recycled as staging ioctls become stable and
are assigned a final number.

Changes since v1:
- Use one module option per staging ioctl
- Only allow GEM_SET_TILING to be called on dma-buf imported buffers
- Move the code setting nvkm_mem::memtype into its own function to avoid
  duplicating code

Alexandre Courbot (1):
  drm/nouveau: placeholders for staging ioctls

Ari Hirvonen (1):
  drm/nouveau: add GEM_SET_TILING staging ioctl

 drm/nouveau/nouveau_bo.c           | 18 ++++++++++++
 drm/nouveau/nouveau_bo.h           |  2 ++
 drm/nouveau/nouveau_drm.c          |  7 +++++
 drm/nouveau/nouveau_gem.c          | 58 ++++++++++++++++++++++++++++++++++++++
 drm/nouveau/nouveau_gem.h          |  2 ++
 drm/nouveau/nouveau_ttm.c          | 13 +--------
 drm/nouveau/uapi/drm/nouveau_drm.h | 11 ++++++++
 7 files changed, 99 insertions(+), 12 deletions(-)

-- 
2.4.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm/nouveau: placeholders for staging ioctls

[Alexandre Courbot]

Reserve the 8 highest ioctls for "staging" features, i.e. ioctls that
may change or be dropped altogether and that thus should not be given a
definite number. This will allow us to experiment with experimental APIs
for a while before setting them in stone.

Staging ioctls are not expected to stay there for a long time, so we
limit their number. Besides, each staging ioctls needs to be explicitly
enabled through its own module option.

Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drm/nouveau/nouveau_drm.c          | 1 +
 drm/nouveau/uapi/drm/nouveau_drm.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
index 63f1d39e8358..28860268cf38 100644
--- a/drm/nouveau/nouveau_drm.c
+++ b/drm/nouveau/nouveau_drm.c
@@ -896,6 +896,7 @@ nouveau_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_PREP, nouveau_gem_ioctl_cpu_prep, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_FINI, nouveau_gem_ioctl_cpu_fini, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_INFO, nouveau_gem_ioctl_info, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
+	/* Staging ioctls */
 };
 
 long
diff --git a/drm/nouveau/uapi/drm/nouveau_drm.h b/drm/nouveau/uapi/drm/nouveau_drm.h
index 5507eead5863..4e7e21f41b5c 100644
--- a/drm/nouveau/uapi/drm/nouveau_drm.h
+++ b/drm/nouveau/uapi/drm/nouveau_drm.h
@@ -140,11 +140,14 @@ struct drm_nouveau_gem_cpu_fini {
 #define DRM_NOUVEAU_GEM_CPU_PREP       0x42
 #define DRM_NOUVEAU_GEM_CPU_FINI       0x43
 #define DRM_NOUVEAU_GEM_INFO           0x44
+/* range 0x98..DRM_COMMAND_END (8 entries) is reserved for staging, unstable ioctls */
+#define DRM_NOUVEAU_STAGING_IOCTL      0x58
 
 #define DRM_IOCTL_NOUVEAU_GEM_NEW            DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_NEW, struct drm_nouveau_gem_new)
 #define DRM_IOCTL_NOUVEAU_GEM_PUSHBUF        DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_PUSHBUF, struct drm_nouveau_gem_pushbuf)
 #define DRM_IOCTL_NOUVEAU_GEM_CPU_PREP       DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_PREP, struct drm_nouveau_gem_cpu_prep)
 #define DRM_IOCTL_NOUVEAU_GEM_CPU_FINI       DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_FINI, struct drm_nouveau_gem_cpu_fini)
 #define DRM_IOCTL_NOUVEAU_GEM_INFO           DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_INFO, struct drm_nouveau_gem_info)
+/* staging ioctls */
 
 #endif /* __NOUVEAU_DRM_H__ */
-- 
2.4.2

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

[Alexandre Courbot]

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.

Signed-off-by: Ari Hirvonen <ahirvonen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org: carry upstream, many fixes]
Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drm/nouveau/nouveau_bo.c           | 18 ++++++++++++
 drm/nouveau/nouveau_bo.h           |  2 ++
 drm/nouveau/nouveau_drm.c          |  6 ++++
 drm/nouveau/nouveau_gem.c          | 58 ++++++++++++++++++++++++++++++++++++++
 drm/nouveau/nouveau_gem.h          |  2 ++
 drm/nouveau/nouveau_ttm.c          | 13 +--------
 drm/nouveau/uapi/drm/nouveau_drm.h |  8 ++++++
 7 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/drm/nouveau/nouveau_bo.c b/drm/nouveau/nouveau_bo.c
index 6edcce1658b7..2a2ebbeb4fc0 100644
--- a/drm/nouveau/nouveau_bo.c
+++ b/drm/nouveau/nouveau_bo.c
@@ -178,6 +178,24 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
 	*size = roundup(*size, PAGE_SIZE);
 }
 
+void
+nouveau_bo_update_tiling(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
+			 struct nvkm_mem *mem)
+{
+	switch (drm->device.info.family) {
+	case NV_DEVICE_INFO_V0_TESLA:
+		if (drm->device.info.chipset != 0x50)
+			mem->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
+		break;
+	case NV_DEVICE_INFO_V0_FERMI:
+	case NV_DEVICE_INFO_V0_KEPLER:
+		mem->memtype = (nvbo->tile_flags & 0xff00) >> 8;
+		break;
+	default:
+		break;
+	}
+}
+
 int
 nouveau_bo_new(struct drm_device *dev, int size, int align,
 	       uint32_t flags, uint32_t tile_mode, uint32_t tile_flags,
diff --git a/drm/nouveau/nouveau_bo.h b/drm/nouveau/nouveau_bo.h
index e42360983229..87d07e3533eb 100644
--- a/drm/nouveau/nouveau_bo.h
+++ b/drm/nouveau/nouveau_bo.h
@@ -69,6 +69,8 @@ nouveau_bo_ref(struct nouveau_bo *ref, struct nouveau_bo **pnvbo)
 extern struct ttm_bo_driver nouveau_bo_driver;
 
 void nouveau_bo_move_init(struct nouveau_drm *);
+void nouveau_bo_update_tiling(struct nouveau_drm *, struct nouveau_bo *,
+			      struct nvkm_mem *);
 int  nouveau_bo_new(struct drm_device *, int size, int align, u32 flags,
 		    u32 tile_mode, u32 tile_flags, struct sg_table *sg,
 		    struct reservation_object *robj,
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);
+
 static struct drm_driver driver_stub;
 static struct drm_driver driver_pci;
 static struct drm_driver driver_platform;
@@ -897,6 +901,7 @@ nouveau_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_FINI, nouveau_gem_ioctl_cpu_fini, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_INFO, nouveau_gem_ioctl_info, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
 	/* Staging ioctls */
+	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_SET_TILING, nouveau_gem_ioctl_set_tiling, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
 };
 
 long
@@ -1029,6 +1034,7 @@ static void nouveau_display_options(void)
 	DRM_DEBUG_DRIVER("... runpm        : %d\n", nouveau_runtime_pm);
 	DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf);
 	DRM_DEBUG_DRIVER("... pstate       : %d\n", nouveau_pstate);
+	DRM_DEBUG_DRIVER("... staging_tiling: %d\n", nouveau_staging_tiling);
 }
 
 static const struct dev_pm_ops nouveau_pm_ops = {
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;
+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;
+	}
+
+	if (nvbo->tile_mode != req->tile_mode ||
+	    nvbo->tile_flags != req->tile_flags) {
+		ret = ttm_bo_reserve(&nvbo->bo, false, false, false, NULL);
+		if (ret)
+			goto out;
+
+		nvbo->tile_mode = req->tile_mode;
+		nvbo->tile_flags = req->tile_flags;
+
+		nouveau_bo_update_tiling(drm, nvbo, nvbo->bo.mem.mm_node);
+
+		/* remap over existing mapping with new tile parameters */
+		vma = nouveau_bo_vma_find(nvbo, cli->vm);
+		if (vma)
+			nvkm_vm_map(vma, nvbo->bo.mem.mm_node);
+
+		ttm_bo_unreserve(&nvbo->bo);
+	}
+
+out:
+	drm_gem_object_unreference_unlocked(gem);
+	return ret;
+}
+
 int
 nouveau_gem_new(struct drm_device *dev, int size, int align, uint32_t domain,
 		uint32_t tile_mode, uint32_t tile_flags,
diff --git a/drm/nouveau/nouveau_gem.h b/drm/nouveau/nouveau_gem.h
index e4049faca780..56e741d98bcd 100644
--- a/drm/nouveau/nouveau_gem.h
+++ b/drm/nouveau/nouveau_gem.h
@@ -23,6 +23,8 @@ extern void nouveau_gem_object_del(struct drm_gem_object *);
 extern int nouveau_gem_object_open(struct drm_gem_object *, struct drm_file *);
 extern void nouveau_gem_object_close(struct drm_gem_object *,
 				     struct drm_file *);
+extern int nouveau_gem_ioctl_set_tiling(struct drm_device *, void *,
+					struct drm_file *);
 extern int nouveau_gem_ioctl_new(struct drm_device *, void *,
 				 struct drm_file *);
 extern int nouveau_gem_ioctl_pushbuf(struct drm_device *, void *,
diff --git a/drm/nouveau/nouveau_ttm.c b/drm/nouveau/nouveau_ttm.c
index 18f449715788..d680153db6e5 100644
--- a/drm/nouveau/nouveau_ttm.c
+++ b/drm/nouveau/nouveau_ttm.c
@@ -174,18 +174,7 @@ nouveau_gart_manager_new(struct ttm_mem_type_manager *man,
 
 	node->page_shift = 12;
 
-	switch (drm->device.info.family) {
-	case NV_DEVICE_INFO_V0_TESLA:
-		if (drm->device.info.chipset != 0x50)
-			node->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
-		break;
-	case NV_DEVICE_INFO_V0_FERMI:
-	case NV_DEVICE_INFO_V0_KEPLER:
-		node->memtype = (nvbo->tile_flags & 0xff00) >> 8;
-		break;
-	default:
-		break;
-	}
+	nouveau_bo_update_tiling(drm, nvbo, node);
 
 	mem->mm_node = node;
 	mem->start   = 0;
diff --git a/drm/nouveau/uapi/drm/nouveau_drm.h b/drm/nouveau/uapi/drm/nouveau_drm.h
index 4e7e21f41b5c..8f10b16b1473 100644
--- a/drm/nouveau/uapi/drm/nouveau_drm.h
+++ b/drm/nouveau/uapi/drm/nouveau_drm.h
@@ -64,6 +64,12 @@ struct drm_nouveau_gem_new {
 	uint32_t align;
 };
 
+struct drm_nouveau_gem_set_tiling {
+	uint32_t handle;
+	uint32_t tile_mode;
+	uint32_t tile_flags;
+};
+
 #define NOUVEAU_GEM_MAX_BUFFERS 1024
 struct drm_nouveau_gem_pushbuf_bo_presumed {
 	uint32_t valid;
@@ -142,6 +148,7 @@ struct drm_nouveau_gem_cpu_fini {
 #define DRM_NOUVEAU_GEM_INFO           0x44
 /* range 0x98..DRM_COMMAND_END (8 entries) is reserved for staging, unstable ioctls */
 #define DRM_NOUVEAU_STAGING_IOCTL      0x58
+#define DRM_NOUVEAU_GEM_SET_TILING     (DRM_NOUVEAU_STAGING_IOCTL + 0x0)
 
 #define DRM_IOCTL_NOUVEAU_GEM_NEW            DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_NEW, struct drm_nouveau_gem_new)
 #define DRM_IOCTL_NOUVEAU_GEM_PUSHBUF        DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_PUSHBUF, struct drm_nouveau_gem_pushbuf)
@@ -149,5 +156,6 @@ struct drm_nouveau_gem_cpu_fini {
 #define DRM_IOCTL_NOUVEAU_GEM_CPU_FINI       DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_FINI, struct drm_nouveau_gem_cpu_fini)
 #define DRM_IOCTL_NOUVEAU_GEM_INFO           DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_INFO, struct drm_nouveau_gem_info)
 /* staging ioctls */
+#define DRM_IOCTL_NOUVEAU_GEM_SET_TILING     DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_SET_TILING, struct drm_nouveau_gem_set_tiling)
 
 #endif /* __NOUVEAU_DRM_H__ */
-- 
2.4.2

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

[Alexandre Courbot]

On 06/15/2015 04:09 PM, Alexandre Courbot wrote:
> From: Ari Hirvonen <ahirvonen@nvidia.com>
>
> 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.

>
> Signed-off-by: Ari Hirvonen <ahirvonen@nvidia.com>
> [acourbot@nvidia.com: carry upstream, many fixes]
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>   drm/nouveau/nouveau_bo.c           | 18 ++++++++++++
>   drm/nouveau/nouveau_bo.h           |  2 ++
>   drm/nouveau/nouveau_drm.c          |  6 ++++
>   drm/nouveau/nouveau_gem.c          | 58 ++++++++++++++++++++++++++++++++++++++
>   drm/nouveau/nouveau_gem.h          |  2 ++
>   drm/nouveau/nouveau_ttm.c          | 13 +--------
>   drm/nouveau/uapi/drm/nouveau_drm.h |  8 ++++++
>   7 files changed, 95 insertions(+), 12 deletions(-)
>
> diff --git a/drm/nouveau/nouveau_bo.c b/drm/nouveau/nouveau_bo.c
> index 6edcce1658b7..2a2ebbeb4fc0 100644
> --- a/drm/nouveau/nouveau_bo.c
> +++ b/drm/nouveau/nouveau_bo.c
> @@ -178,6 +178,24 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
>   	*size = roundup(*size, PAGE_SIZE);
>   }
>
> +void
> +nouveau_bo_update_tiling(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
> +			 struct nvkm_mem *mem)
> +{
> +	switch (drm->device.info.family) {
> +	case NV_DEVICE_INFO_V0_TESLA:
> +		if (drm->device.info.chipset != 0x50)
> +			mem->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
> +		break;
> +	case NV_DEVICE_INFO_V0_FERMI:
> +	case NV_DEVICE_INFO_V0_KEPLER:
> +		mem->memtype = (nvbo->tile_flags & 0xff00) >> 8;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>   int
>   nouveau_bo_new(struct drm_device *dev, int size, int align,
>   	       uint32_t flags, uint32_t tile_mode, uint32_t tile_flags,
> diff --git a/drm/nouveau/nouveau_bo.h b/drm/nouveau/nouveau_bo.h
> index e42360983229..87d07e3533eb 100644
> --- a/drm/nouveau/nouveau_bo.h
> +++ b/drm/nouveau/nouveau_bo.h
> @@ -69,6 +69,8 @@ nouveau_bo_ref(struct nouveau_bo *ref, struct nouveau_bo **pnvbo)
>   extern struct ttm_bo_driver nouveau_bo_driver;
>
>   void nouveau_bo_move_init(struct nouveau_drm *);
> +void nouveau_bo_update_tiling(struct nouveau_drm *, struct nouveau_bo *,
> +			      struct nvkm_mem *);
>   int  nouveau_bo_new(struct drm_device *, int size, int align, u32 flags,
>   		    u32 tile_mode, u32 tile_flags, struct sg_table *sg,
>   		    struct reservation_object *robj,
> 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);
> +
>   static struct drm_driver driver_stub;
>   static struct drm_driver driver_pci;
>   static struct drm_driver driver_platform;
> @@ -897,6 +901,7 @@ nouveau_ioctls[] = {
>   	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_FINI, nouveau_gem_ioctl_cpu_fini, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_INFO, nouveau_gem_ioctl_info, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
>   	/* Staging ioctls */
> +	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_SET_TILING, nouveau_gem_ioctl_set_tiling, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
>   };
>
>   long
> @@ -1029,6 +1034,7 @@ static void nouveau_display_options(void)
>   	DRM_DEBUG_DRIVER("... runpm        : %d\n", nouveau_runtime_pm);
>   	DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf);
>   	DRM_DEBUG_DRIVER("... pstate       : %d\n", nouveau_pstate);
> +	DRM_DEBUG_DRIVER("... staging_tiling: %d\n", nouveau_staging_tiling);
>   }
>
>   static const struct dev_pm_ops nouveau_pm_ops = {
> 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;
> +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;
> +	}
> +
> +	if (nvbo->tile_mode != req->tile_mode ||
> +	    nvbo->tile_flags != req->tile_flags) {
> +		ret = ttm_bo_reserve(&nvbo->bo, false, false, false, NULL);
> +		if (ret)
> +			goto out;
> +
> +		nvbo->tile_mode = req->tile_mode;
> +		nvbo->tile_flags = req->tile_flags;
> +
> +		nouveau_bo_update_tiling(drm, nvbo, nvbo->bo.mem.mm_node);
> +
> +		/* remap over existing mapping with new tile parameters */
> +		vma = nouveau_bo_vma_find(nvbo, cli->vm);
> +		if (vma)
> +			nvkm_vm_map(vma, nvbo->bo.mem.mm_node);
> +
> +		ttm_bo_unreserve(&nvbo->bo);
> +	}
> +
> +out:
> +	drm_gem_object_unreference_unlocked(gem);
> +	return ret;
> +}
> +
>   int
>   nouveau_gem_new(struct drm_device *dev, int size, int align, uint32_t domain,
>   		uint32_t tile_mode, uint32_t tile_flags,
> diff --git a/drm/nouveau/nouveau_gem.h b/drm/nouveau/nouveau_gem.h
> index e4049faca780..56e741d98bcd 100644
> --- a/drm/nouveau/nouveau_gem.h
> +++ b/drm/nouveau/nouveau_gem.h
> @@ -23,6 +23,8 @@ extern void nouveau_gem_object_del(struct drm_gem_object *);
>   extern int nouveau_gem_object_open(struct drm_gem_object *, struct drm_file *);
>   extern void nouveau_gem_object_close(struct drm_gem_object *,
>   				     struct drm_file *);
> +extern int nouveau_gem_ioctl_set_tiling(struct drm_device *, void *,
> +					struct drm_file *);
>   extern int nouveau_gem_ioctl_new(struct drm_device *, void *,
>   				 struct drm_file *);
>   extern int nouveau_gem_ioctl_pushbuf(struct drm_device *, void *,
> diff --git a/drm/nouveau/nouveau_ttm.c b/drm/nouveau/nouveau_ttm.c
> index 18f449715788..d680153db6e5 100644
> --- a/drm/nouveau/nouveau_ttm.c
> +++ b/drm/nouveau/nouveau_ttm.c
> @@ -174,18 +174,7 @@ nouveau_gart_manager_new(struct ttm_mem_type_manager *man,
>
>   	node->page_shift = 12;
>
> -	switch (drm->device.info.family) {
> -	case NV_DEVICE_INFO_V0_TESLA:
> -		if (drm->device.info.chipset != 0x50)
> -			node->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
> -		break;
> -	case NV_DEVICE_INFO_V0_FERMI:
> -	case NV_DEVICE_INFO_V0_KEPLER:
> -		node->memtype = (nvbo->tile_flags & 0xff00) >> 8;
> -		break;
> -	default:
> -		break;
> -	}
> +	nouveau_bo_update_tiling(drm, nvbo, node);
>
>   	mem->mm_node = node;
>   	mem->start   = 0;
> diff --git a/drm/nouveau/uapi/drm/nouveau_drm.h b/drm/nouveau/uapi/drm/nouveau_drm.h
> index 4e7e21f41b5c..8f10b16b1473 100644
> --- a/drm/nouveau/uapi/drm/nouveau_drm.h
> +++ b/drm/nouveau/uapi/drm/nouveau_drm.h
> @@ -64,6 +64,12 @@ struct drm_nouveau_gem_new {
>   	uint32_t align;
>   };
>
> +struct drm_nouveau_gem_set_tiling {
> +	uint32_t handle;
> +	uint32_t tile_mode;
> +	uint32_t tile_flags;
> +};
> +
>   #define NOUVEAU_GEM_MAX_BUFFERS 1024
>   struct drm_nouveau_gem_pushbuf_bo_presumed {
>   	uint32_t valid;
> @@ -142,6 +148,7 @@ struct drm_nouveau_gem_cpu_fini {
>   #define DRM_NOUVEAU_GEM_INFO           0x44
>   /* range 0x98..DRM_COMMAND_END (8 entries) is reserved for staging, unstable ioctls */
>   #define DRM_NOUVEAU_STAGING_IOCTL      0x58
> +#define DRM_NOUVEAU_GEM_SET_TILING     (DRM_NOUVEAU_STAGING_IOCTL + 0x0)
>
>   #define DRM_IOCTL_NOUVEAU_GEM_NEW            DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_NEW, struct drm_nouveau_gem_new)
>   #define DRM_IOCTL_NOUVEAU_GEM_PUSHBUF        DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_PUSHBUF, struct drm_nouveau_gem_pushbuf)
> @@ -149,5 +156,6 @@ struct drm_nouveau_gem_cpu_fini {
>   #define DRM_IOCTL_NOUVEAU_GEM_CPU_FINI       DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_FINI, struct drm_nouveau_gem_cpu_fini)
>   #define DRM_IOCTL_NOUVEAU_GEM_INFO           DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_INFO, struct drm_nouveau_gem_info)
>   /* staging ioctls */
> +#define DRM_IOCTL_NOUVEAU_GEM_SET_TILING     DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_SET_TILING, struct drm_nouveau_gem_set_tiling)
>
>   #endif /* __NOUVEAU_DRM_H__ */
>

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Daniel Vetter]

On Mon, Jun 15, 2015 at 04:09:29PM +0900, 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.
> 
> Signed-off-by: Ari Hirvonen <ahirvonen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> [acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org: carry upstream, many fixes]
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drm/nouveau/nouveau_bo.c           | 18 ++++++++++++
>  drm/nouveau/nouveau_bo.h           |  2 ++
>  drm/nouveau/nouveau_drm.c          |  6 ++++
>  drm/nouveau/nouveau_gem.c          | 58 ++++++++++++++++++++++++++++++++++++++
>  drm/nouveau/nouveau_gem.h          |  2 ++
>  drm/nouveau/nouveau_ttm.c          | 13 +--------
>  drm/nouveau/uapi/drm/nouveau_drm.h |  8 ++++++
>  7 files changed, 95 insertions(+), 12 deletions(-)
> 
> diff --git a/drm/nouveau/nouveau_bo.c b/drm/nouveau/nouveau_bo.c
> index 6edcce1658b7..2a2ebbeb4fc0 100644
> --- a/drm/nouveau/nouveau_bo.c
> +++ b/drm/nouveau/nouveau_bo.c
> @@ -178,6 +178,24 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
>  	*size = roundup(*size, PAGE_SIZE);
>  }
>  
> +void
> +nouveau_bo_update_tiling(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
> +			 struct nvkm_mem *mem)
> +{
> +	switch (drm->device.info.family) {
> +	case NV_DEVICE_INFO_V0_TESLA:
> +		if (drm->device.info.chipset != 0x50)
> +			mem->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
> +		break;
> +	case NV_DEVICE_INFO_V0_FERMI:
> +	case NV_DEVICE_INFO_V0_KEPLER:
> +		mem->memtype = (nvbo->tile_flags & 0xff00) >> 8;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  int
>  nouveau_bo_new(struct drm_device *dev, int size, int align,
>  	       uint32_t flags, uint32_t tile_mode, uint32_t tile_flags,
> diff --git a/drm/nouveau/nouveau_bo.h b/drm/nouveau/nouveau_bo.h
> index e42360983229..87d07e3533eb 100644
> --- a/drm/nouveau/nouveau_bo.h
> +++ b/drm/nouveau/nouveau_bo.h
> @@ -69,6 +69,8 @@ nouveau_bo_ref(struct nouveau_bo *ref, struct nouveau_bo **pnvbo)
>  extern struct ttm_bo_driver nouveau_bo_driver;
>  
>  void nouveau_bo_move_init(struct nouveau_drm *);
> +void nouveau_bo_update_tiling(struct nouveau_drm *, struct nouveau_bo *,
> +			      struct nvkm_mem *);
>  int  nouveau_bo_new(struct drm_device *, int size, int align, u32 flags,
>  		    u32 tile_mode, u32 tile_flags, struct sg_table *sg,
>  		    struct reservation_object *robj,
> 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);

Please use _unsafe here to make sure that setting this option taints the
kernel and gives at least a bit of a deterrent. But in the end the policy
is still that you can't regress anything if people complain, which means
you might end up with a staging ioctl locked down forever.

The other part I don't like with this plan is that it looks a bit like it
could be easily abused to evade the open source userspace requirement
upstream has for new interfaces. Doesn't help that your first staging
ioctl doesn't come with links to mesa/hwc/whatever patches attached ;-)

Overall I don't think this will help - you need internal branch management
anyway, and upstreaming new ABI is somewhat painful for a reason: Screwing
things up is really expensive long-term, and the drm community has paid
that price a few too many times.

Cheers, Daniel


> +
>  static struct drm_driver driver_stub;
>  static struct drm_driver driver_pci;
>  static struct drm_driver driver_platform;
> @@ -897,6 +901,7 @@ nouveau_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_FINI, nouveau_gem_ioctl_cpu_fini, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_INFO, nouveau_gem_ioctl_info, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
>  	/* Staging ioctls */
> +	DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_SET_TILING, nouveau_gem_ioctl_set_tiling, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW),
>  };
>  
>  long
> @@ -1029,6 +1034,7 @@ static void nouveau_display_options(void)
>  	DRM_DEBUG_DRIVER("... runpm        : %d\n", nouveau_runtime_pm);
>  	DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf);
>  	DRM_DEBUG_DRIVER("... pstate       : %d\n", nouveau_pstate);
> +	DRM_DEBUG_DRIVER("... staging_tiling: %d\n", nouveau_staging_tiling);
>  }
>  
>  static const struct dev_pm_ops nouveau_pm_ops = {
> 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;
> +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;
> +	}
> +
> +	if (nvbo->tile_mode != req->tile_mode ||
> +	    nvbo->tile_flags != req->tile_flags) {
> +		ret = ttm_bo_reserve(&nvbo->bo, false, false, false, NULL);
> +		if (ret)
> +			goto out;
> +
> +		nvbo->tile_mode = req->tile_mode;
> +		nvbo->tile_flags = req->tile_flags;
> +
> +		nouveau_bo_update_tiling(drm, nvbo, nvbo->bo.mem.mm_node);
> +
> +		/* remap over existing mapping with new tile parameters */
> +		vma = nouveau_bo_vma_find(nvbo, cli->vm);
> +		if (vma)
> +			nvkm_vm_map(vma, nvbo->bo.mem.mm_node);
> +
> +		ttm_bo_unreserve(&nvbo->bo);
> +	}
> +
> +out:
> +	drm_gem_object_unreference_unlocked(gem);
> +	return ret;
> +}
> +
>  int
>  nouveau_gem_new(struct drm_device *dev, int size, int align, uint32_t domain,
>  		uint32_t tile_mode, uint32_t tile_flags,
> diff --git a/drm/nouveau/nouveau_gem.h b/drm/nouveau/nouveau_gem.h
> index e4049faca780..56e741d98bcd 100644
> --- a/drm/nouveau/nouveau_gem.h
> +++ b/drm/nouveau/nouveau_gem.h
> @@ -23,6 +23,8 @@ extern void nouveau_gem_object_del(struct drm_gem_object *);
>  extern int nouveau_gem_object_open(struct drm_gem_object *, struct drm_file *);
>  extern void nouveau_gem_object_close(struct drm_gem_object *,
>  				     struct drm_file *);
> +extern int nouveau_gem_ioctl_set_tiling(struct drm_device *, void *,
> +					struct drm_file *);
>  extern int nouveau_gem_ioctl_new(struct drm_device *, void *,
>  				 struct drm_file *);
>  extern int nouveau_gem_ioctl_pushbuf(struct drm_device *, void *,
> diff --git a/drm/nouveau/nouveau_ttm.c b/drm/nouveau/nouveau_ttm.c
> index 18f449715788..d680153db6e5 100644
> --- a/drm/nouveau/nouveau_ttm.c
> +++ b/drm/nouveau/nouveau_ttm.c
> @@ -174,18 +174,7 @@ nouveau_gart_manager_new(struct ttm_mem_type_manager *man,
>  
>  	node->page_shift = 12;
>  
> -	switch (drm->device.info.family) {
> -	case NV_DEVICE_INFO_V0_TESLA:
> -		if (drm->device.info.chipset != 0x50)
> -			node->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
> -		break;
> -	case NV_DEVICE_INFO_V0_FERMI:
> -	case NV_DEVICE_INFO_V0_KEPLER:
> -		node->memtype = (nvbo->tile_flags & 0xff00) >> 8;
> -		break;
> -	default:
> -		break;
> -	}
> +	nouveau_bo_update_tiling(drm, nvbo, node);
>  
>  	mem->mm_node = node;
>  	mem->start   = 0;
> diff --git a/drm/nouveau/uapi/drm/nouveau_drm.h b/drm/nouveau/uapi/drm/nouveau_drm.h
> index 4e7e21f41b5c..8f10b16b1473 100644
> --- a/drm/nouveau/uapi/drm/nouveau_drm.h
> +++ b/drm/nouveau/uapi/drm/nouveau_drm.h
> @@ -64,6 +64,12 @@ struct drm_nouveau_gem_new {
>  	uint32_t align;
>  };
>  
> +struct drm_nouveau_gem_set_tiling {
> +	uint32_t handle;
> +	uint32_t tile_mode;
> +	uint32_t tile_flags;
> +};
> +
>  #define NOUVEAU_GEM_MAX_BUFFERS 1024
>  struct drm_nouveau_gem_pushbuf_bo_presumed {
>  	uint32_t valid;
> @@ -142,6 +148,7 @@ struct drm_nouveau_gem_cpu_fini {
>  #define DRM_NOUVEAU_GEM_INFO           0x44
>  /* range 0x98..DRM_COMMAND_END (8 entries) is reserved for staging, unstable ioctls */
>  #define DRM_NOUVEAU_STAGING_IOCTL      0x58
> +#define DRM_NOUVEAU_GEM_SET_TILING     (DRM_NOUVEAU_STAGING_IOCTL + 0x0)
>  
>  #define DRM_IOCTL_NOUVEAU_GEM_NEW            DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_NEW, struct drm_nouveau_gem_new)
>  #define DRM_IOCTL_NOUVEAU_GEM_PUSHBUF        DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_PUSHBUF, struct drm_nouveau_gem_pushbuf)
> @@ -149,5 +156,6 @@ struct drm_nouveau_gem_cpu_fini {
>  #define DRM_IOCTL_NOUVEAU_GEM_CPU_FINI       DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_FINI, struct drm_nouveau_gem_cpu_fini)
>  #define DRM_IOCTL_NOUVEAU_GEM_INFO           DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_INFO, struct drm_nouveau_gem_info)
>  /* staging ioctls */
> +#define DRM_IOCTL_NOUVEAU_GEM_SET_TILING     DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_SET_TILING, struct drm_nouveau_gem_set_tiling)
>  
>  #endif /* __NOUVEAU_DRM_H__ */
> -- 
> 2.4.2
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

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

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

[Alexandre Courbot]

On 06/15/2015 04:56 PM, Daniel Vetter wrote:
> On Mon, Jun 15, 2015 at 04:09:29PM +0900, 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.
>>
>> Signed-off-by: Ari Hirvonen <ahirvonen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> [acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org: carry upstream, many fixes]
>> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drm/nouveau/nouveau_bo.c           | 18 ++++++++++++
>>   drm/nouveau/nouveau_bo.h           |  2 ++
>>   drm/nouveau/nouveau_drm.c          |  6 ++++
>>   drm/nouveau/nouveau_gem.c          | 58 ++++++++++++++++++++++++++++++++++++++
>>   drm/nouveau/nouveau_gem.h          |  2 ++
>>   drm/nouveau/nouveau_ttm.c          | 13 +--------
>>   drm/nouveau/uapi/drm/nouveau_drm.h |  8 ++++++
>>   7 files changed, 95 insertions(+), 12 deletions(-)
>>
>> diff --git a/drm/nouveau/nouveau_bo.c b/drm/nouveau/nouveau_bo.c
>> index 6edcce1658b7..2a2ebbeb4fc0 100644
>> --- a/drm/nouveau/nouveau_bo.c
>> +++ b/drm/nouveau/nouveau_bo.c
>> @@ -178,6 +178,24 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
>>   	*size = roundup(*size, PAGE_SIZE);
>>   }
>>
>> +void
>> +nouveau_bo_update_tiling(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
>> +			 struct nvkm_mem *mem)
>> +{
>> +	switch (drm->device.info.family) {
>> +	case NV_DEVICE_INFO_V0_TESLA:
>> +		if (drm->device.info.chipset != 0x50)
>> +			mem->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
>> +		break;
>> +	case NV_DEVICE_INFO_V0_FERMI:
>> +	case NV_DEVICE_INFO_V0_KEPLER:
>> +		mem->memtype = (nvbo->tile_flags & 0xff00) >> 8;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>>   int
>>   nouveau_bo_new(struct drm_device *dev, int size, int align,
>>   	       uint32_t flags, uint32_t tile_mode, uint32_t tile_flags,
>> diff --git a/drm/nouveau/nouveau_bo.h b/drm/nouveau/nouveau_bo.h
>> index e42360983229..87d07e3533eb 100644
>> --- a/drm/nouveau/nouveau_bo.h
>> +++ b/drm/nouveau/nouveau_bo.h
>> @@ -69,6 +69,8 @@ nouveau_bo_ref(struct nouveau_bo *ref, struct nouveau_bo **pnvbo)
>>   extern struct ttm_bo_driver nouveau_bo_driver;
>>
>>   void nouveau_bo_move_init(struct nouveau_drm *);
>> +void nouveau_bo_update_tiling(struct nouveau_drm *, struct nouveau_bo *,
>> +			      struct nvkm_mem *);
>>   int  nouveau_bo_new(struct drm_device *, int size, int align, u32 flags,
>>   		    u32 tile_mode, u32 tile_flags, struct sg_table *sg,
>>   		    struct reservation_object *robj,
>> 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);
>
> Please use _unsafe here to make sure that setting this option taints the
> kernel and gives at least a bit of a deterrent. But in the end the policy
> is still that you can't regress anything if people complain, which means
> you might end up with a staging ioctl locked down forever.

That would kind of kill the whole purpose of this patchset. But at the 
same time the point of having staging ioctls is to say "don't use them 
in production", so hopefully the message is clear.

> The other part I don't like with this plan is that it looks a bit like it
> could be easily abused to evade the open source userspace requirement
> upstream has for new interfaces. Doesn't help that your first staging
> ioctl doesn't come with links to mesa/hwc/whatever patches attached ;-)

Well, you could abuse it - no more than 8 times though. ;)

The point is not to evade anything though, but rather to have the 
necessary kernel code land in such a way that we can experiment with 
Mesa and other user-space.

> Overall I don't think this will help - you need internal branch management
> anyway, and upstreaming new ABI is somewhat painful for a reason: Screwing
> things up is really expensive long-term, and the drm community has paid
> that price a few too many times.

It seems to me that this staging feature can exactly help with that: 
allow new ioctls to mature a bit (no longer than a kernel release cycle) 
and avoid that "ah, I wish we did this differently" moment. But 
considering the number of ABIs I have driven so far (0), someone more 
experienced may challenge that belief.

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

[Daniel Vetter]

On Mon, Jun 15, 2015 at 06:08:21PM +0900, Alexandre Courbot wrote:
> On 06/15/2015 04:56 PM, Daniel Vetter wrote:
> >On Mon, Jun 15, 2015 at 04:09:29PM +0900, Alexandre Courbot wrote:
> >>From: Ari Hirvonen <ahirvonen@nvidia.com>
> >>
> >>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.
> >>
> >>Signed-off-by: Ari Hirvonen <ahirvonen@nvidia.com>
> >>[acourbot@nvidia.com: carry upstream, many fixes]
> >>Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> >>---
> >>  drm/nouveau/nouveau_bo.c           | 18 ++++++++++++
> >>  drm/nouveau/nouveau_bo.h           |  2 ++
> >>  drm/nouveau/nouveau_drm.c          |  6 ++++
> >>  drm/nouveau/nouveau_gem.c          | 58 ++++++++++++++++++++++++++++++++++++++
> >>  drm/nouveau/nouveau_gem.h          |  2 ++
> >>  drm/nouveau/nouveau_ttm.c          | 13 +--------
> >>  drm/nouveau/uapi/drm/nouveau_drm.h |  8 ++++++
> >>  7 files changed, 95 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/drm/nouveau/nouveau_bo.c b/drm/nouveau/nouveau_bo.c
> >>index 6edcce1658b7..2a2ebbeb4fc0 100644
> >>--- a/drm/nouveau/nouveau_bo.c
> >>+++ b/drm/nouveau/nouveau_bo.c
> >>@@ -178,6 +178,24 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
> >>  	*size = roundup(*size, PAGE_SIZE);
> >>  }
> >>
> >>+void
> >>+nouveau_bo_update_tiling(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
> >>+			 struct nvkm_mem *mem)
> >>+{
> >>+	switch (drm->device.info.family) {
> >>+	case NV_DEVICE_INFO_V0_TESLA:
> >>+		if (drm->device.info.chipset != 0x50)
> >>+			mem->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
> >>+		break;
> >>+	case NV_DEVICE_INFO_V0_FERMI:
> >>+	case NV_DEVICE_INFO_V0_KEPLER:
> >>+		mem->memtype = (nvbo->tile_flags & 0xff00) >> 8;
> >>+		break;
> >>+	default:
> >>+		break;
> >>+	}
> >>+}
> >>+
> >>  int
> >>  nouveau_bo_new(struct drm_device *dev, int size, int align,
> >>  	       uint32_t flags, uint32_t tile_mode, uint32_t tile_flags,
> >>diff --git a/drm/nouveau/nouveau_bo.h b/drm/nouveau/nouveau_bo.h
> >>index e42360983229..87d07e3533eb 100644
> >>--- a/drm/nouveau/nouveau_bo.h
> >>+++ b/drm/nouveau/nouveau_bo.h
> >>@@ -69,6 +69,8 @@ nouveau_bo_ref(struct nouveau_bo *ref, struct nouveau_bo **pnvbo)
> >>  extern struct ttm_bo_driver nouveau_bo_driver;
> >>
> >>  void nouveau_bo_move_init(struct nouveau_drm *);
> >>+void nouveau_bo_update_tiling(struct nouveau_drm *, struct nouveau_bo *,
> >>+			      struct nvkm_mem *);
> >>  int  nouveau_bo_new(struct drm_device *, int size, int align, u32 flags,
> >>  		    u32 tile_mode, u32 tile_flags, struct sg_table *sg,
> >>  		    struct reservation_object *robj,
> >>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);
> >
> >Please use _unsafe here to make sure that setting this option taints the
> >kernel and gives at least a bit of a deterrent. But in the end the policy
> >is still that you can't regress anything if people complain, which means
> >you might end up with a staging ioctl locked down forever.
> 
> That would kind of kill the whole purpose of this patchset. But at the same
> time the point of having staging ioctls is to say "don't use them in
> production", so hopefully the message is clear.
> 
> >The other part I don't like with this plan is that it looks a bit like it
> >could be easily abused to evade the open source userspace requirement
> >upstream has for new interfaces. Doesn't help that your first staging
> >ioctl doesn't come with links to mesa/hwc/whatever patches attached ;-)
> 
> Well, you could abuse it - no more than 8 times though. ;)
> 
> The point is not to evade anything though, but rather to have the necessary
> kernel code land in such a way that we can experiment with Mesa and other
> user-space.
> 
> >Overall I don't think this will help - you need internal branch management
> >anyway, and upstreaming new ABI is somewhat painful for a reason: Screwing
> >things up is really expensive long-term, and the drm community has paid
> >that price a few too many times.
> 
> It seems to me that this staging feature can exactly help with that: allow
> new ioctls to mature a bit (no longer than a kernel release cycle) and avoid
> that "ah, I wish we did this differently" moment. But considering the number
> of ABIs I have driven so far (0), someone more experienced may challenge
> that belief.

Maybe some follow up from irc discussions is in order: It's really a
judgment call whether it makes sense. Imo the problem is that marking
something as staging is way too tempting and you get sloppy and end up
with a baked-in abi that you don't really want. Since in the end it
doesn't matter what you declare as staging but whether there are people
complaining about regressions when you break it (famously demonstrated
years ago with nouveau in staging and giant flamours when nouveau broke
their abi, resulting in Linus ulitmately demanding that nouveau gets
destaged asap).

Given that it's imo better to just roll with a final abi. I think in the
past we've overdone things a bit with experimental abis in the drm core,
only the atomic stuff looks big enough in retrospective to really justify
staging it. Everything else was just chickening out from committing to
things right away. In i915 the only thing we stage nowadays is new hw
enabling, simply because we start so early nowadays that with the first
cut we simply don't know yet whether the code will work perfectly on final
silicon. And we've been bitten in the past by regression reports where
people upgraded from old kernels to versions with experimental support and
couldn't boot any more at all. But that's the only exception.

You also mention the issue with having a common assembly ground for
internal patches. Staging ioctls isn't going to solve that, you need
internal trees anyway. We have lots of those for i915, you just can't see
them ;-)

Summary of my stance: In almost all cases staging ioctls is just an
illusion and doesn't give you real benefits.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Alexandre Courbot]

On Tue, Jun 16, 2015 at 7:07 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jun 15, 2015 at 06:08:21PM +0900, Alexandre Courbot wrote:
>> On 06/15/2015 04:56 PM, Daniel Vetter wrote:
>> >On Mon, Jun 15, 2015 at 04:09:29PM +0900, Alexandre Courbot wrote:
>> >>From: Ari Hirvonen <ahirvonen@nvidia.com>
>> >>
>> >>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.
>> >>
>> >>Signed-off-by: Ari Hirvonen <ahirvonen@nvidia.com>
>> >>[acourbot@nvidia.com: carry upstream, many fixes]
>> >>Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> >>---
>> >>  drm/nouveau/nouveau_bo.c           | 18 ++++++++++++
>> >>  drm/nouveau/nouveau_bo.h           |  2 ++
>> >>  drm/nouveau/nouveau_drm.c          |  6 ++++
>> >>  drm/nouveau/nouveau_gem.c          | 58 ++++++++++++++++++++++++++++++++++++++
>> >>  drm/nouveau/nouveau_gem.h          |  2 ++
>> >>  drm/nouveau/nouveau_ttm.c          | 13 +--------
>> >>  drm/nouveau/uapi/drm/nouveau_drm.h |  8 ++++++
>> >>  7 files changed, 95 insertions(+), 12 deletions(-)
>> >>
>> >>diff --git a/drm/nouveau/nouveau_bo.c b/drm/nouveau/nouveau_bo.c
>> >>index 6edcce1658b7..2a2ebbeb4fc0 100644
>> >>--- a/drm/nouveau/nouveau_bo.c
>> >>+++ b/drm/nouveau/nouveau_bo.c
>> >>@@ -178,6 +178,24 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
>> >>    *size = roundup(*size, PAGE_SIZE);
>> >>  }
>> >>
>> >>+void
>> >>+nouveau_bo_update_tiling(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
>> >>+                    struct nvkm_mem *mem)
>> >>+{
>> >>+   switch (drm->device.info.family) {
>> >>+   case NV_DEVICE_INFO_V0_TESLA:
>> >>+           if (drm->device.info.chipset != 0x50)
>> >>+                   mem->memtype = (nvbo->tile_flags & 0x7f00) >> 8;
>> >>+           break;
>> >>+   case NV_DEVICE_INFO_V0_FERMI:
>> >>+   case NV_DEVICE_INFO_V0_KEPLER:
>> >>+           mem->memtype = (nvbo->tile_flags & 0xff00) >> 8;
>> >>+           break;
>> >>+   default:
>> >>+           break;
>> >>+   }
>> >>+}
>> >>+
>> >>  int
>> >>  nouveau_bo_new(struct drm_device *dev, int size, int align,
>> >>           uint32_t flags, uint32_t tile_mode, uint32_t tile_flags,
>> >>diff --git a/drm/nouveau/nouveau_bo.h b/drm/nouveau/nouveau_bo.h
>> >>index e42360983229..87d07e3533eb 100644
>> >>--- a/drm/nouveau/nouveau_bo.h
>> >>+++ b/drm/nouveau/nouveau_bo.h
>> >>@@ -69,6 +69,8 @@ nouveau_bo_ref(struct nouveau_bo *ref, struct nouveau_bo **pnvbo)
>> >>  extern struct ttm_bo_driver nouveau_bo_driver;
>> >>
>> >>  void nouveau_bo_move_init(struct nouveau_drm *);
>> >>+void nouveau_bo_update_tiling(struct nouveau_drm *, struct nouveau_bo *,
>> >>+                         struct nvkm_mem *);
>> >>  int  nouveau_bo_new(struct drm_device *, int size, int align, u32 flags,
>> >>                u32 tile_mode, u32 tile_flags, struct sg_table *sg,
>> >>                struct reservation_object *robj,
>> >>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);
>> >
>> >Please use _unsafe here to make sure that setting this option taints the
>> >kernel and gives at least a bit of a deterrent. But in the end the policy
>> >is still that you can't regress anything if people complain, which means
>> >you might end up with a staging ioctl locked down forever.
>>
>> That would kind of kill the whole purpose of this patchset. But at the same
>> time the point of having staging ioctls is to say "don't use them in
>> production", so hopefully the message is clear.
>>
>> >The other part I don't like with this plan is that it looks a bit like it
>> >could be easily abused to evade the open source userspace requirement
>> >upstream has for new interfaces. Doesn't help that your first staging
>> >ioctl doesn't come with links to mesa/hwc/whatever patches attached ;-)
>>
>> Well, you could abuse it - no more than 8 times though. ;)
>>
>> The point is not to evade anything though, but rather to have the necessary
>> kernel code land in such a way that we can experiment with Mesa and other
>> user-space.
>>
>> >Overall I don't think this will help - you need internal branch management
>> >anyway, and upstreaming new ABI is somewhat painful for a reason: Screwing
>> >things up is really expensive long-term, and the drm community has paid
>> >that price a few too many times.
>>
>> It seems to me that this staging feature can exactly help with that: allow
>> new ioctls to mature a bit (no longer than a kernel release cycle) and avoid
>> that "ah, I wish we did this differently" moment. But considering the number
>> of ABIs I have driven so far (0), someone more experienced may challenge
>> that belief.
>
> Maybe some follow up from irc discussions is in order: It's really a
> judgment call whether it makes sense. Imo the problem is that marking
> something as staging is way too tempting and you get sloppy and end up
> with a baked-in abi that you don't really want. Since in the end it
> doesn't matter what you declare as staging but whether there are people
> complaining about regressions when you break it (famously demonstrated
> years ago with nouveau in staging and giant flamours when nouveau broke
> their abi, resulting in Linus ulitmately demanding that nouveau gets
> destaged asap).
>
> Given that it's imo better to just roll with a final abi. I think in the
> past we've overdone things a bit with experimental abis in the drm core,
> only the atomic stuff looks big enough in retrospective to really justify
> staging it. Everything else was just chickening out from committing to
> things right away. In i915 the only thing we stage nowadays is new hw
> enabling, simply because we start so early nowadays that with the first
> cut we simply don't know yet whether the code will work perfectly on final
> silicon. And we've been bitten in the past by regression reports where
> people upgraded from old kernels to versions with experimental support and
> couldn't boot any more at all. But that's the only exception.
>
> You also mention the issue with having a common assembly ground for
> internal patches. Staging ioctls isn't going to solve that, you need
> internal trees anyway. We have lots of those for i915, you just can't see
> them ;-)
>
> Summary of my stance: In almost all cases staging ioctls is just an
> illusion and doesn't give you real benefits.

Allright, I am dropping these two patches for now then. On top of what
you said, we are still lacking user-space code for this ioctl anyway.
I will experiment a bit more with our new ioctls and hopefully come to
the same conclusion as you. ;)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- 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 --]

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

[Alexandre Courbot]

Hi Thierry,

On Mon, Jun 29, 2015 at 10:30 PM, Thierry Reding <treding@nvidia.com> wrote:
> 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@nvidia.com>
>> >
>> >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.

Ok, so does it mean that this kind of use-case is best handled by a
driver-specific IOCTL?

>
> 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.

We came to this option with Ben while discussing how staging IOCTLs
could be implemented. Since we decided to drop this idea altogether, I
guess we don't have to worry about how to implement the option
anymore.

>
>> >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.

Yeah, several people weighted in to point out this was a bad idea, so
this patch series is dropped for now. We will just carry these extra
IOCTLs out-of-tree for a longer time, and make sure they are fit and
well-tested before submitting them upstream.

>
>> >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.

The reason is because we actually don't want to *change* the tiling
settings as much as we want to *set* them for imported buffers. The
DRM_NOUVEAU_GEM_NEW ioctl has a tiling parameter, and we need the same
feature for PRIME-imported buffers. This is why I would have preferred
to have this information somehow attached to the buffer itself, or
specified at import time, since having a dedicated ioctl tends to
suggest that it is to change the tiling settings.

I wanted your thoughts on the topic because I know you had the same
issue with tegra DRM and actively looked for a solution - but from
your comments, it seems like that solution is to simply use a
dedicated IOCTL for this.

Thanks for your comments!
Alex.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3947 bytes --]

On Mon, Jul 06, 2015 at 08:41:07PM +0900, Alexandre Courbot wrote:
> Hi Thierry,
> 
> On Mon, Jun 29, 2015 at 10:30 PM, Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > 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.
> 
> Ok, so does it mean that this kind of use-case is best handled by a
> driver-specific IOCTL?

Yeah, I think it is.

> >> >+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.
> 
> The reason is because we actually don't want to *change* the tiling
> settings as much as we want to *set* them for imported buffers. The
> DRM_NOUVEAU_GEM_NEW ioctl has a tiling parameter, and we need the same
> feature for PRIME-imported buffers.

That would make a much better comment than what you currently have. =)

> This is why I would have preferred to have this information somehow
> attached to the buffer itself, or specified at import time, since
> having a dedicated ioctl tends to suggest that it is to change the
> tiling settings.
> 
> I wanted your thoughts on the topic because I know you had the same
> issue with tegra DRM and actively looked for a solution - but from
> your comments, it seems like that solution is to simply use a
> dedicated IOCTL for this.

Yes, I know of no other way to do this.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau