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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> >
> >> >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:
pgp2huL1lfbXM.pgp
Description: PGP signature
