Re: [PATCH v4 7/8] drm/fourcc: amlogic: Add modifier definitions for the Scatter layout

[Neil Armstrong]

Hi,

On 26/03/2020 10:36, Pekka Paalanen wrote:
> On Wed, 25 Mar 2020 17:18:15 +0100
> Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
> 
>> Hi,
>>
>> On 25/03/2020 14:49, Pekka Paalanen wrote:
>>> On Wed, 25 Mar 2020 11:24:15 +0100
>>> Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>>>   
>>>> Hi,
>>>>
>>>> On 25/03/2020 10:04, Simon Ser wrote:  
>>>>> On Wednesday, March 25, 2020 9:50 AM, Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>>>>>     
>>>>>> Amlogic uses a proprietary lossless image compression protocol and format
>>>>>> for their hardware video codec accelerators, either video decoders or
>>>>>> video input encoders.
>>>>>>
>>>>>> This introduces the Scatter Memory layout, means the header contains IOMMU
>>>>>> references to the compressed frames content to optimize memory access
>>>>>> and layout.
>>>>>>
>>>>>> In this mode, only the header memory address is needed, thus the content
>>>>>> memory organization is tied to the current producer execution and cannot
>>>>>> be saved/dumped neither transferrable between Amlogic SoCs supporting this
>>>>>> modifier.    
>>>>>
>>>>> I don't think this is suitable for modifiers. User-space relies on
>>>>> being able to copy a buffer from one machine to another over the
>>>>> network. It would be pretty annoying for user-space to have a blacklist
>>>>> of modifiers that don't work this way.
>>>>>
>>>>> Example of such user-space:
>>>>> https://gitlab.freedesktop.org/mstoeckl/waypipe/
>>>>>     
>>>>
>>>> I really understand your point, but this is one of the use-cases we need solve.
>>>> This is why I split the fourcc patch and added an explicit comment.
>>>>
>>>> Please point me a way to display such buffer, the HW exists, works like that and
>>>> it's a fact and can't change.
>>>>
>>>> It will be the same for secure zero-copy buffers we can't map from userspace, but
>>>> only the HW decoder can read/write and HW display can read.  
>>>
>>> The comparison to secure buffers is a good one.
>>>
>>> Are buffers with the DRM_FORMAT_MOD_AMLOGIC_FBC_LAYOUT_SCATTER modifier
>>> meaningfully mmappable to CPU always / sometimes / never /
>>> varies-and-cannot-know?  
>>
>> mmappable, yes in our WIP V4L2 driver in non-secure path, meaningful, absolutely never.
>>
>> So yeah, these should not be mmappable since not meaningful.
> 
> Ok. So we have a modifier that means there is no point in even trying to
> mmap the buffer.
> 
> Not being able to mmap automatically makes things like waypipe not be
> able to work on the buffer, so the buffer cannot be replicated over a
> network, hence there is no compatibility issue. However, it still
> leaves the problem that, since waypipe is "just" a message relay that
> does not participate in the protocol really, the two end points might
> still negotiate to use a modifier that waypipe cannot handle.

Not mmapable won't be limited to this kind of buffer, or secure, any DMA-BUF
provider can decide to disable mmaping, so waypipe should work with this
whatever this discussion goes to.

> 
> Secure buffers have the same problem: by definition, one must not be
> able to replicate the buffer elsewhere.
> 
> To me it seems there needs to be a way to identify buffers that cannot
> be mmapped. mmap() failing is obvious, but in waypipe's case it is too
> late - the end points have already negotiated the formats and modifiers
> and they cannot handle failures afterwards.

The AFAIK last open question was on this thread:
https://lore.kernel.org/dri-devel/d6f8092d-9f90-d5ff-2ab3-b1867f8f5700@xxxxxx/
But it was more like, how the consumer driver knows the buffer is secure.

Daniel, is there something new ?

> 
>>>
>>> Maybe this type should be handled similar to secure buffers, with the
>>> exception that they are not actually secured but only mostly
>>> inaccessible. Then again, I haven't looked at any of the secure buffer
>>> proposals.  
>>
>> Actually, the Amlogic platforms offers secure video path using these exact
>> modifiers, AFAIK it doesn't support the NV12 dual-write output in secure.
>>
>> AFAIK last submission is from AMD, and it doesn't talk at all about mmapability
>> of the secure BOs.
> 
> To me, a secure buffer concept automatically implies that there cannot
> be CPU access to it. The CPU is not trusted, right? Not even the kernel.
> I would assume secure implies no mmap. So I wonder, how does the secure
> buffers proposal manage userspace like waypipe?

None, as I said, waypipe whould handle non mmapable buffers, by asking
for a different modifier set, or sending a gray buffer with a llama
instead.

> 
> Or, is the secure buffer proposal allowing mmap, but the content is
> indecipherable? Maybe they shouldn't allow mmap?

Definitely, you'll have an HW bus error if you access a secure buffer,
otherwise the security is weak. A bus firewall is the common way to handle
such secure buffers.

> 
> I think much of the criticism against this modifier should also be
> presented to a secure buffers proposal and see how that turns out. If
> they have the same problem, maybe you could use their solution?

Sure, but seems there is no consensus for the compositor side.

Neil

> 
> 
> Thanks,
> pq
>

Attachment: signature.asc
Description: OpenPGP digital signature