Re: [PATCH v6 2/6] drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()

[Javier Martinez Canillas]

Hello Geert,

Thanks a lot for your feedback.

On 3/8/22 17:13, Geert Uytterhoeven wrote:

[snip]

>> +
>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned int pixels,
> 
> "pixels" is not the number of pixels to process, but the number of
> bytes in the monochrome destination buffer.
>

Right, that parameter name is misleading / incorrect indeed. Probably
should be changed by dst_pitch or dst_stride.
 
>> +                                              unsigned int start_offset, unsigned int end_len)
>> +{
>> +       unsigned int xb, i;
>> +
>> +       for (xb = 0; xb < pixels; xb++) {
>> +               unsigned int start = 0, end = 8;
>> +               u8 byte = 0x00;
>> +
>> +               if (xb == 0 && start_offset)
>> +                       start = start_offset;
>> +
>> +               if (xb == pixels - 1 && end_len)
>> +                       end = end_len;
>> +
>> +               for (i = start; i < end; i++) {
>> +                       unsigned int x = xb * 8 + i;
>> +
>> +                       byte >>= 1;
>> +                       if (src[x] >> 7)
>> +                               byte |= BIT(7);
>> +               }
>> +               *dst++ = byte;
>> +       }
> 
> The above is IMHO very hard to read.
> I think it can be made simpler by passing the total number of pixels
> to process instead of "pixels" (which is bytes) and "end_len".
>

Agreed that's hard to read. I think is better if you propose a patch
with your idea to make it simpler.
 
[snip]

>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *vaddr,
>> +                                     const struct drm_framebuffer *fb, const struct drm_rect *clip)
>> +{
>> +       unsigned int linepixels = drm_rect_width(clip);
>> +       unsigned int lines = clip->y2 - clip->y1;
> 
> drm_rect_height(clip)?
>

Yes, unsure why didn't use it since used drm_rect_width() for the width.
 
>> +       unsigned int cpp = fb->format->cpp[0];
>> +       unsigned int len_src32 = linepixels * cpp;
>> +       struct drm_device *dev = fb->dev;
>> +       unsigned int start_offset, end_len;
>> +       unsigned int y;
>> +       u8 *mono = dst, *gray8;
>> +       u32 *src32;
>> +
>> +       if (drm_WARN_ON(dev, fb->format->format != DRM_FORMAT_XRGB8888))
>> +               return;
>> +
>> +       /*
>> +        * The reversed mono destination buffer contains 1 bit per pixel
>> +        * and destination scanlines have to be in multiple of 8 pixels.
>> +        */
>> +       if (!dst_pitch)
>> +               dst_pitch = DIV_ROUND_UP(linepixels, 8);
> 
> This is not correct when clip->x1 is not a multiple of 8 pixels.
> Should be:
> 
>     DIV_ROUND_UP(linepixels + clip->x1 % 8, 8);
>

Agreed.
 
>> +
>> +       drm_WARN_ONCE(dev, dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
> 
> This triggers for me: dst_pitch = 1.
> Which is perfectly fine, when flashing an 8-pixel wide cursor ;-)
>

Indeed. I think we should just drop that warn.

Do you want me to post patches for all these or would you do it
when simplifying the drm_fb_gray8_to_mono_reversed_line() logic ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat