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