Re: [PATCH v5 1/2] drm/fourcc: Add Synaptics VideoSmart tiled modifiers

[Randy Li]

Sent from my iPad

> On Dec 1, 2022, at 5:39 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
>> On Thu, Dec 01, 2022 at 12:49:16AM +0800, Randy Li wrote:
>> 
>> 
>> Sent from my iPad
>> 
>>>> On Nov 30, 2022, at 7:30 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
>>> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>> On Wed, Nov 30, 2022 at 05:21:48PM +0800, Hsia-Jun Li wrote:
>>>> From: "Hsia-Jun(Randy) Li" <randy.li@xxxxxxxxxxxxx>
>>>> Those modifiers only record the parameters would effort pixel
>>>> layout or memory layout. Whether physical memory page mapping
>>>> is used is not a part of format.
>>>> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@xxxxxxxxxxxxx>
>>>> ---
>>>> include/uapi/drm/drm_fourcc.h | 76 +++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 76 insertions(+)
>>>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>>>> index bc056f2d537d..e0905f573f43 100644
>>>> --- a/include/uapi/drm/drm_fourcc.h
>>>> +++ b/include/uapi/drm/drm_fourcc.h
>>>> @@ -407,6 +407,7 @@ extern "C" {
>>>> #define DRM_FORMAT_MOD_VENDOR_ARM     0x08
>>>> #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
>>>> #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
>>>> +#define DRM_FORMAT_MOD_VENDOR_SYNAPTICS 0x0b
>>>> /* add more to the end as needed */
>>>> @@ -1507,6 +1508,81 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
>>>> #define AMD_FMT_MOD_CLEAR(field) \
>>>>     (~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))
>>>> +/*
>>>> + * Synaptics VideoSmart modifiers
>>>> + *
>>>> + * Tiles could be arranged in Groups of Tiles (GOTs), it is a small tile
>>>> + * within a tile. GOT size and layout varies based on platform and
>>>> + * performance concern.
>>>> + *
>>>> + * Besides, an 8 length 4 bytes arrary (32 bytes) would be need to store
>>>> + * some compression parameters for a compression metadata plane.
>>>> + *
>>>> + * Further information can be found in
>>>> + * Documentation/gpu/synaptics.rst
>>>> + *
>>>> + *       Macro
>>>> + * Bits  Param Description
>>>> + * ----  ----- -----------------------------------------------------------------
>>>> + *
>>>> + *  7:0  f     Scan direction description.
>>>> + *
>>>> + *               0 = Invalid
>>>> + *               1 = V4, the scan would always start from vertical for 4 pixel
>>>> + *                   then move back to the start pixel of the next horizontal
>>>> + *                   direction.
>>>> + *               2 = Reserved for future use.
>>>> + *
>>>> + * 15:8  m     The times of pattern repeat in the right angle direction from
>>>> + *             the first scan direction.
>>>> + *
>>>> + * 19:16 p     The padding bits after the whole scan, could be zero.
>>>> + *
>>>> + * 20:20 g     GOT packing flag.
>>>> + *
>>>> + * 23:21 -     Reserved for future use.  Must be zero.
>>> Can you pls fold all the future use reservations into the top end?
>> You see we could put more related flag in each of reserved area.
>> Here is for the group of tiles flag.
>> Bit 35 to 32 could be used for describing the dimension of the group of tiles.
> 
> Oh also on the dimension thing, this is the tile size and has nothing to
> do with the overall buffer size, right?
I don’t think you could have a insufficient tile, that applies to the group of tile.
> Because the overall buffer size is
> meant to be carried in separate metadata (like the drm_framebuffer
> structure or ADDFB2 ioctl data). drm fourcc/modifier assume that height,
> width, offset and stride are specified per plane already (unless the
> auxiary plane has a fixed layout and is not tracked as a separate plane
> for this format).
One thing I noticed here, there is no way to tell the buffer size that user should request/allocate from the drm API. It needs to be calculated in the userspace unless you would use the custom ioctl.
> 
>>> Also I
>>> think it'd be good to at least reserve maybe the top 8 bits or so for a
>>> synaptics specific format indicator, so that it's easier to extend this in
>>> the future ...
>> I think the  bit 56 to 63 are used for storing the vendor id. That is why I didn’t include them below. Or you mean the bit 7 to 0?
>> Do yo
> 
> Yeah there's 8 bit vendor id, but you could reserve another 8 bit at the
> top (so 48:55 or something like that) to enumerate within the synaptics
> space. Just to future proof the schema, because experience says that hw
> engineers absolutely do love to change this stuff eventually.
I left the whole 37:55.
> -Daniel
> 
>>> -Daniel
>>>> + *
>>>> + * 27:24 h     log2(horizontal) of pixels, in GOTs.
>>>> + *
>>>> + * 31:28 v     log2(vertical) of pixels, in GOTs.
>>>> + *
>>>> + * 35:32 -     Reserved for future use.  Must be zero.
>>>> + *
>>>> + * 36:36 c     Compression flag.
>>>> + *
>>>> + * 55:37 -     Reserved for future use.  Must be zero.
>>>> + *
>>>> + */
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_V4_TILED         fourcc_mod_code(SYNAPTICS, 1)
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_MTR_LINEAR_2D(f, m, p, g, h, v, c) \
>>>> +     fourcc_mod_code(SYNAPTICS, ((__u64)((f) & 0xff) | \
>>>> +                              ((__u64)((m) & 0xff) << 8) | \
>>>> +                              ((__u64)((p) & 0xf) << 16) | \
>>>> +                              ((__u64)((g) & 0x1) << 20) | \
>>>> +                              ((__u64)((h) & 0xf) << 24) | \
>>>> +                              ((__u64)((v) & 0xf) << 28) | \
>>>> +                              ((__u64)((c) & 0x1) << 36)))
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_V4H1 \
>>>> +     DRM_FORMAT_MOD_SYNA_MTR_LINEAR_2D(1, 1, 0, 0, 0, 0, 0)
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_V4H3P8 \
>>>> +     DRM_FORMAT_MOD_SYNA_MTR_LINEAR_2D(1, 3, 8, 0, 0, 0, 0)
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_V4H1_64L4_COMPRESSED \
>>>> +     DRM_FORMAT_MOD_SYNA_MTR_LINEAR_2D(1, 1, 0, 1, 6, 2, 1)
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_V4H3P8_64L4_COMPRESSED \
>>>> +     DRM_FORMAT_MOD_SYNA_MTR_LINEAR_2D(1, 3, 8, 1, 6, 2, 1)
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_V4H1_128L128_COMPRESSED \
>>>> +     DRM_FORMAT_MOD_SYNA_MTR_LINEAR_2D(1, 1, 0, 1, 7, 7, 1)
>>>> +
>>>> +#define DRM_FORMAT_MOD_SYNA_V4H3P8_128L128_COMPRESSED \
>>>> +     DRM_FORMAT_MOD_SYNA_MTR_LINEAR_2D(1, 3, 8, 1, 7, 7, 1)
>>>> +
>>>> #if defined(__cplusplus)
>>>> }
>>>> #endif
>>>> --
>>>> 2.37.3
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__blog.ffwll.ch&d=DwIBAg&c=7dfBJ8cXbWjhc0BhImu8wVIoUFmBzj1s88r8EGyM0UY&r=P4xb2_7biqBxD4LGGPrSV6j-jf3C3xlR7PXU-mLTeZE&m=d1mgHWc-ItDXK9dSnz0WGYs9xoXTTk9LqbifMtn2LOxmaHHsc4ieCoE78BFkHI1i&s=8ptqPzTUwb3X3fBSyQA6nVAA6DchubUUsMRgmLIp1lY&e=
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=http-3A__blog.ffwll.ch&d=DwIDaQ&c=7dfBJ8cXbWjhc0BhImu8wVIoUFmBzj1s88r8EGyM0UY&r=reqwguHMk9_Krd2xyybLKUi7qRQg3FIHG-6xLimUZv8&m=tmN4FcUbfHQ74lDlCJfhwkr5WqNNCY6CzNFzTM6X_i1HSp9Nt2WlyXv-Wu-yNI9y&s=yEyPRethHiPbmj0QHoJMsmb1Pj2IN7B0J8_vlBvy0wk&e=