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@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?
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