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

[Jocelyn Falempe]

On 25/08/2023 14:03, Pekka Paalanen wrote:
> 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.

Yes, that's fair.
> 
>> + *     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.

Sorry, I completely misunderstood this part, I will fix that in v3

> 
> 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.

I think that depends on hardware. Sometime VRAM can be faster than 
system RAM, even when accessed from CPU, like in Haswell Iris Pro GPU, 
which have 128M of eDRAM on the chip: 
https://www.anandtech.com/show/6993/intel-iris-pro-5200-graphics-review-core-i74950hq-tested/3
(it's not a good example, because the eDRAM is used as an L4 cache, but 
they may have dedicated it to the GPU).
But I agree that for the most common case of slow VRAM (read) access, 
doing the color conversion directly on it is not recommended.

> 
>> + *     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.

Yes I will add this.
> 
>> + *     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.

I think that's what "alter the visible content" means, modifying the 
user-space buffer would be "visible" from that user.
But I will still add this precision in v3.

> 
>> + *     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


Thanks,

-- 

Jocelyn