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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2574 bytes --]

On Fri, 8 Sep 2023 11:21:51 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 25.08.23 um 16:04 schrieb Jocelyn Falempe:
> [...]
> > + *
> > + *     But there are two exceptions only for dumb buffers:
> > + *     * 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, and doesn't
> > + *       modify the user-provided buffer. An example would be to drop the
> > + *       padding component from a format to save some memory bandwidth.  
> 
> I have strong objections to this point, _especially_ as you're 
> apparently trying to sneak this in after our discussion. NAK on this 
> part from my side.
> 
> If you want userspace to be able to use a certain format, then export 
> the corresponding 4cc code. Then let userspace decide what to do about 
> it. If userspace pick a certain format, go with it.

What is the reason for your objection, exactly?

> Hence, no implicit conversion from XRGB888 to RGB888, just because it's 
> possible.

For the particular driver in question though, the conversion allows
using a display resolution that is otherwise not possible. I also hear
it improves performance since 25% less data needs to travel across a
slow bus. There is also so little VRAM, than all dumb buffers need to
be allocated from sysram instead anyway, so a copy is always necessary.

Since XRGB8888 is the one format that is recommended to be supported by
all drivers, I don't see a problem here. Did you test on your
incredibly slow g200 test rig if the conversion ends up hurting instead
of helping performance there?

If it hurts, then I see that you have a good reason to NAK this.

It's hard to imagine how it would hurt, since you always need a copy
from sysram dumb buffers to VRAM - or do you?


Thanks,
pq

> > + *     On most hardware, VRAM read access are slow, so when doing the software
> > + *     conversion, the dumb buffer should be allocated in system RAM in order to
> > + *     have decent performance.
> > + *     Extra care should be taken when doing software conversion with
> > + *     DRM_CAP_DUMB_PREFER_SHADOW, there are more detailed explanations here:
> > + *     https://lore.kernel.org/dri-devel/20230818162415.2185f8e3@eldfell/
> >    */
> >   
> >   static unsigned int drm_num_planes(struct drm_device *dev)
> > 
> > base-commit: 82d750e9d2f5d0594c8f7057ce59127e701af781  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]