[PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

	Hi all,

The native display format of ssd1306 OLED displays is monochrome
light-on-dark (R1).  This patch series adds support for the R1 buffer
format to both the ssd130x DRM driver and the FB helpers, so monochrome
applications (including fbdev emulation and the text console) can avoid
the overhead of back-and-forth conversions between R1 and XR24.

This series is based on drm-misc/for-linux-next with [1] applied, and
consists of 4 parts:
  - Patches 1-3 contain miscellaneous fixes,
  - Patch 4 adds R1 support to the ssd130x DRM driver,
  - Patches 5-6 update the DRM client and FB helper code to avoid
    calling drm_mode_legacy_fb_format() where the exact buffer format is
    already known, to prepare for R1 support,
  - Patch 7 adds support for R1 to fbdev emulation and the text console,
  - Patch 8 switches ssd130x to R1 for fbdev emulation and the text
    console.

This has been tested on an Adafruit FeatherWing 128x32 OLED, connected
to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
softcore, using the fbdev text console.

Thanks for your comments!

P.S. Note that the biggest hurdle was the copious use of the
     drm_mode_legacy_fb_format() helper in various places.  This helper
     cannot decide between C1 and R1 without knowledge of the
     capabilities of the full display pipeline.  Instead of
     special-casing its return value in three callers, I did so in only
     one place, and got rid of two of these calls in the call chain.
     I think Thomas' grand plan is to replace preferred_{bpp,depth} by a
     preferred fourcc format? That would simplify things a lot...

[1] "[PATCH] drm/ssd130x: Change pixel format used to compute the buffer
    size"
    https://lore.kernel.org/all/20230713085859.907127-1-javierm@redhat.com

Geert Uytterhoeven (8):
  drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()
  drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8
  [RFC] drm/ssd130x: Bail out early if data_array is not yet available
  drm/ssd130x: Add support for DRM_FORMAT_R1
  drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()
  drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size
  drm/fb-helper: Add support for DRM_FORMAT_R1
  drm/ssd130x: Switch preferred_bpp/depth to 1

 drivers/gpu/drm/drm_client.c        | 13 +++---
 drivers/gpu/drm/drm_dumb_buffers.c  |  3 +-
 drivers/gpu/drm/drm_fb_helper.c     | 42 ++++++++++++++-----
 drivers/gpu/drm/drm_fbdev_generic.c |  9 ++---
 drivers/gpu/drm/solomon/ssd130x.c   | 62 ++++++++++++++++++++---------
 include/drm/drm_fb_helper.h         |  2 +
 6 files changed, 87 insertions(+), 44 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

	Hi all,

The native display format of ssd1306 OLED displays is monochrome
light-on-dark (R1).  This patch series adds support for the R1 buffer
format to both the ssd130x DRM driver and the FB helpers, so monochrome
applications (including fbdev emulation and the text console) can avoid
the overhead of back-and-forth conversions between R1 and XR24.

This series is based on drm-misc/for-linux-next with [1] applied, and
consists of 4 parts:
  - Patches 1-3 contain miscellaneous fixes,
  - Patch 4 adds R1 support to the ssd130x DRM driver,
  - Patches 5-6 update the DRM client and FB helper code to avoid
    calling drm_mode_legacy_fb_format() where the exact buffer format is
    already known, to prepare for R1 support,
  - Patch 7 adds support for R1 to fbdev emulation and the text console,
  - Patch 8 switches ssd130x to R1 for fbdev emulation and the text
    console.

This has been tested on an Adafruit FeatherWing 128x32 OLED, connected
to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
softcore, using the fbdev text console.

Thanks for your comments!

P.S. Note that the biggest hurdle was the copious use of the
     drm_mode_legacy_fb_format() helper in various places.  This helper
     cannot decide between C1 and R1 without knowledge of the
     capabilities of the full display pipeline.  Instead of
     special-casing its return value in three callers, I did so in only
     one place, and got rid of two of these calls in the call chain.
     I think Thomas' grand plan is to replace preferred_{bpp,depth} by a
     preferred fourcc format? That would simplify things a lot...

[1] "[PATCH] drm/ssd130x: Change pixel format used to compute the buffer
    size"
    https://lore.kernel.org/all/20230713085859.907127-1-javierm@redhat.com

Geert Uytterhoeven (8):
  drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()
  drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8
  [RFC] drm/ssd130x: Bail out early if data_array is not yet available
  drm/ssd130x: Add support for DRM_FORMAT_R1
  drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()
  drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size
  drm/fb-helper: Add support for DRM_FORMAT_R1
  drm/ssd130x: Switch preferred_bpp/depth to 1

 drivers/gpu/drm/drm_client.c        | 13 +++---
 drivers/gpu/drm/drm_dumb_buffers.c  |  3 +-
 drivers/gpu/drm/drm_fb_helper.c     | 42 ++++++++++++++-----
 drivers/gpu/drm/drm_fbdev_generic.c |  9 ++---
 drivers/gpu/drm/solomon/ssd130x.c   | 62 ++++++++++++++++++++---------
 include/drm/drm_fb_helper.h         |  2 +
 6 files changed, 87 insertions(+), 44 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Geert Uytterhoeven]

The page height must be taken into account only for vertical coordinates
and heights, not for horizontal coordinates and widths.

Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index afb08a8aa9fcdaf2..b4c376962629580b 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	rect->y1 = round_down(rect->y1, page_height);
 	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
 
-	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
+	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
 	if (ret)
-- 
2.34.1

[PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Geert Uytterhoeven]

The page height must be taken into account only for vertical coordinates
and heights, not for horizontal coordinates and widths.

Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index afb08a8aa9fcdaf2..b4c376962629580b 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	rect->y1 = round_down(rect->y1, page_height);
 	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
 
-	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
+	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
 	if (ret)
-- 
2.34.1

Re: [PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

Thanks for your patch!

> The page height must be taken into account only for vertical coordinates
> and heights, not for horizontal coordinates and widths.
>
> Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index afb08a8aa9fcdaf2..b4c376962629580b 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>  	rect->y1 = round_down(rect->y1, page_height);
>  	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
>  
> -	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
> +	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
>

That's true for ssd130x controllers that use R1, but when doing that
change one of my goals was to prepare the driver for supporting the
ssd132x family that use a 16-grayscale pixel format (R4).

For those controllers, the pixels are encoded in 4-bit and each page
has two pixels. So for those controllers the dst_pitch will need to
be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is
not 8 in that case.

So I would prefer to skip this patch from your set, because otherwise
we will need to revert it when adding support for SSD132x controllers.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 11:34 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > The page height must be taken into account only for vertical coordinates
> > and heights, not for horizontal coordinates and widths.
> >
> > Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/drivers/gpu/drm/solomon/ssd130x.c
> > +++ b/drivers/gpu/drm/solomon/ssd130x.c
> > @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
> >       rect->y1 = round_down(rect->y1, page_height);
> >       rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
> >
> > -     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
> > +     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> >
>
> That's true for ssd130x controllers that use R1, but when doing that
> change one of my goals was to prepare the driver for supporting the
> ssd132x family that use a 16-grayscale pixel format (R4).
>
> For those controllers, the pixels are encoded in 4-bit and each page
> has two pixels. So for those controllers the dst_pitch will need to
> be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is
> not 8 in that case.
>
> So I would prefer to skip this patch from your set, because otherwise
> we will need to revert it when adding support for SSD132x controllers.

My point is that the 8 as used here is related to the number of bits per pixel,
not to the page height.  The page height might also be impacted by the
number of bits per pixel, but that is orthogonal.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 11:34 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > The page height must be taken into account only for vertical coordinates
> > and heights, not for horizontal coordinates and widths.
> >
> > Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/drivers/gpu/drm/solomon/ssd130x.c
> > +++ b/drivers/gpu/drm/solomon/ssd130x.c
> > @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
> >       rect->y1 = round_down(rect->y1, page_height);
> >       rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
> >
> > -     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
> > +     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> >
>
> That's true for ssd130x controllers that use R1, but when doing that
> change one of my goals was to prepare the driver for supporting the
> ssd132x family that use a 16-grayscale pixel format (R4).
>
> For those controllers, the pixels are encoded in 4-bit and each page
> has two pixels. So for those controllers the dst_pitch will need to
> be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is
> not 8 in that case.
>
> So I would prefer to skip this patch from your set, because otherwise
> we will need to revert it when adding support for SSD132x controllers.

My point is that the 8 as used here is related to the number of bits per pixel,
not to the page height.  The page height might also be impacted by the
number of bits per pixel, but that is orthogonal.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,
>
> On Fri, Jul 14, 2023 at 11:34 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> > The page height must be taken into account only for vertical coordinates
>> > and heights, not for horizontal coordinates and widths.
>> >
>> > Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data")
>> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
>> > --- a/drivers/gpu/drm/solomon/ssd130x.c
>> > +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> > @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>> >       rect->y1 = round_down(rect->y1, page_height);
>> >       rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
>> >
>> > -     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
>> > +     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
>> >
>>
>> That's true for ssd130x controllers that use R1, but when doing that
>> change one of my goals was to prepare the driver for supporting the
>> ssd132x family that use a 16-grayscale pixel format (R4).
>>
>> For those controllers, the pixels are encoded in 4-bit and each page
>> has two pixels. So for those controllers the dst_pitch will need to
>> be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is
>> not 8 in that case.
>>
>> So I would prefer to skip this patch from your set, because otherwise
>> we will need to revert it when adding support for SSD132x controllers.
>
> My point is that the 8 as used here is related to the number of bits per pixel,
> not to the page height.  The page height might also be impacted by the
> number of bits per pixel, but that is orthogonal.
>

Ah, I see what you mean. Yes, you are right. We can later add a
different variable when adding support for controllers using R4.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,
>
> On Fri, Jul 14, 2023 at 11:34 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> > The page height must be taken into account only for vertical coordinates
>> > and heights, not for horizontal coordinates and widths.
>> >
>> > Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data")
>> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
>> > --- a/drivers/gpu/drm/solomon/ssd130x.c
>> > +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> > @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>> >       rect->y1 = round_down(rect->y1, page_height);
>> >       rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
>> >
>> > -     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
>> > +     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
>> >
>>
>> That's true for ssd130x controllers that use R1, but when doing that
>> change one of my goals was to prepare the driver for supporting the
>> ssd132x family that use a 16-grayscale pixel format (R4).
>>
>> For those controllers, the pixels are encoded in 4-bit and each page
>> has two pixels. So for those controllers the dst_pitch will need to
>> be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is
>> not 8 in that case.
>>
>> So I would prefer to skip this patch from your set, because otherwise
>> we will need to revert it when adding support for SSD132x controllers.
>
> My point is that the 8 as used here is related to the number of bits per pixel,
> not to the page height.  The page height might also be impacted by the
> number of bits per pixel, but that is orthogonal.
>

Ah, I see what you mean. Yes, you are right. We can later add a
different variable when adding support for controllers using R4.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

Hello Geert,

> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>

[...]

>>
>> My point is that the 8 as used here is related to the number of bits per pixel,
>> not to the page height.  The page height might also be impacted by the
>> number of bits per pixel, but that is orthogonal.
>>
>
> Ah, I see what you mean. Yes, you are right. We can later add a
> different variable when adding support for controllers using R4.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

Pushed to drm-misc (drm-misc-next) since this fix is independent of the
rest of the patches. Thanks!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/8] drm/ssd130x: Fix pitch calculation in ssd130x_fb_blit_rect()

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

Hello Geert,

> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>

[...]

>>
>> My point is that the 8 as used here is related to the number of bits per pixel,
>> not to the page height.  The page height might also be impacted by the
>> number of bits per pixel, but that is orthogonal.
>>
>
> Ah, I see what you mean. Yes, you are right. We can later add a
> different variable when adding support for controllers using R4.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

Pushed to drm-misc (drm-misc-next) since this fix is independent of the
rest of the patches. Thanks!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 2/8] drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8

[Geert Uytterhoeven]

drm_mode_create_dumb() calculates the number of characters per pixel
from the number of bits per pixel by rounding up, which is not correct
as the actual value of cpp may be non-integer.  While we do not need to
care here about complex formats like YUV, bpp < 8 is a valid use case.

  - The overflow check for the buffer width is not correct if bpp < 8.
    However, it doesn't hurt, as widths larger than U32_MAX / 8 should
    not happen for real anyway.  Add a comment to clarify.
  - Calculating the stride from the number of characters per pixel is
    not correct.  Fix this by calculating it from the number of bits per
    pixel instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Why is drm_mode_create_dumb.size __u64?  The test for "args->height >
U32_MAX / stride" rejects all sizes not fitting in __u32 anyway.
---
 drivers/gpu/drm/drm_dumb_buffers.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 70032bba1c97e787..21a04c32a5e3d785 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev,
 	/* overflow checks for 32bit size calculations */
 	if (args->bpp > U32_MAX - 8)
 		return -EINVAL;
+	/* Incorrect (especially if bpp < 8), but doesn't hurt much */
 	cpp = DIV_ROUND_UP(args->bpp, 8);
 	if (cpp > U32_MAX / args->width)
 		return -EINVAL;
-	stride = cpp * args->width;
+	stride = DIV_ROUND_UP(args->bpp * args->width, 8);
 	if (args->height > U32_MAX / stride)
 		return -EINVAL;
 
-- 
2.34.1

[PATCH 2/8] drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8

[Geert Uytterhoeven]

drm_mode_create_dumb() calculates the number of characters per pixel
from the number of bits per pixel by rounding up, which is not correct
as the actual value of cpp may be non-integer.  While we do not need to
care here about complex formats like YUV, bpp < 8 is a valid use case.

  - The overflow check for the buffer width is not correct if bpp < 8.
    However, it doesn't hurt, as widths larger than U32_MAX / 8 should
    not happen for real anyway.  Add a comment to clarify.
  - Calculating the stride from the number of characters per pixel is
    not correct.  Fix this by calculating it from the number of bits per
    pixel instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Why is drm_mode_create_dumb.size __u64?  The test for "args->height >
U32_MAX / stride" rejects all sizes not fitting in __u32 anyway.
---
 drivers/gpu/drm/drm_dumb_buffers.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 70032bba1c97e787..21a04c32a5e3d785 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev,
 	/* overflow checks for 32bit size calculations */
 	if (args->bpp > U32_MAX - 8)
 		return -EINVAL;
+	/* Incorrect (especially if bpp < 8), but doesn't hurt much */
 	cpp = DIV_ROUND_UP(args->bpp, 8);
 	if (cpp > U32_MAX / args->width)
 		return -EINVAL;
-	stride = cpp * args->width;
+	stride = DIV_ROUND_UP(args->bpp * args->width, 8);
 	if (args->height > U32_MAX / stride)
 		return -EINVAL;
 
-- 
2.34.1

Re: [PATCH 2/8] drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> drm_mode_create_dumb() calculates the number of characters per pixel
> from the number of bits per pixel by rounding up, which is not correct
> as the actual value of cpp may be non-integer.  While we do not need to
> care here about complex formats like YUV, bpp < 8 is a valid use case.
>
>   - The overflow check for the buffer width is not correct if bpp < 8.
>     However, it doesn't hurt, as widths larger than U32_MAX / 8 should
>     not happen for real anyway.  Add a comment to clarify.
>   - Calculating the stride from the number of characters per pixel is
>     not correct.  Fix this by calculating it from the number of bits per
>     pixel instead.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Why is drm_mode_create_dumb.size __u64?  The test for "args->height >

I don't think can be changed since is a DRM_IOCTL_MODE_CREATE_DUMB uAPI ?

> U32_MAX / stride" rejects all sizes not fitting in __u32 anyway.

Hmm, wonder if should be U64_MAX instead ?

> ---
>  drivers/gpu/drm/drm_dumb_buffers.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index 70032bba1c97e787..21a04c32a5e3d785 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev,
>  	/* overflow checks for 32bit size calculations */
>  	if (args->bpp > U32_MAX - 8)
>  		return -EINVAL;
> +	/* Incorrect (especially if bpp < 8), but doesn't hurt much */
>  	cpp = DIV_ROUND_UP(args->bpp, 8);
>  	if (cpp > U32_MAX / args->width)
>  		return -EINVAL;
> -	stride = cpp * args->width;
> +	stride = DIV_ROUND_UP(args->bpp * args->width, 8);
>  	if (args->height > U32_MAX / stride)
>  		return -EINVAL;
>  

Good catch.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/8] drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> drm_mode_create_dumb() calculates the number of characters per pixel
> from the number of bits per pixel by rounding up, which is not correct
> as the actual value of cpp may be non-integer.  While we do not need to
> care here about complex formats like YUV, bpp < 8 is a valid use case.
>
>   - The overflow check for the buffer width is not correct if bpp < 8.
>     However, it doesn't hurt, as widths larger than U32_MAX / 8 should
>     not happen for real anyway.  Add a comment to clarify.
>   - Calculating the stride from the number of characters per pixel is
>     not correct.  Fix this by calculating it from the number of bits per
>     pixel instead.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Why is drm_mode_create_dumb.size __u64?  The test for "args->height >

I don't think can be changed since is a DRM_IOCTL_MODE_CREATE_DUMB uAPI ?

> U32_MAX / stride" rejects all sizes not fitting in __u32 anyway.

Hmm, wonder if should be U64_MAX instead ?

> ---
>  drivers/gpu/drm/drm_dumb_buffers.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index 70032bba1c97e787..21a04c32a5e3d785 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev,
>  	/* overflow checks for 32bit size calculations */
>  	if (args->bpp > U32_MAX - 8)
>  		return -EINVAL;
> +	/* Incorrect (especially if bpp < 8), but doesn't hurt much */
>  	cpp = DIV_ROUND_UP(args->bpp, 8);
>  	if (cpp > U32_MAX / args->width)
>  		return -EINVAL;
> -	stride = cpp * args->width;
> +	stride = DIV_ROUND_UP(args->bpp * args->width, 8);
>  	if (args->height > U32_MAX / stride)
>  		return -EINVAL;
>  

Good catch.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH/RFC 3/8] drm/ssd130x: Bail out early if data_array is not yet available

[Geert Uytterhoeven]

Calling ssd130x_buf_alloc() from ssd130x_encoder_helper_atomic_enable()
is too late, causing a NULL pointer dereference:

    ssd130x_update_rect.isra.0+0x13c/0x340
    ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
    drm_atomic_helper_commit_planes+0xfc/0x27c

Work around that by checking if data_array is valid.

Obviously this needs a better fix...

Fixes: 49d7d581ceaf4cf8 ("drm/ssd130x: Don't allocate buffers on each plane update")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index b4c376962629580b..8ef5f61854fd7340 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -481,6 +481,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *
 	u32 array_idx = 0;
 	int ret, i, j, k;
 
+if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); return 0; }
 	drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
 
 	/*
-- 
2.34.1

[PATCH/RFC 3/8] drm/ssd130x: Bail out early if data_array is not yet available

[Geert Uytterhoeven]

Calling ssd130x_buf_alloc() from ssd130x_encoder_helper_atomic_enable()
is too late, causing a NULL pointer dereference:

    ssd130x_update_rect.isra.0+0x13c/0x340
    ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
    drm_atomic_helper_commit_planes+0xfc/0x27c

Work around that by checking if data_array is valid.

Obviously this needs a better fix...

Fixes: 49d7d581ceaf4cf8 ("drm/ssd130x: Don't allocate buffers on each plane update")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index b4c376962629580b..8ef5f61854fd7340 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -481,6 +481,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *
 	u32 array_idx = 0;
 	int ret, i, j, k;
 
+if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); return 0; }
 	drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
 
 	/*
-- 
2.34.1

Re: [PATCH/RFC 3/8] drm/ssd130x: Bail out early if data_array is not yet available

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Calling ssd130x_buf_alloc() from ssd130x_encoder_helper_atomic_enable()
> is too late, causing a NULL pointer dereference:
>
>     ssd130x_update_rect.isra.0+0x13c/0x340
>     ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
>     drm_atomic_helper_commit_planes+0xfc/0x27c
>
> Work around that by checking if data_array is valid.
>
> Obviously this needs a better fix...
>

This should be fixed by [0] so we can drop this patch from the set.

[0]: https://lists.freedesktop.org/archives/dri-devel/2023-July/413630.html

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH/RFC 3/8] drm/ssd130x: Bail out early if data_array is not yet available

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Calling ssd130x_buf_alloc() from ssd130x_encoder_helper_atomic_enable()
> is too late, causing a NULL pointer dereference:
>
>     ssd130x_update_rect.isra.0+0x13c/0x340
>     ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
>     drm_atomic_helper_commit_planes+0xfc/0x27c
>
> Work around that by checking if data_array is valid.
>
> Obviously this needs a better fix...
>

This should be fixed by [0] so we can drop this patch from the set.

[0]: https://lists.freedesktop.org/archives/dri-devel/2023-July/413630.html

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

The native display format is monochrome light-on-dark (R1).
Hence add support for R1, so monochrome applications can avoid the
overhead of back-and-forth conversions between R1 and XR24.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
allocate buffers on each plane update") in drm-misc/for-linux-next,
which always allocates the buffer upfront, while it is no longer needed
when never using XR24.

Probably ssd130x->buffer should be allocated on first use.
And why not allocate the buffers using devm_kcalloc()?
---
 drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 8ef5f61854fd7340..130e33a1ba3cba00 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -466,15 +466,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
 				 SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
 }
 
-static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect)
+static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
+			       unsigned int pitch, struct drm_rect *rect)
 {
 	unsigned int x = rect->x1;
 	unsigned int y = rect->y1;
-	u8 *buf = ssd130x->buffer;
 	u8 *data_array = ssd130x->data_array;
 	unsigned int width = drm_rect_width(rect);
 	unsigned int height = drm_rect_height(rect);
-	unsigned int line_length = DIV_ROUND_UP(width, 8);
 	unsigned int page_height = ssd130x->device_info->page_height;
 	unsigned int pages = DIV_ROUND_UP(height, page_height);
 	struct drm_device *drm = &ssd130x->drm;
@@ -534,7 +533,7 @@ if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); re
 			u8 data = 0;
 
 			for (k = 0; k < m; k++) {
-				u8 byte = buf[(8 * i + k) * line_length + j / 8];
+				u8 byte = buf[(8 * i + k) * pitch + j / 8];
 				u8 bit = (byte >> (j % 8)) & 1;
 
 				data |= bit << k;
@@ -570,6 +569,8 @@ if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); re
 
 static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
 {
+	unsigned int pitch = DIV_ROUND_UP(ssd130x->width, 8);
+	u8 *buf = ssd130x->buffer;
 	struct drm_rect fullscreen = {
 		.x1 = 0,
 		.x2 = ssd130x->width,
@@ -577,7 +578,7 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
 		.y2 = ssd130x->height,
 	};
 
-	ssd130x_update_rect(ssd130x, &fullscreen);
+	ssd130x_update_rect(ssd130x, buf, pitch, &fullscreen);
 }
 
 static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap,
@@ -588,27 +589,48 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	struct iosys_map dst;
 	unsigned int dst_pitch;
 	int ret = 0;
-	u8 *buf = ssd130x->buffer;
-
-	if (!buf)
-		return 0;
+	u8 *buf;
 
 	/* Align y to display page boundaries */
 	rect->y1 = round_down(rect->y1, page_height);
 	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
 
-	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+	switch (fb->format->format) {
+	case DRM_FORMAT_R1:
+		/* Align x to byte boundaries */
+		rect->x1 = round_down(rect->x1, 8);
+		rect->x2 = round_up(rect->x2, 8);
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return ret;
+		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
 
-	iosys_map_set_vaddr(&dst, buf);
-	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+		dst_pitch = fb->pitches[0];
+		buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8;
 
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+		ssd130x_update_rect(ssd130x, buf, dst_pitch, rect);
 
-	ssd130x_update_rect(ssd130x, rect);
+		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+		break;
+
+	case DRM_FORMAT_XRGB8888:
+		dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+		buf = ssd130x->buffer;
+		if (!buf)
+			return 0;
+
+		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+
+		iosys_map_set_vaddr(&dst, buf);
+		drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+
+		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
+		ssd130x_update_rect(ssd130x, buf, dst_pitch, rect);
+		break;
+	}
 
 	return ret;
 }
@@ -797,6 +819,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
 };
 
 static const uint32_t ssd130x_formats[] = {
+	DRM_FORMAT_R1,
 	DRM_FORMAT_XRGB8888,
 };
 
-- 
2.34.1

[PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

The native display format is monochrome light-on-dark (R1).
Hence add support for R1, so monochrome applications can avoid the
overhead of back-and-forth conversions between R1 and XR24.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
allocate buffers on each plane update") in drm-misc/for-linux-next,
which always allocates the buffer upfront, while it is no longer needed
when never using XR24.

Probably ssd130x->buffer should be allocated on first use.
And why not allocate the buffers using devm_kcalloc()?
---
 drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 8ef5f61854fd7340..130e33a1ba3cba00 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -466,15 +466,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
 				 SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
 }
 
-static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect)
+static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
+			       unsigned int pitch, struct drm_rect *rect)
 {
 	unsigned int x = rect->x1;
 	unsigned int y = rect->y1;
-	u8 *buf = ssd130x->buffer;
 	u8 *data_array = ssd130x->data_array;
 	unsigned int width = drm_rect_width(rect);
 	unsigned int height = drm_rect_height(rect);
-	unsigned int line_length = DIV_ROUND_UP(width, 8);
 	unsigned int page_height = ssd130x->device_info->page_height;
 	unsigned int pages = DIV_ROUND_UP(height, page_height);
 	struct drm_device *drm = &ssd130x->drm;
@@ -534,7 +533,7 @@ if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); re
 			u8 data = 0;
 
 			for (k = 0; k < m; k++) {
-				u8 byte = buf[(8 * i + k) * line_length + j / 8];
+				u8 byte = buf[(8 * i + k) * pitch + j / 8];
 				u8 bit = (byte >> (j % 8)) & 1;
 
 				data |= bit << k;
@@ -570,6 +569,8 @@ if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); re
 
 static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
 {
+	unsigned int pitch = DIV_ROUND_UP(ssd130x->width, 8);
+	u8 *buf = ssd130x->buffer;
 	struct drm_rect fullscreen = {
 		.x1 = 0,
 		.x2 = ssd130x->width,
@@ -577,7 +578,7 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
 		.y2 = ssd130x->height,
 	};
 
-	ssd130x_update_rect(ssd130x, &fullscreen);
+	ssd130x_update_rect(ssd130x, buf, pitch, &fullscreen);
 }
 
 static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap,
@@ -588,27 +589,48 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	struct iosys_map dst;
 	unsigned int dst_pitch;
 	int ret = 0;
-	u8 *buf = ssd130x->buffer;
-
-	if (!buf)
-		return 0;
+	u8 *buf;
 
 	/* Align y to display page boundaries */
 	rect->y1 = round_down(rect->y1, page_height);
 	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
 
-	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+	switch (fb->format->format) {
+	case DRM_FORMAT_R1:
+		/* Align x to byte boundaries */
+		rect->x1 = round_down(rect->x1, 8);
+		rect->x2 = round_up(rect->x2, 8);
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return ret;
+		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
 
-	iosys_map_set_vaddr(&dst, buf);
-	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+		dst_pitch = fb->pitches[0];
+		buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8;
 
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+		ssd130x_update_rect(ssd130x, buf, dst_pitch, rect);
 
-	ssd130x_update_rect(ssd130x, rect);
+		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+		break;
+
+	case DRM_FORMAT_XRGB8888:
+		dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+		buf = ssd130x->buffer;
+		if (!buf)
+			return 0;
+
+		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+
+		iosys_map_set_vaddr(&dst, buf);
+		drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+
+		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
+		ssd130x_update_rect(ssd130x, buf, dst_pitch, rect);
+		break;
+	}
 
 	return ret;
 }
@@ -797,6 +819,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
 };
 
 static const uint32_t ssd130x_formats[] = {
+	DRM_FORMAT_R1,
 	DRM_FORMAT_XRGB8888,
 };
 
-- 
2.34.1

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

Thanks a lot for your patch, this has been on my TODO for some time!

> The native display format is monochrome light-on-dark (R1).
> Hence add support for R1, so monochrome applications can avoid the
> overhead of back-and-forth conversions between R1 and XR24.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
> allocate buffers on each plane update") in drm-misc/for-linux-next,
> which always allocates the buffer upfront, while it is no longer needed
> when never using XR24.
>

you mean R1 here, right ? It's still used in ssd130x_clear_screen() though.

> Probably ssd130x->buffer should be allocated on first use.

Yes, that makes sense.

> And why not allocate the buffers using devm_kcalloc()?

I think there are some lifetimes discrepancies between struct device and
struct drm_device objects. But we could use drm_device managed resources
helpers, i.e: drmm_kzalloc().

> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 17 deletions(-)
>

[...]

> +	case DRM_FORMAT_XRGB8888:
> +		dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> +		buf = ssd130x->buffer;
> +		if (!buf)
> +			return 0;
> +

I think this check is not needed anymore now that the driver won't attempt
to update planes for disabled CRTCs ?

It's OK for me to be paranoid though, specially after the other issue that
you found. So I'll let you decide if you think is worth to keep the check.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

Thanks a lot for your patch, this has been on my TODO for some time!

> The native display format is monochrome light-on-dark (R1).
> Hence add support for R1, so monochrome applications can avoid the
> overhead of back-and-forth conversions between R1 and XR24.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
> allocate buffers on each plane update") in drm-misc/for-linux-next,
> which always allocates the buffer upfront, while it is no longer needed
> when never using XR24.
>

you mean R1 here, right ? It's still used in ssd130x_clear_screen() though.

> Probably ssd130x->buffer should be allocated on first use.

Yes, that makes sense.

> And why not allocate the buffers using devm_kcalloc()?

I think there are some lifetimes discrepancies between struct device and
struct drm_device objects. But we could use drm_device managed resources
helpers, i.e: drmm_kzalloc().

> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 17 deletions(-)
>

[...]

> +	case DRM_FORMAT_XRGB8888:
> +		dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> +		buf = ssd130x->buffer;
> +		if (!buf)
> +			return 0;
> +

I think this check is not needed anymore now that the driver won't attempt
to update planes for disabled CRTCs ?

It's OK for me to be paranoid though, specially after the other issue that
you found. So I'll let you decide if you think is worth to keep the check.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> Thanks a lot for your patch, this has been on my TODO for some time!
>
> > The native display format is monochrome light-on-dark (R1).
> > Hence add support for R1, so monochrome applications can avoid the
> > overhead of back-and-forth conversions between R1 and XR24.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
> > allocate buffers on each plane update") in drm-misc/for-linux-next,
> > which always allocates the buffer upfront, while it is no longer needed
> > when never using XR24.
>
> you mean R1 here, right ?

I did mean R1. I think you missed the double negation.

> It's still used in ssd130x_clear_screen() though.

I guess it became worthwhile to make ssd130x_clear_screen()
do memset(data_array, 0, ...) and call ssd130x_write_data() directly,
avoiding the pointless reshuffling of black pixels in
ssd130x_update_rect()?

> > Probably ssd130x->buffer should be allocated on first use.
>
> Yes, that makes sense.
>
> > And why not allocate the buffers using devm_kcalloc()?
>
> I think there are some lifetimes discrepancies between struct device and
> struct drm_device objects. But we could use drm_device managed resources
> helpers, i.e: drmm_kzalloc().

The display should not be updated after .remove(), so I think plain
devm_kcalloc() should be fine.

> >  drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
> >  1 file changed, 40 insertions(+), 17 deletions(-)
> >
>
> [...]
>
> > +     case DRM_FORMAT_XRGB8888:
> > +             dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> > +             buf = ssd130x->buffer;
> > +             if (!buf)
> > +                     return 0;
> > +
>
> I think this check is not needed anymore now that the driver won't attempt
> to update planes for disabled CRTCs ?

Probably. We do need it here when allocating on first use.

> It's OK for me to be paranoid though, specially after the other issue that
> you found. So I'll let you decide if you think is worth to keep the check.

I kept the check as I only moved/shifted that part of the code.

> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> Thanks a lot for your patch, this has been on my TODO for some time!
>
> > The native display format is monochrome light-on-dark (R1).
> > Hence add support for R1, so monochrome applications can avoid the
> > overhead of back-and-forth conversions between R1 and XR24.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
> > allocate buffers on each plane update") in drm-misc/for-linux-next,
> > which always allocates the buffer upfront, while it is no longer needed
> > when never using XR24.
>
> you mean R1 here, right ?

I did mean R1. I think you missed the double negation.

> It's still used in ssd130x_clear_screen() though.

I guess it became worthwhile to make ssd130x_clear_screen()
do memset(data_array, 0, ...) and call ssd130x_write_data() directly,
avoiding the pointless reshuffling of black pixels in
ssd130x_update_rect()?

> > Probably ssd130x->buffer should be allocated on first use.
>
> Yes, that makes sense.
>
> > And why not allocate the buffers using devm_kcalloc()?
>
> I think there are some lifetimes discrepancies between struct device and
> struct drm_device objects. But we could use drm_device managed resources
> helpers, i.e: drmm_kzalloc().

The display should not be updated after .remove(), so I think plain
devm_kcalloc() should be fine.

> >  drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
> >  1 file changed, 40 insertions(+), 17 deletions(-)
> >
>
> [...]
>
> > +     case DRM_FORMAT_XRGB8888:
> > +             dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> > +             buf = ssd130x->buffer;
> > +             if (!buf)
> > +                     return 0;
> > +
>
> I think this check is not needed anymore now that the driver won't attempt
> to update planes for disabled CRTCs ?

Probably. We do need it here when allocating on first use.

> It's OK for me to be paranoid though, specially after the other issue that
> you found. So I'll let you decide if you think is worth to keep the check.

I kept the check as I only moved/shifted that part of the code.

> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,
>
> On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> Thanks a lot for your patch, this has been on my TODO for some time!
>>
>> > The native display format is monochrome light-on-dark (R1).
>> > Hence add support for R1, so monochrome applications can avoid the
>> > overhead of back-and-forth conversions between R1 and XR24.
>> >
>> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> > ---
>> > This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
>> > allocate buffers on each plane update") in drm-misc/for-linux-next,
>> > which always allocates the buffer upfront, while it is no longer needed
>> > when never using XR24.
>>
>> you mean R1 here, right ?
>
> I did mean R1. I think you missed the double negation.
>

I did indeed. As a non-native english speaker, I find it very hard to
parse double negations :)

>> It's still used in ssd130x_clear_screen() though.
>
> I guess it became worthwhile to make ssd130x_clear_screen()
> do memset(data_array, 0, ...) and call ssd130x_write_data() directly,
> avoiding the pointless reshuffling of black pixels in
> ssd130x_update_rect()?
>

I think so, yeah.

>> > Probably ssd130x->buffer should be allocated on first use.
>>
>> Yes, that makes sense.
>>
>> > And why not allocate the buffers using devm_kcalloc()?
>>
>> I think there are some lifetimes discrepancies between struct device and
>> struct drm_device objects. But we could use drm_device managed resources
>> helpers, i.e: drmm_kzalloc().
>
> The display should not be updated after .remove(), so I think plain
> devm_kcalloc() should be fine.
>

That was precisely my point, that there could be atomic commits even after
the driver has been removed (e.g: if using DRM fbdev emulation, user-space
can keep the /dev/fb0 opened and continue updating the framebuffer. That's
not released until the fd is closed and struct fb_ops .fb_destroy called.

But that's a general rule in DRM, any user-visible resource must not be
allocated using device managed resources and instead use the drm_device
managed resources helpers. To make sure that are not released until the
last call to drm_dev_put():

https://docs.kernel.org/gpu/drm-internals.html#device-instance-and-driver-handling

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,
>
> On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> Thanks a lot for your patch, this has been on my TODO for some time!
>>
>> > The native display format is monochrome light-on-dark (R1).
>> > Hence add support for R1, so monochrome applications can avoid the
>> > overhead of back-and-forth conversions between R1 and XR24.
>> >
>> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> > ---
>> > This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
>> > allocate buffers on each plane update") in drm-misc/for-linux-next,
>> > which always allocates the buffer upfront, while it is no longer needed
>> > when never using XR24.
>>
>> you mean R1 here, right ?
>
> I did mean R1. I think you missed the double negation.
>

I did indeed. As a non-native english speaker, I find it very hard to
parse double negations :)

>> It's still used in ssd130x_clear_screen() though.
>
> I guess it became worthwhile to make ssd130x_clear_screen()
> do memset(data_array, 0, ...) and call ssd130x_write_data() directly,
> avoiding the pointless reshuffling of black pixels in
> ssd130x_update_rect()?
>

I think so, yeah.

>> > Probably ssd130x->buffer should be allocated on first use.
>>
>> Yes, that makes sense.
>>
>> > And why not allocate the buffers using devm_kcalloc()?
>>
>> I think there are some lifetimes discrepancies between struct device and
>> struct drm_device objects. But we could use drm_device managed resources
>> helpers, i.e: drmm_kzalloc().
>
> The display should not be updated after .remove(), so I think plain
> devm_kcalloc() should be fine.
>

That was precisely my point, that there could be atomic commits even after
the driver has been removed (e.g: if using DRM fbdev emulation, user-space
can keep the /dev/fb0 opened and continue updating the framebuffer. That's
not released until the fd is closed and struct fb_ops .fb_destroy called.

But that's a general rule in DRM, any user-visible resource must not be
allocated using device managed resources and instead use the drm_device
managed resources helpers. To make sure that are not released until the
last call to drm_dev_put():

https://docs.kernel.org/gpu/drm-internals.html#device-instance-and-driver-handling

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 2:35 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> Thanks a lot for your patch, this has been on my TODO for some time!
> >>
> >> > The native display format is monochrome light-on-dark (R1).
> >> > Hence add support for R1, so monochrome applications can avoid the
> >> > overhead of back-and-forth conversions between R1 and XR24.
> >> >
> >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> >> > Probably ssd130x->buffer should be allocated on first use.
> >>
> >> Yes, that makes sense.
> >>
> >> > And why not allocate the buffers using devm_kcalloc()?
> >>
> >> I think there are some lifetimes discrepancies between struct device and
> >> struct drm_device objects. But we could use drm_device managed resources
> >> helpers, i.e: drmm_kzalloc().
> >
> > The display should not be updated after .remove(), so I think plain
> > devm_kcalloc() should be fine.
>
> That was precisely my point, that there could be atomic commits even after
> the driver has been removed (e.g: if using DRM fbdev emulation, user-space
> can keep the /dev/fb0 opened and continue updating the framebuffer. That's
> not released until the fd is closed and struct fb_ops .fb_destroy called.
>
> But that's a general rule in DRM, any user-visible resource must not be
> allocated using device managed resources and instead use the drm_device
> managed resources helpers. To make sure that are not released until the
> last call to drm_dev_put():

These buffers are not user-visible, so they should not be accessed
after .remove().  When these are accessed, the next step would be
to write the buffer data to the device, which would also fail miserably,
as the regmap, GPIO, and regulator are hardware resources managed
through devm_*().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 2:35 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> Thanks a lot for your patch, this has been on my TODO for some time!
> >>
> >> > The native display format is monochrome light-on-dark (R1).
> >> > Hence add support for R1, so monochrome applications can avoid the
> >> > overhead of back-and-forth conversions between R1 and XR24.
> >> >
> >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> >> > Probably ssd130x->buffer should be allocated on first use.
> >>
> >> Yes, that makes sense.
> >>
> >> > And why not allocate the buffers using devm_kcalloc()?
> >>
> >> I think there are some lifetimes discrepancies between struct device and
> >> struct drm_device objects. But we could use drm_device managed resources
> >> helpers, i.e: drmm_kzalloc().
> >
> > The display should not be updated after .remove(), so I think plain
> > devm_kcalloc() should be fine.
>
> That was precisely my point, that there could be atomic commits even after
> the driver has been removed (e.g: if using DRM fbdev emulation, user-space
> can keep the /dev/fb0 opened and continue updating the framebuffer. That's
> not released until the fd is closed and struct fb_ops .fb_destroy called.
>
> But that's a general rule in DRM, any user-visible resource must not be
> allocated using device managed resources and instead use the drm_device
> managed resources helpers. To make sure that are not released until the
> last call to drm_dev_put():

These buffers are not user-visible, so they should not be accessed
after .remove().  When these are accessed, the next step would be
to write the buffer data to the device, which would also fail miserably,
as the regmap, GPIO, and regulator are hardware resources managed
through devm_*().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,

[...]

>> >
>> > The display should not be updated after .remove(), so I think plain
>> > devm_kcalloc() should be fine.
>>
>> That was precisely my point, that there could be atomic commits even after
>> the driver has been removed (e.g: if using DRM fbdev emulation, user-space
>> can keep the /dev/fb0 opened and continue updating the framebuffer. That's
>> not released until the fd is closed and struct fb_ops .fb_destroy called.
>>
>> But that's a general rule in DRM, any user-visible resource must not be
>> allocated using device managed resources and instead use the drm_device
>> managed resources helpers. To make sure that are not released until the
>> last call to drm_dev_put():
>
> These buffers are not user-visible, so they should not be accessed
> after .remove().  When these are accessed, the next step would be
> to write the buffer data to the device, which would also fail miserably,
> as the regmap, GPIO, and regulator are hardware resources managed
> through devm_*().
>

Right, given that we do the shadow buffer -> native buffer copy, I thought
of it as if as a user-visible (well, user-accessible I guess) but you are
correct that's not and trying to write to the device will fail anyways.

So devm_* should be enough indeed and if there are problems with that, it
would be a different bug in the driver to fix.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,

[...]

>> >
>> > The display should not be updated after .remove(), so I think plain
>> > devm_kcalloc() should be fine.
>>
>> That was precisely my point, that there could be atomic commits even after
>> the driver has been removed (e.g: if using DRM fbdev emulation, user-space
>> can keep the /dev/fb0 opened and continue updating the framebuffer. That's
>> not released until the fd is closed and struct fb_ops .fb_destroy called.
>>
>> But that's a general rule in DRM, any user-visible resource must not be
>> allocated using device managed resources and instead use the drm_device
>> managed resources helpers. To make sure that are not released until the
>> last call to drm_dev_put():
>
> These buffers are not user-visible, so they should not be accessed
> after .remove().  When these are accessed, the next step would be
> to write the buffer data to the device, which would also fail miserably,
> as the regmap, GPIO, and regulator are hardware resources managed
> through devm_*().
>

Right, given that we do the shadow buffer -> native buffer copy, I thought
of it as if as a user-visible (well, user-accessible I guess) but you are
correct that's not and trying to write to the device will fail anyways.

So devm_* should be enough indeed and if there are problems with that, it
would be a different bug in the driver to fix.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Geert Uytterhoeven]

Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
which uses bpp and depth to guess the wanted buffer format.
However, drm_client_buffer_addfb() already knows the exact buffer
format, so there is no need to convert back and forth between buffer
format and bpp/depth, and the function can just call drm_mode_addfb2()
directly instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_client.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index f6292ba0e6fc3783..de660a25ad7c6f06 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -374,19 +374,16 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
 				   u32 handle)
 {
 	struct drm_client_dev *client = buffer->client;
-	struct drm_mode_fb_cmd fb_req = { };
-	const struct drm_format_info *info;
+	struct drm_mode_fb_cmd2 fb_req = { };
 	int ret;
 
-	info = drm_format_info(format);
-	fb_req.bpp = drm_format_info_bpp(info, 0);
-	fb_req.depth = info->depth;
 	fb_req.width = width;
 	fb_req.height = height;
-	fb_req.handle = handle;
-	fb_req.pitch = buffer->pitch;
+	fb_req.pixel_format = format;
+	fb_req.handles[0] = handle;
+	fb_req.pitches[0] = buffer->pitch;
 
-	ret = drm_mode_addfb(client->dev, &fb_req, client->file);
+	ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Geert Uytterhoeven]

Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
which uses bpp and depth to guess the wanted buffer format.
However, drm_client_buffer_addfb() already knows the exact buffer
format, so there is no need to convert back and forth between buffer
format and bpp/depth, and the function can just call drm_mode_addfb2()
directly instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_client.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index f6292ba0e6fc3783..de660a25ad7c6f06 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -374,19 +374,16 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
 				   u32 handle)
 {
 	struct drm_client_dev *client = buffer->client;
-	struct drm_mode_fb_cmd fb_req = { };
-	const struct drm_format_info *info;
+	struct drm_mode_fb_cmd2 fb_req = { };
 	int ret;
 
-	info = drm_format_info(format);
-	fb_req.bpp = drm_format_info_bpp(info, 0);
-	fb_req.depth = info->depth;
 	fb_req.width = width;
 	fb_req.height = height;
-	fb_req.handle = handle;
-	fb_req.pitch = buffer->pitch;
+	fb_req.pixel_format = format;
+	fb_req.handles[0] = handle;
+	fb_req.pitches[0] = buffer->pitch;
 
-	ret = drm_mode_addfb(client->dev, &fb_req, client->file);
+	ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
> which uses bpp and depth to guess the wanted buffer format.
> However, drm_client_buffer_addfb() already knows the exact buffer
> format, so there is no need to convert back and forth between buffer
> format and bpp/depth, and the function can just call drm_mode_addfb2()
> directly instead.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/gpu/drm/drm_client.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>

Nice cleanup!

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
> which uses bpp and depth to guess the wanted buffer format.
> However, drm_client_buffer_addfb() already knows the exact buffer
> format, so there is no need to convert back and forth between buffer
> format and bpp/depth, and the function can just call drm_mode_addfb2()
> directly instead.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/gpu/drm/drm_client.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>

Nice cleanup!

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Simon Ser]

On Thursday, July 13th, 2023 at 15:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
> which uses bpp and depth to guess the wanted buffer format.
> However, drm_client_buffer_addfb() already knows the exact buffer
> format, so there is no need to convert back and forth between buffer
> format and bpp/depth, and the function can just call drm_mode_addfb2()
> directly instead.

By any chance, is the commit message wrong? The title refers to
drm_mode_create_dumb(), but the description and code refer to
drm_client_buffer_addfb().

Re: [PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Simon Ser]

On Thursday, July 13th, 2023 at 15:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
> which uses bpp and depth to guess the wanted buffer format.
> However, drm_client_buffer_addfb() already knows the exact buffer
> format, so there is no need to convert back and forth between buffer
> format and bpp/depth, and the function can just call drm_mode_addfb2()
> directly instead.

By any chance, is the commit message wrong? The title refers to
drm_mode_create_dumb(), but the description and code refer to
drm_client_buffer_addfb().

Re: [PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Geert Uytterhoeven]

Hi Simon,

On Fri, Jul 14, 2023 at 1:01 PM Simon Ser <contact@emersion.fr> wrote:
> On Thursday, July 13th, 2023 at 15:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
> > which uses bpp and depth to guess the wanted buffer format.
> > However, drm_client_buffer_addfb() already knows the exact buffer
> > format, so there is no need to convert back and forth between buffer
> > format and bpp/depth, and the function can just call drm_mode_addfb2()
> > directly instead.
>
> By any chance, is the commit message wrong? The title refers to
> drm_mode_create_dumb(), but the description and code refer to
> drm_client_buffer_addfb().

Yes it is, thanks.  Originally, I had copied-and-pasted the wrong
function name. I thought I had fixed all references, but apparently
I missed the one-line summary :-(

Will fix in v2.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/8] drm/client: Convert drm_mode_create_dumb() to drm_mode_addfb2()

[Geert Uytterhoeven]

Hi Simon,

On Fri, Jul 14, 2023 at 1:01 PM Simon Ser <contact@emersion.fr> wrote:
> On Thursday, July 13th, 2023 at 15:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
> > which uses bpp and depth to guess the wanted buffer format.
> > However, drm_client_buffer_addfb() already knows the exact buffer
> > format, so there is no need to convert back and forth between buffer
> > format and bpp/depth, and the function can just call drm_mode_addfb2()
> > directly instead.
>
> By any chance, is the commit message wrong? The title refers to
> drm_mode_create_dumb(), but the description and code refer to
> drm_client_buffer_addfb().

Yes it is, thanks.  Originally, I had copied-and-pasted the wrong
function name. I thought I had fixed all references, but apparently
I missed the one-line summary :-(

Will fix in v2.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Geert Uytterhoeven]

drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
followed by drm_fbdev_generic_helper_fb_probe():
  - The former tries to find a suitable buffer format, taking into
    account limitations of the whole display pipeline,
  - The latter just calls drm_mode_legacy_fb_format() again.

Simplify this by passing the buffer format between these functions
via a new buffer format member in the drm_fb_helper_surface_size
structure.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_fb_helper.c     | 1 +
 drivers/gpu/drm/drm_fbdev_generic.c | 9 ++++-----
 include/drm/drm_fb_helper.h         | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 61a5d450cc20ef0a..e870b2ce7a8625e3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1564,6 +1564,7 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper,
 	info = drm_format_info(surface_format);
 	sizes->surface_bpp = drm_format_info_bpp(info, 0);
 	sizes->surface_depth = info->depth;
+	sizes->surface_format = surface_format;
 
 	/* first up get a count of crtcs now in use and new min/maxes width/heights */
 	crtc_count = 0;
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index 98ae703848a02fa3..953056896c1a5652 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -77,16 +77,15 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
 	struct fb_info *info;
 	size_t screen_size;
 	void *screen_buffer;
-	u32 format;
 	int ret;
 
-	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
+	drm_info(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",
 		    sizes->surface_width, sizes->surface_height,
-		    sizes->surface_bpp);
+		    sizes->surface_bpp, &sizes->surface_format);
 
-	format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
 	buffer = drm_client_framebuffer_create(client, sizes->surface_width,
-					       sizes->surface_height, format);
+					       sizes->surface_height,
+					       sizes->surface_format);
 	if (IS_ERR(buffer))
 		return PTR_ERR(buffer);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 4863b0f8299e89b6..430a17b530fa49e6 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -45,6 +45,7 @@ struct drm_fb_helper;
  * @surface_height: scanout buffer height
  * @surface_bpp: scanout buffer bpp
  * @surface_depth: scanout buffer depth
+ * @surface_format: scanout buffer format (optional)
  *
  * Note that the scanout surface width/height may be larger than the fbdev
  * width/height.  In case of multiple displays, the scanout surface is sized
@@ -61,6 +62,7 @@ struct drm_fb_helper_surface_size {
 	u32 surface_height;
 	u32 surface_bpp;
 	u32 surface_depth;
+	u32 surface_format;
 };
 
 /**
-- 
2.34.1

[PATCH 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Geert Uytterhoeven]

drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
followed by drm_fbdev_generic_helper_fb_probe():
  - The former tries to find a suitable buffer format, taking into
    account limitations of the whole display pipeline,
  - The latter just calls drm_mode_legacy_fb_format() again.

Simplify this by passing the buffer format between these functions
via a new buffer format member in the drm_fb_helper_surface_size
structure.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_fb_helper.c     | 1 +
 drivers/gpu/drm/drm_fbdev_generic.c | 9 ++++-----
 include/drm/drm_fb_helper.h         | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 61a5d450cc20ef0a..e870b2ce7a8625e3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1564,6 +1564,7 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper,
 	info = drm_format_info(surface_format);
 	sizes->surface_bpp = drm_format_info_bpp(info, 0);
 	sizes->surface_depth = info->depth;
+	sizes->surface_format = surface_format;
 
 	/* first up get a count of crtcs now in use and new min/maxes width/heights */
 	crtc_count = 0;
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index 98ae703848a02fa3..953056896c1a5652 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -77,16 +77,15 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
 	struct fb_info *info;
 	size_t screen_size;
 	void *screen_buffer;
-	u32 format;
 	int ret;
 
-	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
+	drm_info(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",
 		    sizes->surface_width, sizes->surface_height,
-		    sizes->surface_bpp);
+		    sizes->surface_bpp, &sizes->surface_format);
 
-	format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
 	buffer = drm_client_framebuffer_create(client, sizes->surface_width,
-					       sizes->surface_height, format);
+					       sizes->surface_height,
+					       sizes->surface_format);
 	if (IS_ERR(buffer))
 		return PTR_ERR(buffer);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 4863b0f8299e89b6..430a17b530fa49e6 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -45,6 +45,7 @@ struct drm_fb_helper;
  * @surface_height: scanout buffer height
  * @surface_bpp: scanout buffer bpp
  * @surface_depth: scanout buffer depth
+ * @surface_format: scanout buffer format (optional)
  *
  * Note that the scanout surface width/height may be larger than the fbdev
  * width/height.  In case of multiple displays, the scanout surface is sized
@@ -61,6 +62,7 @@ struct drm_fb_helper_surface_size {
 	u32 surface_height;
 	u32 surface_bpp;
 	u32 surface_depth;
+	u32 surface_format;
 };
 
 /**
-- 
2.34.1

Re: [PATCH 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
> followed by drm_fbdev_generic_helper_fb_probe():
>   - The former tries to find a suitable buffer format, taking into
>     account limitations of the whole display pipeline,
>   - The latter just calls drm_mode_legacy_fb_format() again.
>
> Simplify this by passing the buffer format between these functions
> via a new buffer format member in the drm_fb_helper_surface_size
> structure.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

[...]

> -	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
> +	drm_info(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",

You are promoting a debug printout here to info but that change is not
mentioned in the commit message. If you think this will be useful, maybe
do it as a separate patch ?

The rest of the patch looks good to me though.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
> followed by drm_fbdev_generic_helper_fb_probe():
>   - The former tries to find a suitable buffer format, taking into
>     account limitations of the whole display pipeline,
>   - The latter just calls drm_mode_legacy_fb_format() again.
>
> Simplify this by passing the buffer format between these functions
> via a new buffer format member in the drm_fb_helper_surface_size
> structure.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

[...]

> -	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
> +	drm_info(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",

You are promoting a debug printout here to info but that change is not
mentioned in the commit message. If you think this will be useful, maybe
do it as a separate patch ?

The rest of the patch looks good to me though.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 12:25 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
> > followed by drm_fbdev_generic_helper_fb_probe():
> >   - The former tries to find a suitable buffer format, taking into
> >     account limitations of the whole display pipeline,
> >   - The latter just calls drm_mode_legacy_fb_format() again.
> >
> > Simplify this by passing the buffer format between these functions
> > via a new buffer format member in the drm_fb_helper_surface_size
> > structure.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
>
> [...]
>
> > -     drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
> > +     drm_info(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",
>
> You are promoting a debug printout here to info but that change is not
> mentioned in the commit message. If you think this will be useful, maybe
> do it as a separate patch ?

Oops, that was unintentional.  Will fix in v2.

> The rest of the patch looks good to me though.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 12:25 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
> > followed by drm_fbdev_generic_helper_fb_probe():
> >   - The former tries to find a suitable buffer format, taking into
> >     account limitations of the whole display pipeline,
> >   - The latter just calls drm_mode_legacy_fb_format() again.
> >
> > Simplify this by passing the buffer format between these functions
> > via a new buffer format member in the drm_fb_helper_surface_size
> > structure.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
>
> [...]
>
> > -     drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
> > +     drm_info(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",
>
> You are promoting a debug printout here to info but that change is not
> mentioned in the commit message. If you think this will be useful, maybe
> do it as a separate patch ?

Oops, that was unintentional.  Will fix in v2.

> The rest of the patch looks good to me though.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 7/8] drm/fb-helper: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Add support for the monochrome light-on-dark buffer format (R1) to the
fb helper, so this format can be used for fbdev emulation and for the
text console.  This avoids the overhead of using XR24 and the associated
conversions on display hardware that supports only a simple monochrome
format.

R1 is very similar to C1 (monochrome indexed color), and shares the same
depth and bpp.  As drm_mode_legacy_fb_format() returns a format based on
only depth and bpp, it cannot distinguish between R1 and C1.  Hence
drm_fb_helper_find_format() is modified to try to fall back to R1 if C1
is not supported.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 41 ++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e870b2ce7a8625e3..1f1bfa764b6b9f00 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1130,7 +1130,7 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
 {
 	u8 depth = format->depth;
 
-	if (format->is_color_indexed) {
+	if (format->format == DRM_FORMAT_R1 || format->is_color_indexed) {
 		var->red.offset = 0;
 		var->green.offset = 0;
 		var->blue.offset = 0;
@@ -1236,6 +1236,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	case DRM_FORMAT_C1:
 	case DRM_FORMAT_C2:
 	case DRM_FORMAT_C4:
+	case DRM_FORMAT_R1:
 		/* supported format with sub-byte pixels */
 		break;
 
@@ -1439,12 +1440,24 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
+static bool is_supported_format(uint32_t format, const uint32_t *formats,
+				size_t format_count)
+{
+	size_t i;
+
+	for (i = 0; i < format_count; ++i) {
+		if (formats[i] == format)
+			return true;
+	}
+
+	return false;
+}
+
 static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const uint32_t *formats,
 					  size_t format_count, uint32_t bpp, uint32_t depth)
 {
 	struct drm_device *dev = fb_helper->dev;
 	uint32_t format;
-	size_t i;
 
 	/*
 	 * Do not consider YUV or other complicated formats
@@ -1457,10 +1470,12 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const
 	if (!format)
 		goto err;
 
-	for (i = 0; i < format_count; ++i) {
-		if (formats[i] == format)
-			return format;
-	}
+	if (is_supported_format(format, formats, format_count))
+		return format;
+
+	if (format == DRM_FORMAT_C1 &&
+	    is_supported_format(DRM_FORMAT_R1, formats, format_count))
+		return DRM_FORMAT_R1;
 
 err:
 	/* We found nothing. */
@@ -1680,11 +1695,15 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
 }
 
 static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
-				   bool is_color_indexed)
+				   const struct drm_format_info *format)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
-					    : FB_VISUAL_TRUECOLOR;
+	if (format->format == DRM_FORMAT_R1)
+		info->fix.visual = FB_VISUAL_MONO10;
+	else if (format->is_color_indexed)
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+	else
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1707,6 +1726,7 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
 	case DRM_FORMAT_C1:
 	case DRM_FORMAT_C2:
 	case DRM_FORMAT_C4:
+	case DRM_FORMAT_R1:
 		/* supported format with sub-byte pixels */
 		break;
 
@@ -1747,8 +1767,7 @@ void drm_fb_helper_fill_info(struct fb_info *info,
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 
-	drm_fb_helper_fill_fix(info, fb->pitches[0],
-			       fb->format->is_color_indexed);
+	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);
 
-- 
2.34.1

[PATCH 7/8] drm/fb-helper: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Add support for the monochrome light-on-dark buffer format (R1) to the
fb helper, so this format can be used for fbdev emulation and for the
text console.  This avoids the overhead of using XR24 and the associated
conversions on display hardware that supports only a simple monochrome
format.

R1 is very similar to C1 (monochrome indexed color), and shares the same
depth and bpp.  As drm_mode_legacy_fb_format() returns a format based on
only depth and bpp, it cannot distinguish between R1 and C1.  Hence
drm_fb_helper_find_format() is modified to try to fall back to R1 if C1
is not supported.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 41 ++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e870b2ce7a8625e3..1f1bfa764b6b9f00 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1130,7 +1130,7 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
 {
 	u8 depth = format->depth;
 
-	if (format->is_color_indexed) {
+	if (format->format == DRM_FORMAT_R1 || format->is_color_indexed) {
 		var->red.offset = 0;
 		var->green.offset = 0;
 		var->blue.offset = 0;
@@ -1236,6 +1236,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	case DRM_FORMAT_C1:
 	case DRM_FORMAT_C2:
 	case DRM_FORMAT_C4:
+	case DRM_FORMAT_R1:
 		/* supported format with sub-byte pixels */
 		break;
 
@@ -1439,12 +1440,24 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
+static bool is_supported_format(uint32_t format, const uint32_t *formats,
+				size_t format_count)
+{
+	size_t i;
+
+	for (i = 0; i < format_count; ++i) {
+		if (formats[i] == format)
+			return true;
+	}
+
+	return false;
+}
+
 static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const uint32_t *formats,
 					  size_t format_count, uint32_t bpp, uint32_t depth)
 {
 	struct drm_device *dev = fb_helper->dev;
 	uint32_t format;
-	size_t i;
 
 	/*
 	 * Do not consider YUV or other complicated formats
@@ -1457,10 +1470,12 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const
 	if (!format)
 		goto err;
 
-	for (i = 0; i < format_count; ++i) {
-		if (formats[i] == format)
-			return format;
-	}
+	if (is_supported_format(format, formats, format_count))
+		return format;
+
+	if (format == DRM_FORMAT_C1 &&
+	    is_supported_format(DRM_FORMAT_R1, formats, format_count))
+		return DRM_FORMAT_R1;
 
 err:
 	/* We found nothing. */
@@ -1680,11 +1695,15 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
 }
 
 static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
-				   bool is_color_indexed)
+				   const struct drm_format_info *format)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
-					    : FB_VISUAL_TRUECOLOR;
+	if (format->format == DRM_FORMAT_R1)
+		info->fix.visual = FB_VISUAL_MONO10;
+	else if (format->is_color_indexed)
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+	else
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1707,6 +1726,7 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
 	case DRM_FORMAT_C1:
 	case DRM_FORMAT_C2:
 	case DRM_FORMAT_C4:
+	case DRM_FORMAT_R1:
 		/* supported format with sub-byte pixels */
 		break;
 
@@ -1747,8 +1767,7 @@ void drm_fb_helper_fill_info(struct fb_info *info,
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 
-	drm_fb_helper_fill_fix(info, fb->pitches[0],
-			       fb->format->is_color_indexed);
+	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);
 
-- 
2.34.1

Re: [PATCH 7/8] drm/fb-helper: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Add support for the monochrome light-on-dark buffer format (R1) to the
> fb helper, so this format can be used for fbdev emulation and for the
> text console.  This avoids the overhead of using XR24 and the associated
> conversions on display hardware that supports only a simple monochrome
> format.
>
> R1 is very similar to C1 (monochrome indexed color), and shares the same
> depth and bpp.  As drm_mode_legacy_fb_format() returns a format based on
> only depth and bpp, it cannot distinguish between R1 and C1.  Hence
> drm_fb_helper_find_format() is modified to try to fall back to R1 if C1
> is not supported.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 7/8] drm/fb-helper: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Add support for the monochrome light-on-dark buffer format (R1) to the
> fb helper, so this format can be used for fbdev emulation and for the
> text console.  This avoids the overhead of using XR24 and the associated
> conversions on display hardware that supports only a simple monochrome
> format.
>
> R1 is very similar to C1 (monochrome indexed color), and shares the same
> depth and bpp.  As drm_mode_legacy_fb_format() returns a format based on
> only depth and bpp, it cannot distinguish between R1 and C1.  Hence
> drm_fb_helper_find_format() is modified to try to fall back to R1 if C1
> is not supported.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 8/8] drm/ssd130x: Switch preferred_bpp/depth to 1

[Geert Uytterhoeven]

The native display format is R1.  Hence change the preferred_depth and
preferred_bpp to 1, to avoid the overhead of using XR24 and the
associated conversions when using fbdev emulation and its text console.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 130e33a1ba3cba00..93af2e5fc816b5f0 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -945,7 +945,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
 	drm->mode_config.max_width = max_width;
 	drm->mode_config.min_height = mode->vdisplay;
 	drm->mode_config.max_height = max_height;
-	drm->mode_config.preferred_depth = 24;
+	drm->mode_config.preferred_depth = 1;
 	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
 
 	/* Primary plane */
@@ -1075,7 +1075,7 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		return ERR_PTR(dev_err_probe(dev, ret, "DRM device register failed\n"));
 
-	drm_fbdev_generic_setup(drm, 32);
+	drm_fbdev_generic_setup(drm, 1);
 
 	return ssd130x;
 }
-- 
2.34.1

[PATCH 8/8] drm/ssd130x: Switch preferred_bpp/depth to 1

[Geert Uytterhoeven]

The native display format is R1.  Hence change the preferred_depth and
preferred_bpp to 1, to avoid the overhead of using XR24 and the
associated conversions when using fbdev emulation and its text console.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 130e33a1ba3cba00..93af2e5fc816b5f0 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -945,7 +945,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
 	drm->mode_config.max_width = max_width;
 	drm->mode_config.min_height = mode->vdisplay;
 	drm->mode_config.max_height = max_height;
-	drm->mode_config.preferred_depth = 24;
+	drm->mode_config.preferred_depth = 1;
 	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
 
 	/* Primary plane */
@@ -1075,7 +1075,7 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		return ERR_PTR(dev_err_probe(dev, ret, "DRM device register failed\n"));
 
-	drm_fbdev_generic_setup(drm, 32);
+	drm_fbdev_generic_setup(drm, 1);
 
 	return ssd130x;
 }
-- 
2.34.1

Re: [PATCH 8/8] drm/ssd130x: Switch preferred_bpp/depth to 1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> The native display format is R1.  Hence change the preferred_depth and
> preferred_bpp to 1, to avoid the overhead of using XR24 and the
> associated conversions when using fbdev emulation and its text console.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks again for the series!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 8/8] drm/ssd130x: Switch preferred_bpp/depth to 1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> The native display format is R1.  Hence change the preferred_depth and
> preferred_bpp to 1, to avoid the overhead of using XR24 and the
> associated conversions when using fbdev emulation and its text console.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks again for the series!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> 	Hi all,
>
> The native display format of ssd1306 OLED displays is monochrome
> light-on-dark (R1).  This patch series adds support for the R1 buffer
> format to both the ssd130x DRM driver and the FB helpers, so monochrome
> applications (including fbdev emulation and the text console) can avoid
> the overhead of back-and-forth conversions between R1 and XR24.
>

I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
me, but had some issues when trying to run your fbtest suite. Because the
test005 gets killed with a SIGSEGV.

$ ./fbtest -d 
fb_init()
fb_open()
fb_get_var()
fb_get_fix()
fb_map()
fb_start = 0, fb_offset = 0, fb_len = 1000
fb_save()
fb_clear()
Using drawops planar (monochrome and (interleaved) bitplanes)
Available visuals:
  Monochrome
  Grayscale 2
Using visops monochrome
Running all tests
Running test test001
test001: PASSED
Running test test002
test002: PASSED
Running test test003
Requirement num_colors >= 16 not met
Running test test004
test004: PASSED
Running test test005
Caught signal 11. Exiting
fb_cleanup()
fb_restore()
fb_unmap()
fb_set_var()
fb_get_var()
fb_get_fix()
fb_close()

Maybe more tests are missing the minimum num_colors requirement? Also, the
penguin in test004 is not displayed correctly. I was expecting that to be
working correctly since you mentioned to be using the Linux logo on boot.

Another question, do you know if is possible to change the default format?
I believe that fbset won't work because the DRM fbdev emulation layer does
not implement mode settings but I thought that changing the mode using the
atomic KMS API would work.

$ modetest -M ssd130x -P 31@33:128x64@XR24 -a
$ echo $?
0

but after that I still get:

$ fbset -i

mode "128x64"
    geometry 128 64 128 64 1
    timings 0 0 0 0 0 0 0
    rgba 1/0,1/0,1/0,0/0
endmode

Frame buffer device information:
    Name        : ssd130xdrmfb
    Address     : (nil)
    Size        : 4096
    Type        : PACKED PIXELS
    Visual      : MONO10
    XPanStep    : 1
    YPanStep    : 1
    YWrapStep   : 0
    LineLength  : 16
    Accelerator : No

Maybe I'm doing something wrong or misunderstading about how should work?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> 	Hi all,
>
> The native display format of ssd1306 OLED displays is monochrome
> light-on-dark (R1).  This patch series adds support for the R1 buffer
> format to both the ssd130x DRM driver and the FB helpers, so monochrome
> applications (including fbdev emulation and the text console) can avoid
> the overhead of back-and-forth conversions between R1 and XR24.
>

I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
me, but had some issues when trying to run your fbtest suite. Because the
test005 gets killed with a SIGSEGV.

$ ./fbtest -d 
fb_init()
fb_open()
fb_get_var()
fb_get_fix()
fb_map()
fb_start = 0, fb_offset = 0, fb_len = 1000
fb_save()
fb_clear()
Using drawops planar (monochrome and (interleaved) bitplanes)
Available visuals:
  Monochrome
  Grayscale 2
Using visops monochrome
Running all tests
Running test test001
test001: PASSED
Running test test002
test002: PASSED
Running test test003
Requirement num_colors >= 16 not met
Running test test004
test004: PASSED
Running test test005
Caught signal 11. Exiting
fb_cleanup()
fb_restore()
fb_unmap()
fb_set_var()
fb_get_var()
fb_get_fix()
fb_close()

Maybe more tests are missing the minimum num_colors requirement? Also, the
penguin in test004 is not displayed correctly. I was expecting that to be
working correctly since you mentioned to be using the Linux logo on boot.

Another question, do you know if is possible to change the default format?
I believe that fbset won't work because the DRM fbdev emulation layer does
not implement mode settings but I thought that changing the mode using the
atomic KMS API would work.

$ modetest -M ssd130x -P 31@33:128x64@XR24 -a
$ echo $?
0

but after that I still get:

$ fbset -i

mode "128x64"
    geometry 128 64 128 64 1
    timings 0 0 0 0 0 0 0
    rgba 1/0,1/0,1/0,0/0
endmode

Frame buffer device information:
    Name        : ssd130xdrmfb
    Address     : (nil)
    Size        : 4096
    Type        : PACKED PIXELS
    Visual      : MONO10
    XPanStep    : 1
    YPanStep    : 1
    YWrapStep   : 0
    LineLength  : 16
    Accelerator : No

Maybe I'm doing something wrong or misunderstading about how should work?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Sun, Jul 16, 2023 at 3:30 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > The native display format of ssd1306 OLED displays is monochrome
> > light-on-dark (R1).  This patch series adds support for the R1 buffer
> > format to both the ssd130x DRM driver and the FB helpers, so monochrome
> > applications (including fbdev emulation and the text console) can avoid
> > the overhead of back-and-forth conversions between R1 and XR24.
>
> I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
> me, but had some issues when trying to run your fbtest suite. Because the

Thanks, due to the limited userspace environment on my RV32 test system,
I didn't run fbtest myself.

> test005 gets killed with a SIGSEGV.
>
> $ ./fbtest -d
> fb_init()
> fb_open()
> fb_get_var()
> fb_get_fix()
> fb_map()
> fb_start = 0, fb_offset = 0, fb_len = 1000

   [...]

> Running test test005
> Caught signal 11. Exiting

Strange.

> Maybe more tests are missing the minimum num_colors requirement? Also, the

On monochrome, test005 should make the left half of the screen black,
and the right half white.  It works on ARAnyM, and there don't seem
to be off-by-one errors in the call to fill_rect().
Can you please run this under gdb or valgrind?

> penguin in test004 is not displayed correctly. I was expecting that to be
> working correctly since you mentioned to be using the Linux logo on boot.

Linux has logos for displays using 2, 16, and 256 colors. Note that the
default logos are 80x80, which is larger than your display, so no logo
is drawn.
Fbtest has only the full color logo, so it will look bad on a monochrome
display.

> Another question, do you know if is possible to change the default format?

AFAIK DRM does not support that.

> I believe that fbset won't work because the DRM fbdev emulation layer does
> not implement mode settings but I thought that changing the mode using the

Correct.

> atomic KMS API would work.
>
> $ modetest -M ssd130x -P 31@33:128x64@XR24 -a
> $ echo $?
> 0
>
> but after that I still get:
>
> $ fbset -i
>
> mode "128x64"
>     geometry 128 64 128 64 1
>     timings 0 0 0 0 0 0 0
>     rgba 1/0,1/0,1/0,0/0
> endmode
>
> Frame buffer device information:
>     Name        : ssd130xdrmfb
>     Address     : (nil)
>     Size        : 4096
>     Type        : PACKED PIXELS
>     Visual      : MONO10
>     XPanStep    : 1
>     YPanStep    : 1
>     YWrapStep   : 0
>     LineLength  : 16
>     Accelerator : No
>
> Maybe I'm doing something wrong or misunderstading about how should work?

Do you need the "-d" option (drop master after mode set) of modetest?
Still, that would only impact DRM.  The fbdev emulation on top of DRM has
already been initialized before, and is never reinitialized.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Sun, Jul 16, 2023 at 3:30 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > The native display format of ssd1306 OLED displays is monochrome
> > light-on-dark (R1).  This patch series adds support for the R1 buffer
> > format to both the ssd130x DRM driver and the FB helpers, so monochrome
> > applications (including fbdev emulation and the text console) can avoid
> > the overhead of back-and-forth conversions between R1 and XR24.
>
> I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
> me, but had some issues when trying to run your fbtest suite. Because the

Thanks, due to the limited userspace environment on my RV32 test system,
I didn't run fbtest myself.

> test005 gets killed with a SIGSEGV.
>
> $ ./fbtest -d
> fb_init()
> fb_open()
> fb_get_var()
> fb_get_fix()
> fb_map()
> fb_start = 0, fb_offset = 0, fb_len = 1000

   [...]

> Running test test005
> Caught signal 11. Exiting

Strange.

> Maybe more tests are missing the minimum num_colors requirement? Also, the

On monochrome, test005 should make the left half of the screen black,
and the right half white.  It works on ARAnyM, and there don't seem
to be off-by-one errors in the call to fill_rect().
Can you please run this under gdb or valgrind?

> penguin in test004 is not displayed correctly. I was expecting that to be
> working correctly since you mentioned to be using the Linux logo on boot.

Linux has logos for displays using 2, 16, and 256 colors. Note that the
default logos are 80x80, which is larger than your display, so no logo
is drawn.
Fbtest has only the full color logo, so it will look bad on a monochrome
display.

> Another question, do you know if is possible to change the default format?

AFAIK DRM does not support that.

> I believe that fbset won't work because the DRM fbdev emulation layer does
> not implement mode settings but I thought that changing the mode using the

Correct.

> atomic KMS API would work.
>
> $ modetest -M ssd130x -P 31@33:128x64@XR24 -a
> $ echo $?
> 0
>
> but after that I still get:
>
> $ fbset -i
>
> mode "128x64"
>     geometry 128 64 128 64 1
>     timings 0 0 0 0 0 0 0
>     rgba 1/0,1/0,1/0,0/0
> endmode
>
> Frame buffer device information:
>     Name        : ssd130xdrmfb
>     Address     : (nil)
>     Size        : 4096
>     Type        : PACKED PIXELS
>     Visual      : MONO10
>     XPanStep    : 1
>     YPanStep    : 1
>     YWrapStep   : 0
>     LineLength  : 16
>     Accelerator : No
>
> Maybe I'm doing something wrong or misunderstading about how should work?

Do you need the "-d" option (drop master after mode set) of modetest?
Still, that would only impact DRM.  The fbdev emulation on top of DRM has
already been initialized before, and is never reinitialized.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,
>
> On Sun, Jul 16, 2023 at 3:30 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> > The native display format of ssd1306 OLED displays is monochrome
>> > light-on-dark (R1).  This patch series adds support for the R1 buffer
>> > format to both the ssd130x DRM driver and the FB helpers, so monochrome
>> > applications (including fbdev emulation and the text console) can avoid
>> > the overhead of back-and-forth conversions between R1 and XR24.
>>
>> I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
>> me, but had some issues when trying to run your fbtest suite. Because the
>
> Thanks, due to the limited userspace environment on my RV32 test system,
> I didn't run fbtest myself.
>

You are welcome.

>> test005 gets killed with a SIGSEGV.
>>
>> $ ./fbtest -d
>> fb_init()
>> fb_open()
>> fb_get_var()
>> fb_get_fix()
>> fb_map()
>> fb_start = 0, fb_offset = 0, fb_len = 1000
>
>    [...]
>
>> Running test test005
>> Caught signal 11. Exiting
>
> Strange.
>
>> Maybe more tests are missing the minimum num_colors requirement? Also, the
>
> On monochrome, test005 should make the left half of the screen black,
> and the right half white.  It works on ARAnyM, and there don't seem
> to be off-by-one errors in the call to fill_rect().
> Can you please run this under gdb or valgrind?
>

Sure. I only spent my free time on these panels though so likely will do
during the week or more likely the weekend. I believe the bug is somewhere
in the test though and not in your kernel patches.

>> penguin in test004 is not displayed correctly. I was expecting that to be
>> working correctly since you mentioned to be using the Linux logo on boot.
>
> Linux has logos for displays using 2, 16, and 256 colors. Note that the
> default logos are 80x80, which is larger than your display, so no logo
> is drawn.
> Fbtest has only the full color logo, so it will look bad on a monochrome
> display.
>

I see. Should the test check for minimum num_colors and skip that test then?

>> Another question, do you know if is possible to change the default format?
>
> AFAIK DRM does not support that.
>
>> I believe that fbset won't work because the DRM fbdev emulation layer does
>> not implement mode settings but I thought that changing the mode using the
>
> Correct.
>

[...]

>> Maybe I'm doing something wrong or misunderstading about how should work?
>
> Do you need the "-d" option (drop master after mode set) of modetest?
> Still, that would only impact DRM.  The fbdev emulation on top of DRM has
> already been initialized before, and is never reinitialized.
>

Ah, that explains and makes sense now. I tested other user-space fbdev
programs that only support XRGB8888 and those were working correctly so I
guess that user-space that supports R1 be defaulted to that and not able
to change is an acceptable behaviour.

When you post v2, feel free to add:

Tested-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,
>
> On Sun, Jul 16, 2023 at 3:30 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> > The native display format of ssd1306 OLED displays is monochrome
>> > light-on-dark (R1).  This patch series adds support for the R1 buffer
>> > format to both the ssd130x DRM driver and the FB helpers, so monochrome
>> > applications (including fbdev emulation and the text console) can avoid
>> > the overhead of back-and-forth conversions between R1 and XR24.
>>
>> I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
>> me, but had some issues when trying to run your fbtest suite. Because the
>
> Thanks, due to the limited userspace environment on my RV32 test system,
> I didn't run fbtest myself.
>

You are welcome.

>> test005 gets killed with a SIGSEGV.
>>
>> $ ./fbtest -d
>> fb_init()
>> fb_open()
>> fb_get_var()
>> fb_get_fix()
>> fb_map()
>> fb_start = 0, fb_offset = 0, fb_len = 1000
>
>    [...]
>
>> Running test test005
>> Caught signal 11. Exiting
>
> Strange.
>
>> Maybe more tests are missing the minimum num_colors requirement? Also, the
>
> On monochrome, test005 should make the left half of the screen black,
> and the right half white.  It works on ARAnyM, and there don't seem
> to be off-by-one errors in the call to fill_rect().
> Can you please run this under gdb or valgrind?
>

Sure. I only spent my free time on these panels though so likely will do
during the week or more likely the weekend. I believe the bug is somewhere
in the test though and not in your kernel patches.

>> penguin in test004 is not displayed correctly. I was expecting that to be
>> working correctly since you mentioned to be using the Linux logo on boot.
>
> Linux has logos for displays using 2, 16, and 256 colors. Note that the
> default logos are 80x80, which is larger than your display, so no logo
> is drawn.
> Fbtest has only the full color logo, so it will look bad on a monochrome
> display.
>

I see. Should the test check for minimum num_colors and skip that test then?

>> Another question, do you know if is possible to change the default format?
>
> AFAIK DRM does not support that.
>
>> I believe that fbset won't work because the DRM fbdev emulation layer does
>> not implement mode settings but I thought that changing the mode using the
>
> Correct.
>

[...]

>> Maybe I'm doing something wrong or misunderstading about how should work?
>
> Do you need the "-d" option (drop master after mode set) of modetest?
> Still, that would only impact DRM.  The fbdev emulation on top of DRM has
> already been initialized before, and is never reinitialized.
>

Ah, that explains and makes sense now. I tested other user-space fbdev
programs that only support XRGB8888 and those were working correctly so I
guess that user-space that supports R1 be defaulted to that and not able
to change is an acceptable behaviour.

When you post v2, feel free to add:

Tested-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jul 17, 2023 at 11:13 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Sun, Jul 16, 2023 at 3:30 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> > The native display format of ssd1306 OLED displays is monochrome
> >> > light-on-dark (R1).  This patch series adds support for the R1 buffer
> >> > format to both the ssd130x DRM driver and the FB helpers, so monochrome
> >> > applications (including fbdev emulation and the text console) can avoid
> >> > the overhead of back-and-forth conversions between R1 and XR24.
> >>
> >> I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
> >> me, but had some issues when trying to run your fbtest suite. Because the
> >
> > Thanks, due to the limited userspace environment on my RV32 test system,
> > I didn't run fbtest myself.
> >
>
> You are welcome.
>
> >> test005 gets killed with a SIGSEGV.
> >>
> >> $ ./fbtest -d
> >> fb_init()
> >> fb_open()
> >> fb_get_var()
> >> fb_get_fix()
> >> fb_map()
> >> fb_start = 0, fb_offset = 0, fb_len = 1000
> >
> >    [...]
> >
> >> Running test test005
> >> Caught signal 11. Exiting
> >
> > Strange.
> >
> >> Maybe more tests are missing the minimum num_colors requirement? Also, the
> >
> > On monochrome, test005 should make the left half of the screen black,
> > and the right half white.  It works on ARAnyM, and there don't seem
> > to be off-by-one errors in the call to fill_rect().
> > Can you please run this under gdb or valgrind?
> >
>
> Sure. I only spent my free time on these panels though so likely will do
> during the week or more likely the weekend. I believe the bug is somewhere
> in the test though and not in your kernel patches.

OK.

> >> penguin in test004 is not displayed correctly. I was expecting that to be
> >> working correctly since you mentioned to be using the Linux logo on boot.
> >
> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
> > default logos are 80x80, which is larger than your display, so no logo
> > is drawn.
> > Fbtest has only the full color logo, so it will look bad on a monochrome
> > display.
>
> I see. Should the test check for minimum num_colors and skip that test then?

The test still works (you did see an ugly black-and-white penguin), doesn't it?

> When you post v2, feel free to add:
>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jul 17, 2023 at 11:13 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Sun, Jul 16, 2023 at 3:30 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> > The native display format of ssd1306 OLED displays is monochrome
> >> > light-on-dark (R1).  This patch series adds support for the R1 buffer
> >> > format to both the ssd130x DRM driver and the FB helpers, so monochrome
> >> > applications (including fbdev emulation and the text console) can avoid
> >> > the overhead of back-and-forth conversions between R1 and XR24.
> >>
> >> I've tested your series on a ssd1306 I2C OLED panel and fbcon did work for
> >> me, but had some issues when trying to run your fbtest suite. Because the
> >
> > Thanks, due to the limited userspace environment on my RV32 test system,
> > I didn't run fbtest myself.
> >
>
> You are welcome.
>
> >> test005 gets killed with a SIGSEGV.
> >>
> >> $ ./fbtest -d
> >> fb_init()
> >> fb_open()
> >> fb_get_var()
> >> fb_get_fix()
> >> fb_map()
> >> fb_start = 0, fb_offset = 0, fb_len = 1000
> >
> >    [...]
> >
> >> Running test test005
> >> Caught signal 11. Exiting
> >
> > Strange.
> >
> >> Maybe more tests are missing the minimum num_colors requirement? Also, the
> >
> > On monochrome, test005 should make the left half of the screen black,
> > and the right half white.  It works on ARAnyM, and there don't seem
> > to be off-by-one errors in the call to fill_rect().
> > Can you please run this under gdb or valgrind?
> >
>
> Sure. I only spent my free time on these panels though so likely will do
> during the week or more likely the weekend. I believe the bug is somewhere
> in the test though and not in your kernel patches.

OK.

> >> penguin in test004 is not displayed correctly. I was expecting that to be
> >> working correctly since you mentioned to be using the Linux logo on boot.
> >
> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
> > default logos are 80x80, which is larger than your display, so no logo
> > is drawn.
> > Fbtest has only the full color logo, so it will look bad on a monochrome
> > display.
>
> I see. Should the test check for minimum num_colors and skip that test then?

The test still works (you did see an ugly black-and-white penguin), doesn't it?

> When you post v2, feel free to add:
>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,

[...]

>> >> penguin in test004 is not displayed correctly. I was expecting that to be
>> >> working correctly since you mentioned to be using the Linux logo on boot.
>> >
>> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
>> > default logos are 80x80, which is larger than your display, so no logo
>> > is drawn.
>> > Fbtest has only the full color logo, so it will look bad on a monochrome
>> > display.
>>
>> I see. Should the test check for minimum num_colors and skip that test then?
>
> The test still works (you did see an ugly black-and-white penguin), doesn't it?
>

Fair enough. But when it defaulted to XRGB8888, it looked better. So I
thought that it was a regression. No strong opinion though if the test
should be skipped or not.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,

[...]

>> >> penguin in test004 is not displayed correctly. I was expecting that to be
>> >> working correctly since you mentioned to be using the Linux logo on boot.
>> >
>> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
>> > default logos are 80x80, which is larger than your display, so no logo
>> > is drawn.
>> > Fbtest has only the full color logo, so it will look bad on a monochrome
>> > display.
>>
>> I see. Should the test check for minimum num_colors and skip that test then?
>
> The test still works (you did see an ugly black-and-white penguin), doesn't it?
>

Fair enough. But when it defaulted to XRGB8888, it looked better. So I
thought that it was a regression. No strong opinion though if the test
should be skipped or not.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jul 17, 2023 at 11:33 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> >> penguin in test004 is not displayed correctly. I was expecting that to be
> >> >> working correctly since you mentioned to be using the Linux logo on boot.
> >> >
> >> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
> >> > default logos are 80x80, which is larger than your display, so no logo
> >> > is drawn.
> >> > Fbtest has only the full color logo, so it will look bad on a monochrome
> >> > display.
> >>
> >> I see. Should the test check for minimum num_colors and skip that test then?
> >
> > The test still works (you did see an ugly black-and-white penguin), doesn't it?
>
> Fair enough. But when it defaulted to XRGB8888, it looked better. So I
> thought that it was a regression. No strong opinion though if the test
> should be skipped or not.

IC, fbtest's mono_match_color() just finds the closest color (black or
white), while drm_fb_xrgb8888_to_gray8_line() uses a weighted average
of the RGB components. That might make a small but visible difference.

We could make it look even better using Floyd-Steinberg dithering... ;-)
Fbtest does have an unused match_color_error() helper, so I must have
had that in mind, initially...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jul 17, 2023 at 11:33 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> >> penguin in test004 is not displayed correctly. I was expecting that to be
> >> >> working correctly since you mentioned to be using the Linux logo on boot.
> >> >
> >> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
> >> > default logos are 80x80, which is larger than your display, so no logo
> >> > is drawn.
> >> > Fbtest has only the full color logo, so it will look bad on a monochrome
> >> > display.
> >>
> >> I see. Should the test check for minimum num_colors and skip that test then?
> >
> > The test still works (you did see an ugly black-and-white penguin), doesn't it?
>
> Fair enough. But when it defaulted to XRGB8888, it looked better. So I
> thought that it was a regression. No strong opinion though if the test
> should be skipped or not.

IC, fbtest's mono_match_color() just finds the closest color (black or
white), while drm_fb_xrgb8888_to_gray8_line() uses a weighted average
of the RGB components. That might make a small but visible difference.

We could make it look even better using Floyd-Steinberg dithering... ;-)
Fbtest does have an unused match_color_error() helper, so I must have
had that in mind, initially...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,
>
> On Mon, Jul 17, 2023 at 11:33 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> >> >> penguin in test004 is not displayed correctly. I was expecting that to be
>> >> >> working correctly since you mentioned to be using the Linux logo on boot.
>> >> >
>> >> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
>> >> > default logos are 80x80, which is larger than your display, so no logo
>> >> > is drawn.
>> >> > Fbtest has only the full color logo, so it will look bad on a monochrome
>> >> > display.
>> >>
>> >> I see. Should the test check for minimum num_colors and skip that test then?
>> >
>> > The test still works (you did see an ugly black-and-white penguin), doesn't it?
>>
>> Fair enough. But when it defaulted to XRGB8888, it looked better. So I
>> thought that it was a regression. No strong opinion though if the test
>> should be skipped or not.
>
> IC, fbtest's mono_match_color() just finds the closest color (black or
> white), while drm_fb_xrgb8888_to_gray8_line() uses a weighted average
> of the RGB components. That might make a small but visible difference.
>
> We could make it look even better using Floyd-Steinberg dithering... ;-)
> Fbtest does have an unused match_color_error() helper, so I must have
> had that in mind, initially...
>

Interesting. I'll take a look to that if I have time! But as mentioned,
all that is something to improve in your fbtest suite and the ssd130x
changes in the driver do look good to me and are working as expected.

> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,
>
> On Mon, Jul 17, 2023 at 11:33 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> >> >> penguin in test004 is not displayed correctly. I was expecting that to be
>> >> >> working correctly since you mentioned to be using the Linux logo on boot.
>> >> >
>> >> > Linux has logos for displays using 2, 16, and 256 colors. Note that the
>> >> > default logos are 80x80, which is larger than your display, so no logo
>> >> > is drawn.
>> >> > Fbtest has only the full color logo, so it will look bad on a monochrome
>> >> > display.
>> >>
>> >> I see. Should the test check for minimum num_colors and skip that test then?
>> >
>> > The test still works (you did see an ugly black-and-white penguin), doesn't it?
>>
>> Fair enough. But when it defaulted to XRGB8888, it looked better. So I
>> thought that it was a regression. No strong opinion though if the test
>> should be skipped or not.
>
> IC, fbtest's mono_match_color() just finds the closest color (black or
> white), while drm_fb_xrgb8888_to_gray8_line() uses a weighted average
> of the RGB components. That might make a small but visible difference.
>
> We could make it look even better using Floyd-Steinberg dithering... ;-)
> Fbtest does have an unused match_color_error() helper, so I must have
> had that in mind, initially...
>

Interesting. I'll take a look to that if I have time! But as mentioned,
all that is something to improve in your fbtest suite and the ssd130x
changes in the driver do look good to me and are working as expected.

> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat