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

[Pekka Paalanen <ppaalanen@gmail.com>]

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

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

> Hi
> 
> Am 08.09.23 um 13:16 schrieb Pekka Paalanen:
> > 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?  
> 
> I have a number of concerns. My point it not that we shouldn't optimize. 
> I just don't want it in the kernel. Mgag200 can export DRM_FORMAT_RGB888 
> for userspace to use.
> 
> AFAICT the main argument against userspace is that Mesa doesn't like 
> 3-byte pixels. But I don't see how this conversion cannot be a 
> post-processing step within Mesa: do the rendering in RGB32 and then 
> convert to a framebuffer in RGB24. Userspace can do that more 
> efficiently than the kernel. This has all of the upsides of reduced 
> bandwidth, but none of the downsides of kernel code. Applications and/or 
> Mesa would be in control of the buffer format and apply the optimization 
> where it makes sense. And it would be available for all drivers that are 
> similar to mgag200.
> 
> My main point is simplicity of the driver: I prefer the driver to be 
> simple without unnecessary indirection or overhead. Optimizations like 
> these my or may not work on a given system with a certain workload. I'd 
> better leave this heuristic to userspace.
> 
> Another point of concern is CPU consumption: Slow I/O buses may stall 
> the display thread, but the CPU could do something else in the meantime. 
> Doing format conversion on the CPU prevents that, hence affecting other 
> parts of the system negatively. Of course, that's more of a gut feeling 
> than hard data.
> 
> Please note that the kernel's conversion code uses memory allocation of 
> intermediate buffers. We even recently had a discussion about allocation 
> overhead during display updates. Userspace can surely do a better job at 
> keeping such buffers around.
> 
> And finally a note the hardware itself: on low-end hardware like those 
> Matrox chips, just switch to RGB16. That will be pretty and fast enough 
> for these chips' server systems. Anyone who cares about fast and 
> beautiful should buy a real graphics card.

Fair enough.

Did you consider that every frame will be copied twice: once in
userspace to get RGB888, then again by the driver into VRAM, since dumb
buffers reside in sysram?

RGB565 would probably go the same route I guess.

I suspect the intermediate buffers (dumb buffers in this case) need to
be full frame size rather than a scanline, too. I'm not sure why the
driver would need any scratch buffers beyond a couple dozen bytes while
doing a software conversion, just enough to have the lowest common
multiple of 3 bytes and optimal bus write width.


Thanks,
pq

> 
> Best regards
> Thomas
> 
> > 
> > 
> > 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 --]