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

[Mikko Perttunen]

On 10/22/20 7:20 AM, Dmitry Osipenko wrote:
> 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.

Sure, on older Tegras this is not strictly necessary because the 
firewall can check pointers. It's not related to IOMMU grouping.

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

In the absence of context isolation, this is correct. But with context 
isolation (which is next on my upstream todo-list), on supported chips 
(T186+), we can map to individual contexts, which are associated with 
channel_ctx's.

Without context isolation, this IOCTL just maps to the underlying 
engine. The list of mappings can also be used by the firewall later - as 
mentioned, just the relocs would be enough for that, but I think there's 
a good orthogonality in CHANNEL_MAP describing memory regions accessible 
by the engine, and relocations just doing relocations.

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

Yeah, maybe we could just have one "inline" channel context in the DRM 
file, that's just initialized by the CHANNEL_OPEN IOCTL.

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

I'll try to split up where possible.

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

Can you elaborate what you mean by this?

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

Considering that the "no-op" support for it (map the whole buffer but 
just keep track of the starting offset) is only a couple of lines, I'd 
like to keep it in.

> 
> Waiting for fences on host1x should be a separate feature.

OK.

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

I'll separate it - the reason it's there is to avoid the overhead of the 
extra ID/threshold -> sync_file conversion IOCTL if you need it.

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

I already wrote some code for syncobj I can probably pull in. Regarding 
DRM_SCHED, help is accepted. However, we should keep using the hardware 
scheduler, and considering it's a bigger piece of work, let's not block 
this series on it.

Mikko