Re: [PATCH 1/4] drm/fourcc: Add modifier definitions for describing Amlogic... | linux-arm-kernel

quoted

Hi Pekka, Brian, Daniel,

On 06/03/2020 11:13, Daniel Vetter wrote:
On Tue, Mar 03, 2020 at 05:33:32PM +0200, Pekka Paalanen wrote:
On Tue, 3 Mar 2020 15:25:41 +0200
Pekka Paalanen [off-list ref] wrote:

On Tue, 3 Mar 2020 12:37:16 +0100
Daniel Vetter [off-list ref] wrote:

On Tue, Mar 3, 2020 at 11:53 AM Brian Starkey [off-list ref] wrote:  
Hi,

On Tue, Mar 03, 2020 at 12:10:29PM +0200, Pekka Paalanen wrote:    
On Fri, 21 Feb 2020 10:08:42 +0100
Neil Armstrong [off-list ref] wrote:

...
+/*
+ * Amlogic Video Framebuffer Compression modifiers
+ *
+ * Amlogic uses a proprietary lossless image compression protocol and format
+ * for their hardware video codec accelerators, either video decoders or
+ * video input encoders.
+ *
+ * It considerably reduces memory bandwidth while writing and reading
+ * frames in memory.
+ * Implementation details may be platform and SoC specific, and shared
+ * between the producer and the decoder on the same platform.    
Hi,

after a lengthy IRC discussion on #dri-devel, this "may be platform and
SoC specific" is a problem.
This one is definitely only for the SCATTER modifier, not the DEFAULT and MEM_SAVING.

It can be an issue in two ways:

- If something in the data acts like a sub-modifier, then advertising
  support for one modifier does not really tell if the data layout is
  supported or not.
It's clearly not.

The DEFAULT and MEM_SAVING modifiers are clearly transferable, and their layout is
extremely simple. While we don't have the memory compression algorithm, the memory
layout is simple to describe and doesn't act as a sub-modifier.

The complexity lies in the SCATTER modifier, which describe an instant live memory
layout, that is not transferable and with an unknown and variable layout.

- If you need to know the platform and/or SoC to be able to interpret
  the data, it means the modifier is ill-defined and cannot be used in
  inter-machine communication (e.g. Pipewire).
It's not the case for the DEFAULT and MEM_SAVING modifiers.

The SCATTER modifier is mandatory for the Amlogic G12A and G12B HW video decoder,
but the same HW is capable of displaying the non-SCATTER buffer for example.

Playing devil's advocate, the comment sounds similar to
I915_FORMAT_MOD_{X,Y}_TILED:

 * This format is highly platforms specific and not useful for cross-driver
 * sharing. It exists since on a given platform it does uniquely identify the
 * layout in a simple way for i915-specific userspace.    
Yeah which we regret now. We need to now roll out a new set of
modifiers for at least some of the differences in these on the
modern-ish chips (the old crap is pretty much lost cause anyway).

This was kinda a nasty hack to smooth things over since we have epic
amounts of userspace, but it's really not a great idea (and no one
else really has epic amounts of existing userspace that uses tiling
flags everywhere, this is all new code).
-Daniel

Isn't the statement that this for sharing between producer and decoder
_on the same platform_ a similar clause with the same effect?

What advantage is there to exposing the gory details? For Arm AFBC
it's necessary because IP on the SoC can be (likely to be) from
different vendors with different capabilities.

If this is only for talking between Amlogic IP on the same SoC, and
those devices support all the same "flavours", I don't see what is
gained by making userspace care about internals.    
The trouble is if you mix&match IP cores, and one of them supports
flavours A, B, C and the other C, D, E. But all you have is a single
magic modifier for "whatever the flavour is that soc prefers". So
someone gets to stuff this in DT.
This is not the case here, maybe I should explicit the "DEFAULT" modifier with
a bit like "BASIC" to explicitly define support for the currently defined
DEFAULT mode.

Also eventually, maybe, perhaps ARM does grow up into the
client/server space with add-on pcie graphics, and at least for client
you very often end up with integrated + add-in pcie gpu. At that point
you really can't have magic per-soc modifiers anymore.  
Hi,

I also heard that Pipewire will copy buffers and modifiers verbatim
from one machine to another when streaming across network, assuming
that the same modifier means the same thing on all machines.[Citation needed]
Transferring AFBC buffers doesn't sound like a good idea to me....

If that is something that must not be done with DRM modifiers, then
please contact them and document that.
Sorry, it's waypipe, not pipewire:
https://gitlab.freedesktop.org/mstoeckl/waypipe/
I do think this is very much something we want to make possible. They
might pick a silly modifier (compression modifiers only compress bw, by
necessity the lossless ones have to increase storage space so kinda dumb
thing to push over the network if you don't add .xz or whatever on top).
The AFBC, and Amlogic FBC are not size optimized compressions, but really
layout and memory access optimized compressions, without a proper network
size compression, transferring plain NV12 would be the same.

I'm also hoping that intel's modifiers are definitely the one and only
that we ever screwed up, and we should be getting those fixed in the near
future too.
I'd like too.

So maybe what we should do instead is add a comment to the modifier docs
that this stuff _is_ supposed to be transferrable over networks and work.
Only the "SCATTER" is not transferable, the other options are definitely
transferable, and across 6 families and at least between a minimum of 15
different upstream supported SoCs.

Should it be in the modifier description ? should I add a reserved bit
in the Amlogic modifier space describing it's non-transferable nature ?

-Daniel
Neil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help