[PATCH v7 1/9] drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill

[Thomas Zimmermann]

Hi

Am 17.01.24 um 17:40 schrieb Jocelyn Falempe:
> 
> 
> On 17/01/2024 16:06, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 04.01.24 um 17:00 schrieb Jocelyn Falempe:
>>> This is needed for drm_panic, to draw the font, and fill
>>> the background color, in multiple color format.
>>
>> TBH, I don't like this patch at all. It looks like you're 
>> reimplementing existing functionality for a single use case; 
>> specifically drm_fb_blit().
>>
>> What's wrong with the existing interfaces?

I've spend considerable time to clean up the format-helper code and get 
it into shape. It's not there yet, but on its way. So I'd rather prefer 
to update the existing code for new use cases. Adding a new interface 
for a single use case is something like a leap backwards.

So let's see if we can work out something.

> 
> drm_fb_blit() is good to copy a framebuffer to another, but is clearly 
> unoptimal to draw font.

1) The framebuffer data structure is only there for historical reasons. 
It should be removed from the internal implementation entirely. A first 
patch should go into this in any case. It didn't happened so far, as 
I've been busy with other work.

2) For the public API, I've long wanted to replace framebuffers with 
something more flexible, let's call it drm_pixmap

   struct drm_pixmap {
     struct drm_format_info *format
     unsigned int width, height
     unsigned int pitches[DRM_FORMAT_MAX_PLANES]
     iomap vaddr[DRM_FORMAT_MAX_PLANES]
   };

It's the essence of drm_framebuffer. Let's say there's also an init 
helper drm_pixmap_init_from_framebuffer(pix, fb) that sets up 
everything. The implementation of drm_fb_blit() would look like this:

   drm_fb_blit(...) {

	drm_pixmap pix;
	drm_pixmap_init_from_framebuffer(pix, fb)
	__drm_fb_blit_pixmap( <like drm_fb_blit() but with a pixmap> )
   }

That would require some changes to drivers, but it's only simple 
refactoring.

3) When looking at your patch, there's

   src = font->data + (msg->txt[i] * font->height) * src_stride;

which should be in a helper that sets up the drm_pixmap for a font 
character:

   drm_pixmap_init_from_char(pixmap, c, font_data)

where 'c' equals msg->txt[i]

The text drawing in the panic handler would do something like

   for (msg->txt[i]) {
	drm_pixmap_init_from_char(pixmap, ...)
       	drm_fb_blit_pixmap(...)
   }


> It handles xrgb8888 to any rgb format, and I need monochrome to any rgb 
> format.

4) You're free to add any conversion to drm_fb_blit(). It's supposed to 
handle all available format conversion. With the pixmap-related changes 
outlined above and the actual conversion code, I think that would 
already put characters on the screen.

> I need to convert foreground and background color to the destination 
> format, but using drm_fb_blit() to convert 1 pixel is tedious.

5) I've recently added drm_format_conv_state to the API. It is supposed 
to hold state that is required for the conversion process. I 
specifically had color palettes in mind. Please use the data structure. 
Something like that:

   struct drm_format_conv_state {
	...
	const drm_color_lut *palette;
   }

and in the conversion code:

   void r1_to_rgb() {
	for (x < pixels) {
		rgb = state->palette[r1]
	}
   }

> 
> It also requires an additional memory buffer, and do an additional 
> memory copy that we don't need at all.

6) That memcpy_to_io() not a big deal. You should pre-allocate that 
memory buffer in the panic handler and init the drm_format_conv_state 
with DRM_FORMAT_CONV_STATE_INIT_PREALLOCATED().

> 
> It also has no way to fill a region with the background color.

7) Please add a separate drm_fb_fill() implementation. If you have a 
palette in struct drm_format_conf_state, you can add a helper for each 
destination format that takes a drm_color_lut value as input.

This point is probably worth a separate discussion.

> 
> The last thing, is if I plan to add YUV support, with this 
> implementation, I only need to write one function that convert one 
> pixel. Otherwise I would need to add the drm_fb_r1_to_yuvxxx_line() and 
> drm_fb_r1_to_yuvxxxx() boilerplate.

8) YUVs are multi-plane formats IIRC. So it's likely a bit more 
complicated.And I'm not aware of any current use case for YUV. If the 
framebuffer console doesn't support it, the panic helper probably won't 
either.

Best regards
Thomas

> 
> Best regards,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240119/54a72e84/attachment-0001.sig>