[PATCH v2] drm/plane: Add documentation about software color conversion.

[Pekka Paalanen]

On Fri, 25 Aug 2023 10:55:35 +0200
Jocelyn Falempe <jfalempe at redhat.com> wrote:

> After discussions on IRC, the consensus is that the DRM drivers should
> avoid software color conversion, and only advertise the formats supported
> by hardware.
> Update the doc accordingly so that the rule and exceptions are clear for
> everyone.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

Hi,

thanks for revising the patch!

> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 24e7998d1731..7215521afddd 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -140,6 +140,25 @@
>   *     DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have been
>   *     various bugs in this area with inconsistencies between the capability
>   *     flag and per-plane properties.
> + *
> + *     All drivers must support XRGB8888, even if the hardware cannot support

Maybe "should" rather than "must"?

If a driver that never supported XRGB8888 before continues to not
support it, I would not count it as a bug.

> + *     it. This has become the de-facto standard and a lot of user-space assume
> + *     it will be present. If XRGB8888 is not natively supported, then it
> + *     shouldn't be the default for preferred depth or fbdev emulation.
> + *
> + *     DRM drivers should not do software color conversion in dumb buffers, and

This seems to be the opposite of what I was trying to say. In my
opinion:

Dumb buffers are the only type of buffers where software color
conversion could be expected. I would never expect software color
conversion with non-dumb buffers or buffers imported from another
device.

But it has a catch: if software color conversion is needed, the dumb
buffers are better be allocated in sysram. The conversion performance
would likely be abysmal if dumb buffers were allocated from discrete
VRAM.

> + *     only advertise the formats they support in hardware. This is for
> + *     performance reason, and to avoid multiple conversions in userspace and
> + *     kernel space.

Maybe also add:

KMS page flips are generally expected to be very cheap operations.

> + *     But there are two exceptions:
> + *     * To support XRGB8888 if it's not supported by the hardware.
> + *     * Any driver is free to modify its internal representation of the format,
> + *       as long as it doesn't alter the visible content in any way. An example
> + *       would be to drop the padding component from a format to save some memory
> + *       bandwidth.

This is fine as long as no-one can mistake this to refer to in-place
conversion. The driver must not modify the userspace submitted buffer's
contents.

> + *     Extra care should be taken when doing software conversion with
> + *     DRM_CAP_DUMB_PREFER_SHADOW, there are more detailed explanation here:
> + *     https://lore.kernel.org/dri-devel/20230818162415.2185f8e3@eldfell/
>   */
>  
>  static unsigned int drm_num_planes(struct drm_device *dev)
> 
> base-commit: 82d750e9d2f5d0594c8f7057ce59127e701af781


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230825/b94ac2f4/attachment.sig>