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

[Mikko Perttunen <cyndis@kapsi.fi>]

On 10/19/20 8:27 PM, Dmitry Osipenko wrote:
> 19.10.2020 11:13, Mikko Perttunen пишет:
>> On 10/19/20 5:21 AM, Dmitry Osipenko wrote:
>>> 07.10.2020 20:12, Mikko Perttunen пишет:
>>>> +int tegra_drm_ioctl_channel_map(struct drm_device *drm, void *data,
>>>> +                struct drm_file *file)
>>>> +{
>>>
>>> Hello, Mikko!
>>>
>>> Could you please tell what are the host1x clients that are going to be
>>> upstreamed and will need this IOCTL?
>>>
>>
>> Hi Dmitry!
>>
>> It is needed for any engine/job that wants to access memory, as this
>> IOCTL must be used to map memory for the engine. So all of them.
>>
>> Downstream doesn't have an equivalent IOCTL because it (currently) does
>> mapping at submit time, but that is suboptimal because
>>
>> - it requires doing relocations in the kernel which isn't required for
>> T186+
>> - it's a big performance penalty, due to which the downstream kernel has
>> the "deferred dma-buf unmapping" feature, where unmapping a dma_buf may
>> not immediately unmap it in case it's used later, so that the "mapping"
>> later is faster. A feature which we'd preferably get rid of.
>> - because of the above feature not being controlled by the user, it can
>> cause variance in submit times.
>>
>> On the other hand, we cannot (at least always) do the mapping at
>> allocation/import time, because
>>
>> - A single FD may have multiple channel_ctx's, and an allocation/import
>> may need to be used in any subset of them
>> - The import IOCTL is fixed and doesn't have the parameters we'd need to
>> do this at import time
>> - Overall it's more orthogonal to have GEM object acquirement in one
>> step and mapping in another.
>>
>> Maybe that's not quite what you asked, but it's some background anyway :)
> 
> 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).

> 
> I'm also not quite sure about whether the current model of the unified
> Tegra DRM driver is suitable for having the separated engines. Perhaps
> each separated engine should just have its own rendering node?
> 

Yeah, having separate nodes per engine might be better, e.g. so it's 
easier to do access control. I don't have a strong opinion on that though.

Mikko