Hi,FYI for points 1 and 2, I'm typing up a patchset that introduces drm_pixmap for the source buffer. I'll post it when I have something ready.
Best regards Thomas Am 19.01.24 um 11:58 schrieb 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_pixmapstruct 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 ThomasBest 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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
