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?
>
> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
