Re: [PATCH v3 19/20] drm/tegra: Implement new UAPI

From: Dmitry Osipenko <digetx@gmail.com>
To: Mikko Perttunen <cyndis@kapsi.fi>,
	Mikko Perttunen <mperttunen@nvidia.com>,
	thierry.reding@gmail.com, jonathanh@nvidia.com, airlied@linux.ie,
	daniel@ffwll.ch
Cc: linux-tegra@vger.kernel.org, talho@nvidia.com,
	bhuntsman@nvidia.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 19/20] drm/tegra: Implement new UAPI
Date: Thu, 22 Oct 2020 07:20:48 +0300	[thread overview]
Message-ID: <48f5bbac-3955-c227-dbe1-d987b4ec5bd0@gmail.com> (raw)
In-Reply-To: <6a7b4d4e-8d4a-416e-9789-45282b44bce5@kapsi.fi>

20.10.2020 12:18, Mikko Perttunen пишет:
>> I'm asking this question because right now there is only one potential
>> client for this IOCTL, the VIC. If other clients aren't supposed to be a
>> part of the DRM driver, like for example NVDEC which probably should be
>> a V4L driver, then DRM driver will have only a single VIC and in this
>> case we shouldn't need this IOCTL because DRM and V4L should use generic
>> dmabuf API for importing and exporting buffers.
> 
> This IOCTL is required for GR2D/GR3D too, as they need to access memory
> as well. This is a different step from import/export -- first you import
> or allocate your memory so you have a GEM handle, then you map it to the
> channel, which does the IOMMU mapping (if there is an IOMMU).
> 

This doesn't answer my question. I don't have a full picture and for now
will remain dubious about this IOCTL, but it should be fine to have it
in a form of WIP patches (maybe staging feature) until userspace code
and hardware specs will become available.

Some more comments:

1. Older Tegra SoCs do not have restrictions which do not allow to group
IOMMU as wished by kernel driver. It's fine to have one static mapping
per-GEM that can be accessed by all DRM devices, that's why CHANNEL_MAP
is questionable.

2. IIUC, the mappings need to be done per-device group/stream and not
per-channel_ctx. It looks like nothing stops channel contexts to guess
mapping addresses of the other context, isn't it?

I'm suggesting that each GEM should have a per-device mapping and the
new IOCTL should create these GEM-mappings, instead of the channel_ctx
mappings.

3. We shouldn't need channel contexts at all, a single "DRM file"
context should be enough to have.

4. The new UAPI need to be separated into several parts in the next
revision, one patch for each new feature.

The first patches should be the ones that are relevant to the existing
userspace code, like support for the waits.

Partial mappings should be a separate feature because it's a
questionable feature that needs to be proved by a real userspace first.
Maybe it would be even better to drop it for the starter, to ease reviewing.

Waiting for fences on host1x should be a separate feature.

The syncfile support needs to be a separate feature as well because I
don't see a use-case for it right now.

I'd like to see the DRM_SCHED and syncobj support. I can help you with
it if it's out of yours scope for now.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel