On 23/01/2024 13:56, Thomas Zimmermann wrote:
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.
Thanks, I didn't have time to look into this yet.
Best regards,
--
Jocelyn
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_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,
