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

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

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