Hi
Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
> Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
> drivers that control monochromatic panels, that only have 1 bit per pixel.
>
> The drm_fb_gray8_to_mono_reversed() helper was based on the function that
> does the same in the drivers/gpu/drm/tiny/repaper.c driver.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
> include/drm/drm_format_helper.h | 7 +++
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 0f28dd2bdd72..cdce4b7c25d9 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> return -EINVAL;
> }
> EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
> +{
> + unsigned int xb, i;
> +
> + for (xb = 0; xb < pixels / 8; xb++) {
In practice, all mode widths are multiples of 8 because VGA mandated it.
So it's ok-ish to assume this here. You should probably at least print a
warning somewhere if (pixels % 8 != 0)
> + u8 byte = 0x00;
> +
> + for (i = 0; i < 8; i++) {
> + int x = xb * 8 + i;
> +
> + byte >>= 1;
> + if (src[x] >> 7)
> + byte |= BIT(7);
> + }
> + *dst++ = byte;
> + }
> +}
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> + const struct drm_rect *clip)
There's a bug here. You want to pass in a drm_framebuffer as fourth
argument.
> +{
> +
> + size_t height = drm_rect_height(clip);
> + size_t width = drm_rect_width(clip);
> + unsigned int y;
> + const u8 *gray8 = src;
> + u8 *mono = dst;
> +
> + if (!dst_pitch)
> + dst_pitch = width;
The dst_pitch is given in bytes. You have to device by 8. Here would be
a good place to warn if (width % 8 != 0).
> +
> + for (y = 0; y < height; y++) {
> + drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
> + mono += (dst_pitch / 8);
The dst_pitch is already given in bytes.
> + gray8 += dst_pitch;
'gray8 += fb->pitches[0]' would be correct.
> + }
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @src: XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use this function to convert to the native format.
> + *
> + * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and
> + * then the result is converted from grayscale to reversed monohrome.
> + */
> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> + const struct drm_framebuffer *fb,
> + const struct drm_rect *clip)
> +{
> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
> + return;
> +
> + if (!dst_pitch)
> + dst_pitch = drm_rect_width(clip);
> +
> + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
> + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
Converting from dst into dst can give incorrect results. At some point
we probably want to add restrict qualifiers to these pointers, to help
the compiler with optimizing.
A better approach here is to pull the per-line conversion from
drm_fb_xrgb8888_to_gray8() into a separate helper and implement a
line-by-line conversion here. something like this:
drm_fb_xrgb8888_to_mono_reversed()
{
char *tmp = kmalloc(size of a single line of gray8)
for (heigth) {
drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
src += fb->pitches[0]
dst += dst_pitch;
}
kfree(tmp);
}
Best regards
Thomas
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index b30ed5de0a33..85e551a5cbe6 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> const void *vmap, const struct drm_framebuffer *fb,
> const struct drm_rect *rect);
>
> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> + const struct drm_rect *clip);
> +
> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> + const struct drm_framebuffer *fb,
> + const struct drm_rect *clip);
> +
> #endif /* __LINUX_DRM_FORMAT_HELPER_H */
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hello Thomas,
Thanks a lot for your feedback.
On 2/4/22 16:52, Thomas Zimmermann wrote:
[snip]
>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>> +{
>> + unsigned int xb, i;
>> +
>> + for (xb = 0; xb < pixels / 8; xb++) {
>
> In practice, all mode widths are multiples of 8 because VGA mandated it.
> So it's ok-ish to assume this here. You should probably at least print a
> warning somewhere if (pixels % 8 != 0)
>
Agreed.
[snip]
>> + * DRM doesn't have native monochrome or grayscale support.
>> + * Such drivers can announce the commonly supported XR24 format to userspace
>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>> + * helper function to convert to the native format.
>> + */
>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>> + const struct drm_rect *clip)
>
> There's a bug here. You want to pass in a drm_framebuffer as fourth
> argument.
>
>> +{
>> +
>> + size_t height = drm_rect_height(clip);
>> + size_t width = drm_rect_width(clip);
>> + unsigned int y;
>> + const u8 *gray8 = src;
>> + u8 *mono = dst;
>> +
>> + if (!dst_pitch)
>> + dst_pitch = width;
>
> The dst_pitch is given in bytes. You have to device by 8. Here would be
> a good place to warn if (width % 8 != 0).
>
Ok.
>> +
>> + for (y = 0; y < height; y++) {
>> + drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
>> + mono += (dst_pitch / 8);
>
> The dst_pitch is already given in bytes.
>
Yes, I know but for reversed mono we want only 1/8 of the width since we
are converting from 8 bits per pixel greyscale to 1 bit per pixel mono.
Or am I misunderstanding what you meant ?
>> + gray8 += dst_pitch;
>
> 'gray8 += fb->pitches[0]' would be correct.
>
Ok.
[snip]
>> + */
>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>> + const struct drm_framebuffer *fb,
>> + const struct drm_rect *clip)
>> +{
>> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>> + return;
>> +
>> + if (!dst_pitch)
>> + dst_pitch = drm_rect_width(clip);
>> +
>> + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>> + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
>
> Converting from dst into dst can give incorrect results. At some point
> we probably want to add restrict qualifiers to these pointers, to help
> the compiler with optimizing.
>
> A better approach here is to pull the per-line conversion from
> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a
> line-by-line conversion here. something like this:
>
> drm_fb_xrgb8888_to_mono_reversed()
> {
> char *tmp = kmalloc(size of a single line of gray8)
>
> for (heigth) {
> drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
> drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
>
> src += fb->pitches[0]
> dst += dst_pitch;
> }
>
> kfree(tmp);
> }
>
I see. Yes, that sounds a much better approach. I'll change it in v3.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Hi
Am 04.02.22 um 20:31 schrieb Javier Martinez Canillas:
> Hello Thomas,
>
> Thanks a lot for your feedback.
>
> On 2/4/22 16:52, Thomas Zimmermann wrote:
>
> [snip]
>
>>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>>> +{
>>> + unsigned int xb, i;
>>> +
>>> + for (xb = 0; xb < pixels / 8; xb++) {
>>
>> In practice, all mode widths are multiples of 8 because VGA mandated it.
>> So it's ok-ish to assume this here. You should probably at least print a
>> warning somewhere if (pixels % 8 != 0)
>>
>
> Agreed.
After sending the mail, I realized that some code copies only parts of
the source around; specifically for damage handling. None of this is
aligned to multiple of 8. So the copying could start and end in the
middle of bytes. You'd need a pixel-offset value of some sort.
If you don't want to handle this now, maybe at least detect this case
and put a warning somewhere.
>
> [snip]
>
>>> + * DRM doesn't have native monochrome or grayscale support.
>>> + * Such drivers can announce the commonly supported XR24 format to userspace
>>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>>> + * helper function to convert to the native format.
>>> + */
>>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>>> + const struct drm_rect *clip)
>>
>> There's a bug here. You want to pass in a drm_framebuffer as fourth
>> argument.
>>
>>> +{
>>> +
>>> + size_t height = drm_rect_height(clip);
>>> + size_t width = drm_rect_width(clip);
>>> + unsigned int y;
>>> + const u8 *gray8 = src;
>>> + u8 *mono = dst;
>>> +
>>> + if (!dst_pitch)
>>> + dst_pitch = width;
>>
>> The dst_pitch is given in bytes. You have to device by 8. Here would be
>> a good place to warn if (width % 8 != 0).
>>
>
> Ok.
>
>>> +
>>> + for (y = 0; y < height; y++) {
>>> + drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch);
>>> + mono += (dst_pitch / 8);
>>
>> The dst_pitch is already given in bytes.
>>
>
> Yes, I know but for reversed mono we want only 1/8 of the width since we
> are converting from 8 bits per pixel greyscale to 1 bit per pixel mono.
>
> Or am I misunderstanding what you meant ?
I mean that if there are 80 pixel on a scanline, the value of dst_pitch
is already 10. These pitch values are always given in bytes.
Best regards
Thomas
>
>>> + gray8 += dst_pitch;
>>
>> 'gray8 += fb->pitches[0]' would be correct.
>>
>
> Ok.
>
> [snip]
>
>>> + */
>>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
>>> + const struct drm_framebuffer *fb,
>>> + const struct drm_rect *clip)
>>> +{
>>> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>>> + return;
>>> +
>>> + if (!dst_pitch)
>>> + dst_pitch = drm_rect_width(clip);
>>> +
>>> + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>>> + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
>>
>> Converting from dst into dst can give incorrect results. At some point
>> we probably want to add restrict qualifiers to these pointers, to help
>> the compiler with optimizing.
>>
>> A better approach here is to pull the per-line conversion from
>> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a
>> line-by-line conversion here. something like this:
>>
>> drm_fb_xrgb8888_to_mono_reversed()
>> {
>> char *tmp = kmalloc(size of a single line of gray8)
>>
>> for (heigth) {
>> drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
>> drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
>>
>> src += fb->pitches[0]
>> dst += dst_pitch;
>> }
>>
>> kfree(tmp);
>> }
>>
>
> I see. Yes, that sounds a much better approach. I'll change it in v3.
>
> Best regards,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On Fri, Feb 4, 2022 at 10:53 AM Thomas Zimmermann <[email protected]> wrote:
>
> Hi
>
> Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
> > Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
> > drivers that control monochromatic panels, that only have 1 bit per pixel.
> >
> > The drm_fb_gray8_to_mono_reversed() helper was based on the function that
> > does the same in the drivers/gpu/drm/tiny/repaper.c driver.
> >
> > Signed-off-by: Javier Martinez Canillas <[email protected]>
> > ---
> >
> > (no changes since v1)
> >
> > drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
> > include/drm/drm_format_helper.h | 7 +++
> > 2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index 0f28dd2bdd72..cdce4b7c25d9 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> > return -EINVAL;
> > }
> > EXPORT_SYMBOL(drm_fb_blit_toio);
> > +
> > +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
> > +{
> > + unsigned int xb, i;
> > +
> > + for (xb = 0; xb < pixels / 8; xb++) {
>
> In practice, all mode widths are multiples of 8 because VGA mandated it.
> So it's ok-ish to assume this here. You should probably at least print a
> warning somewhere if (pixels % 8 != 0)
Not sure if it's relevant, but 1366x768 was a fairly popular laptop
resolution. There's even a dedicated drm_mode_fixup_1366x768 in
drm_edid.c. (Would it have killed them to add 2 more horizontal
pixels? Apparently.)
Cheers,
-ilia
Am 04.02.22 um 16:52 schrieb Thomas Zimmermann:
[...]
>> +/**
>> + * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed
>> monochrome
>> + * @dst: reversed monochrome destination buffer
>> + * @dst_pitch: Number of bytes between two consecutive scanlines
>> within dst
>> + * @src: XRGB8888 source buffer
>> + * @fb: DRM framebuffer
>> + * @clip: Clip rectangle area to copy
>> + *
>> + * DRM doesn't have native monochrome support.
>> + * Such drivers can announce the commonly supported XR24 format to
>> userspace
>> + * and use this function to convert to the native format.
>> + *
>> + * This function uses drm_fb_xrgb8888_to_gray8() to convert to
>> grayscale and
>> + * then the result is converted from grayscale to reversed monohrome.
>> + */
>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int
>> dst_pitch, const void *src,
>> + const struct drm_framebuffer *fb,
>> + const struct drm_rect *clip)
>> +{
>> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>> + return;
>> +
>> + if (!dst_pitch)
>> + dst_pitch = drm_rect_width(clip);
>> +
>> + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip);
>> + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip);
>
> Converting from dst into dst can give incorrect results. At some point
> we probably want to add restrict qualifiers to these pointers, to help
> the compiler with optimizing.
>
> A better approach here is to pull the per-line conversion from
> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a
> line-by-line conversion here. something like this:
>
> drm_fb_xrgb8888_to_mono_reversed()
> {
> char *tmp = kmalloc(size of a single line of gray8)
>
> for (heigth) {
> drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...);
> drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...);
Here, I meant 'drm_fb_gray8_to_mono_reversed_line()'
>
> src += fb->pitches[0]
> dst += dst_pitch;
> }
>
> kfree(tmp);
> }
To elaborate, this is an example of what I meant by composable. In the
future, we can generalize this function and hand-in 2 function pointers
the do the conversion with tmp as intermediate buffer. That would work
for any combination of formats that have a common intermediate format.
>
> Best regards
> Thomas
>
>> +}
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
>> diff --git a/include/drm/drm_format_helper.h
>> b/include/drm/drm_format_helper.h
>> index b30ed5de0a33..85e551a5cbe6 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned
>> int dst_pitch, uint32_t dst_for
>> const void *vmap, const struct drm_framebuffer *fb,
>> const struct drm_rect *rect);
>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch,
>> const void *src,
>> + const struct drm_rect *clip);
>> +
>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int
>> dst_pitch, const void *src,
>> + const struct drm_framebuffer *fb,
>> + const struct drm_rect *clip);
>> +
>> #endif /* __LINUX_DRM_FORMAT_HELPER_H */
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi
Am 04.02.22 um 22:02 schrieb Ilia Mirkin:
> On Fri, Feb 4, 2022 at 10:53 AM Thomas Zimmermann <[email protected]> wrote:
>>
>> Hi
>>
>> Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas:
>>> Add support to convert XR24 and 8-bit grayscale to reversed monochrome for
>>> drivers that control monochromatic panels, that only have 1 bit per pixel.
>>>
>>> The drm_fb_gray8_to_mono_reversed() helper was based on the function that
>>> does the same in the drivers/gpu/drm/tiny/repaper.c driver.
>>>
>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>> drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++
>>> include/drm/drm_format_helper.h | 7 +++
>>> 2 files changed, 87 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>>> index 0f28dd2bdd72..cdce4b7c25d9 100644
>>> --- a/drivers/gpu/drm/drm_format_helper.c
>>> +++ b/drivers/gpu/drm/drm_format_helper.c
>>> @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>> return -EINVAL;
>>> }
>>> EXPORT_SYMBOL(drm_fb_blit_toio);
>>> +
>>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels)
>>> +{
>>> + unsigned int xb, i;
>>> +
>>> + for (xb = 0; xb < pixels / 8; xb++) {
>>
>> In practice, all mode widths are multiples of 8 because VGA mandated it.
>> So it's ok-ish to assume this here. You should probably at least print a
>> warning somewhere if (pixels % 8 != 0)
>
> Not sure if it's relevant, but 1366x768 was a fairly popular laptop
> resolution. There's even a dedicated drm_mode_fixup_1366x768 in
> drm_edid.c. (Would it have killed them to add 2 more horizontal
> pixels? Apparently.)
D'oh!
Do you know how the text console looks in this mode? Fonts still expect
a multiple of 8.
Best regards
Thomas
>
> Cheers,
>
> -ilia
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev