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

[Jocelyn Falempe]

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,
>>>
>>
>