| Message ID | 20220204134347.1187749-1-javierm@redhat.com |
|---|---|
| Headers | show |
| Series | drm/tiny: Add driver for Solomon SSD1307 OLED displays | expand |
On Fri, Feb 04, 2022 at 02:43:46PM +0100, Javier Martinez Canillas wrote: > To make sure that tools like the get_maintainer.pl script will suggest > to Cc me if patches are posted for this driver. > > Also include the Device Tree binding for the old ssd1307fb fbdev driver > since the new DRM driver was made compatible with the existing binding. ... > drivers/gpu/drm/drm_format_helper.c | 2 +- Nothing about this in the commit message... Stray change?
Hi Javier, On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306, > SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver. [...] > This is a v2 that addresses all the issues pointed in v1, thanks a lot > to everyone that gave me feedback and reviews. I tried to not miss any > comment, but there were a lot so forgive me if something is not there. Thanks for the update! > Changes in v2: [...] Note that the individual patches say "(no changes since v1)"? 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
Hello Geert, On 2/4/22 15:31, Geert Uytterhoeven wrote: > Hi Javier, > > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306, >> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver. > > [...] > >> This is a v2 that addresses all the issues pointed in v1, thanks a lot >> to everyone that gave me feedback and reviews. I tried to not miss any >> comment, but there were a lot so forgive me if something is not there. > > Thanks for the update! > You are welcome! >> Changes in v2: > > [...] > > Note that the individual patches say "(no changes since v1)"? > That's due patman (the tool I use to post patches) not being that smart. I only added the v2 changelog in the cover letter and not the individual patches to avoid adding noise, since there are a lot of changes since v1. But patman then thought that means individual patches had no changes... Best regards,
Am 04.02.22 um 16:52 schrieb Thomas Zimmermann: [...] >> +/** >> + * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed >> monochrome >> + * @dst: reversed monochrome destination buffer >> + * @dst_pitch: Number of bytes between two consecutive scanlines >> within dst >> + * @src: XRGB8888 source buffer >> + * @fb: DRM framebuffer >> + * @clip: Clip rectangle area to copy >> + * >> + * DRM doesn't have native monochrome support. >> + * Such drivers can announce the commonly supported XR24 format to >> userspace >> + * and use this function to convert to the native format. >> + * >> + * This function uses drm_fb_xrgb8888_to_gray8() to convert to >> grayscale and >> + * then the result is converted from grayscale to reversed monohrome. >> + */ >> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int >> dst_pitch, const void *src, >> + const struct drm_framebuffer *fb, >> + const struct drm_rect *clip) >> +{ >> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888)) >> + return; >> + >> + if (!dst_pitch) >> + dst_pitch = drm_rect_width(clip); >> + >> + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip); >> + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip); > > Converting from dst into dst can give incorrect results. At some point > we probably want to add restrict qualifiers to these pointers, to help > the compiler with optimizing. > > A better approach here is to pull the per-line conversion from > drm_fb_xrgb8888_to_gray8() into a separate helper and implement a > line-by-line conversion here. something like this: > > drm_fb_xrgb8888_to_mono_reversed() > { > char *tmp = kmalloc(size of a single line of gray8) > > for (heigth) { > drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...); > drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...); Here, I meant 'drm_fb_gray8_to_mono_reversed_line()' > > src += fb->pitches[0] > dst += dst_pitch; > } > > kfree(tmp); > } To elaborate, this is an example of what I meant by composable. In the future, we can generalize this function and hand-in 2 function pointers the do the conversion with tmp as intermediate buffer. That would work for any combination of formats that have a common intermediate format. > > Best regards > Thomas > >> +} >> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed); >> diff --git a/include/drm/drm_format_helper.h >> b/include/drm/drm_format_helper.h >> index b30ed5de0a33..85e551a5cbe6 100644 >> --- a/include/drm/drm_format_helper.h >> +++ b/include/drm/drm_format_helper.h >> @@ -43,4 +43,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned >> int dst_pitch, uint32_t dst_for >> const void *vmap, const struct drm_framebuffer *fb, >> const struct drm_rect *rect); >> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, >> const void *src, >> + const struct drm_rect *clip); >> + >> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int >> dst_pitch, const void *src, >> + const struct drm_framebuffer *fb, >> + const struct drm_rect *clip); >> + >> #endif /* __LINUX_DRM_FORMAT_HELPER_H */ >
Hi Am 04.02.22 um 20:31 schrieb Javier Martinez Canillas: > Hello Thomas, > > Thanks a lot for your feedback. > > On 2/4/22 16:52, Thomas Zimmermann wrote: > > [snip] > >>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels) >>> +{ >>> + unsigned int xb, i; >>> + >>> + for (xb = 0; xb < pixels / 8; xb++) { >> >> In practice, all mode widths are multiples of 8 because VGA mandated it. >> So it's ok-ish to assume this here. You should probably at least print a >> warning somewhere if (pixels % 8 != 0) >> > > Agreed. After sending the mail, I realized that some code copies only parts of the source around; specifically for damage handling. None of this is aligned to multiple of 8. So the copying could start and end in the middle of bytes. You'd need a pixel-offset value of some sort. If you don't want to handle this now, maybe at least detect this case and put a warning somewhere. > > [snip] > >>> + * DRM doesn't have native monochrome or grayscale support. >>> + * Such drivers can announce the commonly supported XR24 format to userspace >>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this >>> + * helper function to convert to the native format. >>> + */ >>> +void drm_fb_gray8_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src, >>> + const struct drm_rect *clip) >> >> There's a bug here. You want to pass in a drm_framebuffer as fourth >> argument. >> >>> +{ >>> + >>> + size_t height = drm_rect_height(clip); >>> + size_t width = drm_rect_width(clip); >>> + unsigned int y; >>> + const u8 *gray8 = src; >>> + u8 *mono = dst; >>> + >>> + if (!dst_pitch) >>> + dst_pitch = width; >> >> The dst_pitch is given in bytes. You have to device by 8. Here would be >> a good place to warn if (width % 8 != 0). >> > > Ok. > >>> + >>> + for (y = 0; y < height; y++) { >>> + drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch); >>> + mono += (dst_pitch / 8); >> >> The dst_pitch is already given in bytes. >> > > Yes, I know but for reversed mono we want only 1/8 of the width since we > are converting from 8 bits per pixel greyscale to 1 bit per pixel mono. > > Or am I misunderstanding what you meant ? I mean that if there are 80 pixel on a scanline, the value of dst_pitch is already 10. These pitch values are always given in bytes. Best regards Thomas > >>> + gray8 += dst_pitch; >> >> 'gray8 += fb->pitches[0]' would be correct. >> > > Ok. > > [snip] > >>> + */ >>> +void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src, >>> + const struct drm_framebuffer *fb, >>> + const struct drm_rect *clip) >>> +{ >>> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888)) >>> + return; >>> + >>> + if (!dst_pitch) >>> + dst_pitch = drm_rect_width(clip); >>> + >>> + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip); >>> + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip); >> >> Converting from dst into dst can give incorrect results. At some point >> we probably want to add restrict qualifiers to these pointers, to help >> the compiler with optimizing. >> >> A better approach here is to pull the per-line conversion from >> drm_fb_xrgb8888_to_gray8() into a separate helper and implement a >> line-by-line conversion here. something like this: >> >> drm_fb_xrgb8888_to_mono_reversed() >> { >> char *tmp = kmalloc(size of a single line of gray8) >> >> for (heigth) { >> drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...); >> drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...); >> >> src += fb->pitches[0] >> dst += dst_pitch; >> } >> >> kfree(tmp); >> } >> > > I see. Yes, that sounds a much better approach. I'll change it in v3. > > Best regards,
Hello Geert, Thanks a lot for testing! On 2/8/22 15:19, Geert Uytterhoeven wrote: > Hi Javier, > > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306, >> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver. > > I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an > OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore. > > Findings: > - Kernel size increased by 349 KiB, > - The "Memory:" line reports 412 KiB less memory, > - On top of that, "free" shows ca. 92 KiB more memory in use after > bootup. > - The logo (I have a custom monochrome logo enabled) is no longer shown. I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004 > - The screen is empty, with a (very very slow) flashing cursor in the > middle of the screen, with a bogus long line next to it, which I can > see being redrawn. > - Writing text (e.g. hello) to /dev/tty0, I first see the text, > followed by an enlargement of some of the characters. So far I was mostly testing using your fbtest repo tests and all of them (modulo test009 that says "Screen size too small for this test"). But I've tried now using as a VT and I see the same visual artifacts. I wonder what's the difference between fbcon and the way your tests use the fbdev API. > - "time ls" on the serial console (no files in the current directory, > so nothing to print) increases from 0.86s to 1.92s, so the system is > more loaded. As ssd1307fb relied on deferred I/O too, the slowdown > might be (partly) due to redrawing of the visual artefacts > mentioned above. > I was trying to first have the driver and then figure out how to optimize it. For v3 I'm using regmap to access instead of the I2C layer directly. I noticed that this is even slower but it makes the driver more clean and allows to support both I2C and SPI (untested but will include it as a WIP). > So while the displays seems to be initialized correctly, it looks like > there are some serious bugs in the conversion from xrgb8888 to > monochrome. > Yes, that's possible. I haven't tried to use it as a console before because the display resolution is just too small. But will include now in my tests. > Gr{oetje,eeting}s, > Best regards,
Hi Javier, On Tue, Feb 8, 2022 at 4:10 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > On 2/8/22 15:19, Geert Uytterhoeven wrote: > > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas > > <javierm@redhat.com> wrote: > >> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306, > >> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver. > > > > I gave it a try on an Adafruit FeatherWing 128x32 OLED, connected to an > > OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V softcore. > > > > Findings: > > - Kernel size increased by 349 KiB, > > - The "Memory:" line reports 412 KiB less memory, > > - On top of that, "free" shows ca. 92 KiB more memory in use after > > bootup. > > - The logo (I have a custom monochrome logo enabled) is no longer shown. > > I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004 I meant the kernel's logo (FB_LOGO_*),. Obviously you need to enable a smaller one, as the default 80x80 logo is too large, and thus can't be drawn on your 128x64 or my 128x32 display. > > - The screen is empty, with a (very very slow) flashing cursor in the > > middle of the screen, with a bogus long line next to it, which I can > > see being redrawn. > > - Writing text (e.g. hello) to /dev/tty0, I first see the text, > > followed by an enlargement of some of the characters. > > So far I was mostly testing using your fbtest repo tests and all of them > (modulo test009 that says "Screen size too small for this test"). > > But I've tried now using as a VT and I see the same visual artifacts. I > wonder what's the difference between fbcon and the way your tests use > the fbdev API. Fbcon does small writes to the shadow frame buffer, while fbtest writes to the mmap()ed /dev/fbX, causing a full page to be updated. 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
On 2/8/22 16:23, Geert Uytterhoeven wrote: [snip] >>> - The logo (I have a custom monochrome logo enabled) is no longer shown. >> >> I was able to display your tux monochrome with ./fbtest -f /dev/fb1 test004 > > I meant the kernel's logo (FB_LOGO_*),. Obviously you need to enable > a smaller one, as the default 80x80 logo is too large, and thus can't > be drawn on your 128x64 or my 128x32 display. > That makes sense. >>> - The screen is empty, with a (very very slow) flashing cursor in the >>> middle of the screen, with a bogus long line next to it, which I can >>> see being redrawn. >>> - Writing text (e.g. hello) to /dev/tty0, I first see the text, >>> followed by an enlargement of some of the characters. >> >> So far I was mostly testing using your fbtest repo tests and all of them >> (modulo test009 that says "Screen size too small for this test"). >> >> But I've tried now using as a VT and I see the same visual artifacts. I >> wonder what's the difference between fbcon and the way your tests use >> the fbdev API. > > Fbcon does small writes to the shadow frame buffer, while fbtest > writes to the mmap()ed /dev/fbX, causing a full page to be updated. > I see. Thanks for the information. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On 2/8/22 16:40, Javier Martinez Canillas wrote: > On 2/8/22 16:23, Geert Uytterhoeven wrote: [snip] >> >> Fbcon does small writes to the shadow frame buffer, while fbtest >> writes to the mmap()ed /dev/fbX, causing a full page to be updated. >> > > I see. Thanks for the information. > I found the bug. Partial updates where indeed broken and only a full screen update was working. I didn't notice because where using your fbtests that mmap and do a full update. Thanks a lot for reporting this, the issue should be fixed in v3. Best regards,
Hi Andy, On Wed, Feb 9, 2022 at 2:48 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Feb 08, 2022 at 04:10:49PM +0100, Javier Martinez Canillas wrote: > > On 2/8/22 15:19, Geert Uytterhoeven wrote: > > > On Fri, Feb 4, 2022 at 2:43 PM Javier Martinez Canillas > > > <javierm@redhat.com> wrote: > > > - Kernel size increased by 349 KiB, > > > - The "Memory:" line reports 412 KiB less memory, > > > - On top of that, "free" shows ca. 92 KiB more memory in use after > > > bootup. > > The memory consumption should really be taken seriously, because these kind of > displays are for embedded platforms with limited amount of resources. Thanks for your concern! Looking at the options that are auto-enabled, a few stand out that look like they're not needed on systems witch such small displays, or on legacy systems predating DDC: menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA select DRM_NOMODESET select DRM_PANEL_ORIENTATION_QUIRKS select HDMI Not everyone pays HDMI royalties ;-) select FB_CMDLINE select I2C select I2C_ALGOBIT I do need I2C, as it's the transport for my SSD1306 display, but not everyone needs it. select DMA_SHARED_BUFFER select SYNC_FILE # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate # device and dmabuf fd. Let's make sure that is available for our userspace. select KCMP And: config DRM_BRIDGE def_bool y depends on DRM help Bridge registration and lookup framework. config DRM_PANEL_BRIDGE def_bool y 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
On Wed, Feb 09, 2022 at 03:42:16PM +0100, Javier Martinez Canillas wrote: > On 2/9/22 15:27, Geert Uytterhoeven wrote: ... > Now, this is a reason why I mentioned that the old fbdev driver shouldn't > be removed yet. I agree on this conclusion. I think based on the fbtft resurrection discussion I can send a new version to unorphan it, route via fbdev, and leave under staging, so it will be a compromise between all stakeholders.
On Wed, Feb 09, 2022 at 05:32:15PM +0200, Andy Shevchenko wrote: > On Wed, Feb 09, 2022 at 03:42:16PM +0100, Javier Martinez Canillas wrote: > > On 2/9/22 15:27, Geert Uytterhoeven wrote: > > ... > > > Now, this is a reason why I mentioned that the old fbdev driver shouldn't > > be removed yet. > > I agree on this conclusion. > > I think based on the fbtft resurrection discussion I can send a new version > to unorphan it, route via fbdev, and leave under staging, so it will be a > compromise between all stakeholders. The DT bindings still don't belong anywhere in the main tree. Maxime
This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306, SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver. Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes: ./fbtest -f /dev/fb1 Using drawops cfb32 (32 bpp packed pixels) Available visuals: Monochrome Grayscale 256 Truecolor 8:8:8:0 Using visops truecolor Running all tests test001: PASSED test002: PASSED test003: PASSED test004: PASSED test005: PASSED test006: PASSED test008: PASSED Screen size too small for this test test010: PASSED Benchmarking... 10x10 squares: 414.41 Mpixels/s Benchmarking... 20x20 squares: 858.31 Mpixels/s Benchmarking... 50x50 squares: 1586.33 Mpixels/s test012: PASSED Benchmarking... R5 circles: 234.68 Mpixels/s Benchmarking... R10 circles: 498.24 Mpixels/s Benchmarking... R25 circles: 942.34 Mpixels/s test013: PASSED This is a v2 that addresses all the issues pointed in v1, thanks a lot to everyone that gave me feedback and reviews. I tried to not miss any comment, but there were a lot so forgive me if something is not there. Patch #1 adds two new helpers, drm_fb_gray8_to_mono_reversed() to convert from grayscale to monochrome and a drm_fb_xrgb8888_to_mono_reversed() to convert from XR24 to monochrome. The latter internally use thes former. Patch #2 adds the driver. The name ssd130x was used instead of ssd1307fb to denote that this driver is not only for SSD1307, but also for other displays from the same chip family. Patch #3 just adds a MAINTAINERS entry for the DRM driver and patch #4 adds myself as a co-maintainer of the existing Device Tree binding for ssd1307fb, since the same is shared between the fbdev and DRM drivers. Best regards, Javier Changes in v2: - Drop patch that was adding a DRM_MODE_CONNECTOR_I2C type. - Invert order of backlight {en,dis}able and display {on,off} (Sam Ravnborg) - Don't clear the screen and turn on display on probe (Sam Ravnborg) - Use backlight_get_brightness() macro to get BL brightness (Sam Ravnborg) - Use dev managed version of devm_backlight_device_register() (Sam Ravnborg) - Use dev_name(dev) for backlight name instead of an array (Sam Ravnborg) - Drop the .get_brightness callback since isn't needed (Sam Ravnborg) - Add myself as co-maintainer of the ssd1370fb DT binding (Sam Ravnborg) - Add Sam Ravnborg's acked-by tag to patch 3/4. - Rename driver to ssd130x since supports a display family (Thomas Zimmermann) - Drop the TINY prefix from the Kconfig symbol (Thomas Zimmermann) - Sort the Kconfig symbol dependencies alphabetically (Thomas Zimmermann) - Rename struct ssd130x_array to struct ssd130x_i2c_msg (Thomas Zimmermann) - Rename struct ssd130x_i2c_msg .type member to .cmd (Thomas Zimmermann) - Use sizeof(*foo) instead of sizeof(struct foo) (Thomas Zimmermann) - Use struct_size() macro to calculate sizeof(*foo) + len (Thomas Zimmermann) - Use kcalloc() instead of kmalloc_array() + memset() (Thomas Zimmermann) - Use shadow plane helpers virtual screen support (Thomas Zimmermann) - Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann) - Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann) - Use shadow plane helpers virtual screen support (Thomas Zimmermann) - Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann) - Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann) - Reorganize code in probe to make it more legible (Thomas Zimmermann) - ssd130x_write_cmd() uses varargs to simplify I2C code (Thomas Zimmermann) - Move regulator/pwm init logic to display pipe enable callback. - Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann) - Add a drm_fb_gray8_to_mono_reversed_line() helper (Thomas Zimmermann) Javier Martinez Canillas (4): drm/format-helper: Add drm_fb_{xrgb8888,gray8}_to_mono_reversed() drm/tiny: Add driver for Solomon SSD130X OLED displays MAINTAINERS: Add entry for Solomon SSD130X OLED displays DRM driver dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer .../bindings/display/solomon,ssd1307fb.yaml | 1 + MAINTAINERS | 7 + drivers/gpu/drm/drm_format_helper.c | 80 ++ drivers/gpu/drm/tiny/Kconfig | 12 + drivers/gpu/drm/tiny/Makefile | 1 + drivers/gpu/drm/tiny/ssd130x.c | 971 ++++++++++++++++++ include/drm/drm_format_helper.h | 7 + 7 files changed, 1079 insertions(+) create mode 100644 drivers/gpu/drm/tiny/ssd130x.c