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
