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

[Thomas Zimmermann]

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_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/20240123/ecdd0f59/attachment-0001.sig>