Re: [PATCH v3 2/7] drm/format-helper: Add drm_fb_{xrgb8888, gray8}_to_mono_reversed()

[Javier Martinez Canillas <javierm@redhat.com>]

On 2/9/22 16:21, Thomas Zimmermann wrote:

[snip]

>>
>> It could be taken from this patch-set anyways if someone wants to wire the
>> needed support for R8.
> 
> I think, policy is to not keep unused code around.
>

Ok, I'll drop it then. We can include it again when adding R8 formats.
 
[snip]

>>> If might be faster to allocate both buffers in one step and set the
>>> pointers into the allocated buffer.
>>>
>>
>> Not sure I got this. Do you mean to have a single buffer with length
>> linepixels + len_src32 and point src32 and gray8 to the same buffer ?
> 
> That's the idea. I don't know the exact overhead for kalloc(), but at 
> least the in userspace, malloc() in hot code paths is not a good idea. 
> There's usually some searching for free space involved.
>

Sure, let's do it in one allocation then and I'll add some comments to
make easier for people to follow the code.
 
> In the long term, we could add a field in struct drm_framebuffer to keep 
> such buffers around for reuse.
> 
>>
>>>> +
>>>> +	/*
>>>> +	 * For damage handling, it is possible that only parts of the source
>>>> +	 * buffer is copied and this could lead to start and end pixels that
>>>> +	 * are not aligned to multiple of 8.
>>>> +	 *
>>>> +	 * Calculate if the start and end pixels are not aligned and set the
>>>> +	 * offsets for the reversed mono line conversion function to adjust.
>>>> +	 */
>>>> +	start_offset = clip->x1 % 8;
>>>> +	end_offset = clip->x2 % 8;
>>>
>>> end_len, again. If you have 1 single bit set in the final byte, the
>>> offset is 0, but the length is 1.
>>>
>>
>> Agreed, will change it too.
> 
> Feel free to add my
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>

Thanks!

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat