Re: [PATCH v3 0/2] allow simple{fb, drm} drivers to be used on non-x86 EFI platforms
On 7/20/21 3:01 PM, Daniel Vetter wrote: > On Mon, Jul 19, 2021 at 09:10:52AM +0200, Ard Biesheuvel wrote: >> On Mon, 19 Jul 2021 at 04:59, Dave Airlie wrote: [snip] >>> >>> Can we just merge via drm-misc and make sure the acks are present and >>> I'll deal with the fallout if any. >>> >> >> Fine with me. Could you stick it on a separate branch so I can double >> check whether there are any issues wrt the EFI tree? > > It'll pop up in linux-next for integration testing or you can pick up the > patch here for test-merge if you want. > Thanks a lot Dave and Daniel! > And since Dave has given a blanket cheque for handling fallout he'll deal > with the need for fixups too if there's any. I also plan to look at any regression that might had been introduced by these. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 0/2] allow simple{fb, drm} drivers to be used on non-x86 EFI platforms
Hello Thomas, On 7/20/21 8:38 PM, Thomas Zimmermann wrote: > Am 20.07.21 um 15:59 schrieb Daniel Vetter: >> On Tue, Jul 20, 2021 at 03:42:45PM +0200, Javier Martinez Canillas wrote: >>> On 7/20/21 3:01 PM, Daniel Vetter wrote: >>>> On Mon, Jul 19, 2021 at 09:10:52AM +0200, Ard Biesheuvel wrote: >>>>> On Mon, 19 Jul 2021 at 04:59, Dave Airlie wrote: [snip] >>>>>> Can we just merge via drm-misc and make sure the acks are present and >>>>>> I'll deal with the fallout if any. >>>>>> >>>>> >>>>> Fine with me. Could you stick it on a separate branch so I can double >>>>> check whether there are any issues wrt the EFI tree? >>>> >>>> It'll pop up in linux-next for integration testing or you can pick up the >>>> patch here for test-merge if you want. >>>> This is what Daniel said... >>> >>> Thanks a lot Dave and Daniel! >> >> Oh I haven't merged them, I'm assuming Thomas will do that. Just figured > > Can I simply put the patches in to drm-misc-next? There was some talk > about a topic branch? > ... which AFAIU means that there's no need for a topic branch, since the patches will be present in linux-next. And the EFI folks can use that to check if there are any integration issues or regressions caused by these. > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 0/2] allow simple{fb, drm} drivers to be used on non-x86 EFI platforms
On 7/21/21 12:07 PM, Thomas Zimmermann wrote: > Hi > > Am 21.07.21 um 07:09 schrieb Javier Martinez Canillas: > ... >>> >>> Can I simply put the patches in to drm-misc-next? There was some talk >>> about a topic branch? >>> >> >> ... which AFAIU means that there's no need for a topic branch, since the >> patches will be present in linux-next. And the EFI folks can use that to >> check if there are any integration issues or regressions caused by these. > > Merged into drm-misc-next. > Thanks a lot Thomas for all your help! Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] drivers/firmware: fix sysfb depends to prevent build failures
The Generic System Framebuffers support is built when the COMPILE_TEST option is enabled. But this wrongly assumes that all the architectures declare a struct screen_info. This is true for most architectures, but at least the following do not: arc, m68k, microblaze, openrisc, parisc and s390. By attempting to make this compile testeable on all architectures, it leads to linking errors as reported by the kernel test robot for parisc: All errors (new ones prefixed by >>): hppa-linux-ld: drivers/firmware/sysfb.o: in function `sysfb_init': (.init.text+0x24): undefined reference to `screen_info' >> hppa-linux-ld: (.init.text+0x28): undefined reference to `screen_info' To prevent these errors only allow sysfb to be built on systems that are going to need it, which are x86 BIOS and EFI. The EFI Kconfig symbol is used instead of (ARM || ARM64 || RISC) because some of these architectures only declare a struct screen_info if EFI is enabled. And also, because the sysfb code is only used for EFI on these architectures. For !EFI the "simple-framebuffer" device is registered by OF when parsing the Device Tree Blob (if a DT node for this is defined). Reported-by: kernel test robot Signed-off-by: Javier Martinez Canillas --- drivers/firmware/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index af6719cc576..897f5f25c64 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT config SYSFB bool default y - depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST + depends on X86 || EFI config SYSFB_SIMPLEFB bool "Mark VGA/VBE/EFI FB as generic system framebuffer" -- 2.31.1
Re: linux-next: build failure due to the drm tree
Hello Mark,
On 7/26/21 11:36 PM, Mark Brown wrote:
> Hi all,
>
> Today's -next fails to build an arm64 allnoconfig:
>
> aarch64-none-linux-gnu-ld: drivers/firmware/sysfb.o: in function `sysfb_init':
> sysfb.c:(.init.text+0xc): undefined reference to `screen_info'
> aarch64-none-linux-gnu-ld: drivers/firmware/sysfb.o: relocation
> R_AARCH64_ADR_PREL_PG_HI21 against symbol `screen_info' which may bind
> externally can not be used when making a shared object; recompile with -fPIC
> sysfb.c:(.init.text+0xc): dangerous relocation: unsupported relocation
> aarch64-none-linux-gnu-ld: sysfb.c:(.init.text+0x10): undefined reference to
> `screen_info'
> make[1]: *** [/tmp/next/build/Makefile:1276: vmlinux] Error 1
>
> Caused by
>
> d391c58271072d0b0f ("drivers/firmware: move x86 Generic System Framebuffers
> support")
>
Yes, this was already reported by the kernel test robot and posted a fix
a few days ago: https://lore.kernel.org/patchwork/patch/1465623/
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH] efi: sysfb_efi: fix build when EFI is not set
On 7/27/21 7:04 AM, Randy Dunlap wrote: > When # CONFIG_EFI is not set, there are 2 definitions of > sysfb_apply_efi_quirks(). The stub from sysfb.h should be used > and the __init function from sysfb_efi.c should not be used. > > ../drivers/firmware/efi/sysfb_efi.c:337:13: error: redefinition of > ‘sysfb_apply_efi_quirks’ > __init void sysfb_apply_efi_quirks(struct platform_device *pd) > ^~ > In file included from ../drivers/firmware/efi/sysfb_efi.c:26:0: > ../include/linux/sysfb.h:65:20: note: previous definition of > ‘sysfb_apply_efi_quirks’ was here > static inline void sysfb_apply_efi_quirks(struct platform_device *pd) > ^~ > > Signed-off-by: Randy Dunlap > Cc: Ard Biesheuvel > Cc: linux-...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: Javier Martinez Canillas > Cc: Thomas Zimmermann > Cc: Mark Brown > Cc: linux-n...@vger.kernel.org > --- > drivers/firmware/efi/sysfb_efi.c |2 ++ > 1 file changed, 2 insertions(+) > > --- linext-20210726.orig/drivers/firmware/efi/sysfb_efi.c > +++ linext-20210726/drivers/firmware/efi/sysfb_efi.c > @@ -332,6 +332,7 @@ static const struct fwnode_operations ef > .add_links = efifb_add_links, > }; > > +#ifdef CONFIG_EFI > static struct fwnode_handle efifb_fwnode; > > __init void sysfb_apply_efi_quirks(struct platform_device *pd) > @@ -354,3 +355,4 @@ __init void sysfb_apply_efi_quirks(struc > pd->dev.fwnode = &efifb_fwnode; > } > } > +#endif > Thanks for the patch. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures
The Generic System Framebuffers support is built when the COMPILE_TEST
option is enabled. But this wrongly assumes that all the architectures
declare a struct screen_info.
This is true for most architectures, but at least the following do not:
arc, m68k, microblaze, openrisc, parisc and s390.
By attempting to make this compile testeable on all architectures, it
leads to linking errors as reported by the kernel test robot for parisc:
All errors (new ones prefixed by >>):
hppa-linux-ld: drivers/firmware/sysfb.o: in function `sysfb_init':
(.init.text+0x24): undefined reference to `screen_info'
>> hppa-linux-ld: (.init.text+0x28): undefined reference to `screen_info'
To prevent these errors only allow sysfb to be built on systems that are
going to need it, which are x86 BIOS and EFI.
The EFI Kconfig symbol is used instead of (ARM || ARM64 || RISC) because
some of these architectures only declare a struct screen_info if EFI is
enabled. And also, because the SYSFB code is only used for EFI on these
architectures. For !EFI the "simple-framebuffer" device is registered by
OF when parsing the Device Tree Blob (if a DT node for this was defined).
Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers
support")
Reported-by: kernel test robot
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Add a Fixes tag to the changelog.
drivers/firmware/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index af6719cc576b..897f5f25c64e 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
config SYSFB
bool
default y
- depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
+ depends on X86 || EFI
config SYSFB_SIMPLEFB
bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
--
2.31.1
Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures
Hello Geert,
On 7/27/21 12:03 PM, Geert Uytterhoeven wrote:
[snip]
> Thanks for your patch!
>
You are welcome.
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -254,7 +254,7 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>> config SYSFB
>> bool
>> default y
>> - depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
>> + depends on X86 || EFI
>
> Thanks, much better.
> Still, now this worm is crawling out of the X86 can, I'm wondering
> why this option is so important that it has to default to y?
> It is not just a dependency for SYSFB_SIMPLEFB, but also causes the
> inclusion of drivers/firmware/sysfb.c.
>
It defaults to yes because drivers/firmware/sysfb.c contains the logic
to register a "simple-framebuffer" device (or "efi-framebuffer" if the
CONFIG_SYSFB_SIMPLEFB Kconfig symbol is not enabled).
Not enabling this, would mean that a platform device to match a driver
supporting the EFI GOP framebuffer (e.g: simple{drm,fb} or efifb) will
not be registered. Which will lead to not having an early framebuffer.
The logic used to be in drivers/firmware/efi/efi-init.c, that's built
in if CONFIG_EFI is enabled. We just consolidated both X86 and EFI:
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8633ef82f101
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures
On 7/27/21 1:17 PM, Geert Uytterhoeven wrote:
[snip]
>> Not enabling this, would mean that a platform device to match a driver
>> supporting the EFI GOP framebuffer (e.g: simple{drm,fb} or efifb) will
>> not be registered. Which will lead to not having an early framebuffer.
>
> Do all (embedded) EFI systems have a frame buffer?
>
That's a good question. I don't know if all EFI firmwares are expected
to provide a GOP or not. But even the u-boot EFI stub provides one, if
video output is supported by u-boot on that system.
> Perhaps SYSFB should be selected by SYSFB_SIMPLEFB, FB_VESA,
> and FB_EFI?
>
It's another option, yes. I just thought that the use of select was not
encouraged and using depends was less fragile / error prone.
>> The logic used to be in drivers/firmware/efi/efi-init.c, that's built
>> in if CONFIG_EFI is enabled. We just consolidated both X86 and EFI:
>>
>> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8633ef82f101
>
> Thanks, I'm aware of that commit, as I was just about to reply to it,
> when I saw the patch is this thread ;-)
>
Ok :)
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2] drivers/firmware: fix SYSFB depends to prevent build failures
Hello Geert, On 7/27/21 1:39 PM, Geert Uytterhoeven wrote: [snip] >>> Perhaps SYSFB should be selected by SYSFB_SIMPLEFB, FB_VESA, >>> and FB_EFI? >> >> It's another option, yes. I just thought that the use of select was not >> encouraged and using depends was less fragile / error prone. > > Select is very useful for config symbols that are invisible to the user (i.e. > cannot be enabled/disabled manually). > Got it. I don't have a strong opinion on this really. In fact, the first version of the patch did use select for the arches but I got as feedback that should use depends instead: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg351961.html Granted, that was for the arches but you are proposing to do it for the drivers that match against the platform devices registered by sysfb. So it does make more sense to what I did in v1. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] drm: Remove unused code to load the non-existing fbcon.ko
Commit 6104c37094e7 ("fbcon: Make fbcon a built-time depency for fbdev")
changed the FRAMEBUFFER_CONSOLE Kconfig symbol from tristate to bool.
But the drm_kms_helper_init() function still attempts to load the fbcon
module, even when this is always built-in since the mentioned change.
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/drm_kms_helper_common.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_kms_helper_common.c
b/drivers/gpu/drm/drm_kms_helper_common.c
index f933da1656eb..47e92400548d 100644
--- a/drivers/gpu/drm/drm_kms_helper_common.c
+++ b/drivers/gpu/drm/drm_kms_helper_common.c
@@ -64,17 +64,6 @@ MODULE_PARM_DESC(edid_firmware,
static int __init drm_kms_helper_init(void)
{
- /*
-* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
-* but the module doesn't depend on any fb console symbols. At least
-* attempt to load fbcon to avoid leaving the system without a usable
-* console.
-*/
- if (IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION) &&
- IS_MODULE(CONFIG_FRAMEBUFFER_CONSOLE) &&
- !IS_ENABLED(CONFIG_EXPERT))
- request_module_nowait("fbcon");
-
return drm_dp_aux_dev_init();
}
--
2.31.1
[RFC PATCH 0/4] Allow to use DRM fbdev emulation layer with CONFIG_FB disabled
This patch series splits the fbdev core support in two different Kconfig
symbols: FB and FB_CORE. The motivation for this is to allow CONFIG_FB to
be disabled, while still using fbcon with the DRM fbdev emulation layer.
The reason for doing this is that now with simpledrm we could just boot
with simpledrm -> real DRM driver, without needing any legacy fbdev driver
(e.g: efifb or simplefb) even for the early console.
We want to do that in the Fedora kernel, but currently need to keep option
CONFIG_FB enabled and all fbdev drivers explicitly disabled, which makes
the configuration harder to maintain.
It is a RFC because I'm not that familiar with the fbdev core, but I have
tested and works with CONFIG_DRM_FBDEV_EMULATION=y and CONFIG_FB disabled.
This config automatically disables all the fbdev drivers that is our goal.
Patch 1/4 is just a clean up, patch 2/4 moves a couple of functions out of
fbsysfs.o, that are not related to sysfs attributes creation and finally
patch 3/4 makes the fbdev split that is mentioned above.
Patch 4/4 makes the DRM fbdev emulation depend on the new FB_CORE symbol
instead of FB. This could be done as a follow-up but for completeness is
also included in this series.
Best regards,
Javier
Javier Martinez Canillas (4):
fbdev: Rename fb_*_device() functions names to match what they do
fbdev: Move framebuffer_{alloc,release}() functions to fbmem.c
fbdev: Split frame buffer support in FB and FB_CORE symbols
drm: Make fbdev emulation depend on FB_CORE instead of FB
arch/x86/Makefile | 2 +-
arch/x86/video/Makefile| 2 +-
drivers/gpu/drm/Kconfig| 2 +-
drivers/video/console/Kconfig | 2 +-
drivers/video/fbdev/Kconfig| 57 +-
drivers/video/fbdev/core/Makefile | 13 +++--
drivers/video/fbdev/core/fbmem.c | 73 ++--
drivers/video/fbdev/core/fbsysfs.c | 77 +-
include/linux/fb.h | 18 ++-
9 files changed, 134 insertions(+), 112 deletions(-)
--
2.31.1
[RFC PATCH 1/4] fbdev: Rename fb_*_device() functions names to match what they do
The fb_init_device() and fb_cleanup_device() functions only register and
unregister sysfs attributes as their initialization and cleanup logic.
Let's rename these functions to better match what they actually do.
There's only a call to dev_set_drvdata() that's not related to the sysfs
registration, so move it to the do_register_framebuffer() function which
is where the device is created.
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fbmem.c | 8 +---
drivers/video/fbdev/core/fbsysfs.c | 6 ++
include/linux/fb.h | 4 ++--
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 71fb710f1ce..d886582c0a4 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1602,8 +1602,10 @@ static int do_register_framebuffer(struct fb_info
*fb_info)
/* Not fatal */
printk(KERN_WARNING "Unable to create device for framebuffer
%d; errno = %ld\n", i, PTR_ERR(fb_info->dev));
fb_info->dev = NULL;
- } else
- fb_init_device(fb_info);
+ } else {
+ dev_set_drvdata(fb_info->dev, fb_info);
+ fb_register_sysfs(fb_info);
+ }
if (fb_info->pixmap.addr == NULL) {
fb_info->pixmap.addr = kmalloc(FBPIXMAPSIZE, GFP_KERNEL);
@@ -1701,7 +1703,7 @@ static void do_unregister_framebuffer(struct fb_info
*fb_info)
fb_destroy_modelist(&fb_info->modelist);
registered_fb[fb_info->node] = NULL;
num_registered_fb--;
- fb_cleanup_device(fb_info);
+ fb_unregister_sysfs(fb_info);
#ifdef CONFIG_GUMSTIX_AM200EPD
{
struct fb_event event;
diff --git a/drivers/video/fbdev/core/fbsysfs.c
b/drivers/video/fbdev/core/fbsysfs.c
index 65dae05fff8..a040d6bd6c3 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -507,12 +507,10 @@ static struct device_attribute device_attrs[] = {
#endif
};
-int fb_init_device(struct fb_info *fb_info)
+int fb_register_sysfs(struct fb_info *fb_info)
{
int i, error = 0;
- dev_set_drvdata(fb_info->dev, fb_info);
-
fb_info->class_flag |= FB_SYSFS_FLAG_ATTR;
for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
@@ -531,7 +529,7 @@ int fb_init_device(struct fb_info *fb_info)
return 0;
}
-void fb_cleanup_device(struct fb_info *fb_info)
+void fb_unregister_sysfs(struct fb_info *fb_info)
{
unsigned int i;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 5950f8f5dc7..96111248a25 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -689,8 +689,8 @@ static inline bool fb_be_math(struct fb_info *info)
/* drivers/video/fbsysfs.c */
extern struct fb_info *framebuffer_alloc(size_t size, struct device *dev);
extern void framebuffer_release(struct fb_info *info);
-extern int fb_init_device(struct fb_info *fb_info);
-extern void fb_cleanup_device(struct fb_info *head);
+extern int fb_register_sysfs(struct fb_info *fb_info);
+extern void fb_unregister_sysfs(struct fb_info *head);
extern void fb_bl_default_curve(struct fb_info *fb_info, u8 off, u8 min, u8
max);
/* drivers/video/fbmon.c */
--
2.31.1
[RFC PATCH 2/4] fbdev: Move framebuffer_{alloc, release}() functions to fbmem.c
The framebuffer_alloc() and framebuffer_release() functions are much more
related with the functions in drivers/fbdev/core/fbmem.c than the ones in
drivers/fbdev/core/fbsysfs.c, that are only to manage sysfs attributes.
Signed-off-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fbmem.c | 65 +++
drivers/video/fbdev/core/fbsysfs.c | 71 --
2 files changed, 65 insertions(+), 71 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index d886582c0a4..2b22e46b815 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -57,6 +57,71 @@ bool fb_center_logo __read_mostly;
int fb_logo_count __read_mostly = -1;
+/**
+ * framebuffer_alloc - creates a new frame buffer info structure
+ *
+ * @size: size of driver private data, can be zero
+ * @dev: pointer to the device for this fb, this can be NULL
+ *
+ * Creates a new frame buffer info structure. Also reserves @size bytes
+ * for driver private data (info->par). info->par (if any) will be
+ * aligned to sizeof(long).
+ *
+ * Returns the new structure, or NULL if an error occurred.
+ *
+ */
+struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
+{
+#define BYTES_PER_LONG (BITS_PER_LONG/8)
+#define PADDING (BYTES_PER_LONG - (sizeof(struct fb_info) % BYTES_PER_LONG))
+ int fb_info_size = sizeof(struct fb_info);
+ struct fb_info *info;
+ char *p;
+
+ if (size)
+ fb_info_size += PADDING;
+
+ p = kzalloc(fb_info_size + size, GFP_KERNEL);
+
+ if (!p)
+ return NULL;
+
+ info = (struct fb_info *) p;
+
+ if (size)
+ info->par = p + fb_info_size;
+
+ info->device = dev;
+ info->fbcon_rotate_hint = -1;
+
+#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
+ mutex_init(&info->bl_curve_mutex);
+#endif
+
+ return info;
+#undef PADDING
+#undef BYTES_PER_LONG
+}
+EXPORT_SYMBOL(framebuffer_alloc);
+
+/**
+ * framebuffer_release - marks the structure available for freeing
+ *
+ * @info: frame buffer info structure
+ *
+ * Drop the reference count of the device embedded in the
+ * framebuffer info structure.
+ *
+ */
+void framebuffer_release(struct fb_info *info)
+{
+ if (!info)
+ return;
+ kfree(info->apertures);
+ kfree(info);
+}
+EXPORT_SYMBOL(framebuffer_release);
+
static struct fb_info *get_fb_info(unsigned int idx)
{
struct fb_info *fb_info;
diff --git a/drivers/video/fbdev/core/fbsysfs.c
b/drivers/video/fbdev/core/fbsysfs.c
index a040d6bd6c3..2c1e3f0effe 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -5,12 +5,6 @@
* Copyright (c) 2004 James Simmons
*/
-/*
- * Note: currently there's only stubs for framebuffer_alloc and
- * framebuffer_release here. The reson for that is that until all drivers
- * are converted to use it a sysfsification will open OOPSable races.
- */
-
#include
#include
#include
@@ -20,71 +14,6 @@
#define FB_SYSFS_FLAG_ATTR 1
-/**
- * framebuffer_alloc - creates a new frame buffer info structure
- *
- * @size: size of driver private data, can be zero
- * @dev: pointer to the device for this fb, this can be NULL
- *
- * Creates a new frame buffer info structure. Also reserves @size bytes
- * for driver private data (info->par). info->par (if any) will be
- * aligned to sizeof(long).
- *
- * Returns the new structure, or NULL if an error occurred.
- *
- */
-struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
-{
-#define BYTES_PER_LONG (BITS_PER_LONG/8)
-#define PADDING (BYTES_PER_LONG - (sizeof(struct fb_info) % BYTES_PER_LONG))
- int fb_info_size = sizeof(struct fb_info);
- struct fb_info *info;
- char *p;
-
- if (size)
- fb_info_size += PADDING;
-
- p = kzalloc(fb_info_size + size, GFP_KERNEL);
-
- if (!p)
- return NULL;
-
- info = (struct fb_info *) p;
-
- if (size)
- info->par = p + fb_info_size;
-
- info->device = dev;
- info->fbcon_rotate_hint = -1;
-
-#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
- mutex_init(&info->bl_curve_mutex);
-#endif
-
- return info;
-#undef PADDING
-#undef BYTES_PER_LONG
-}
-EXPORT_SYMBOL(framebuffer_alloc);
-
-/**
- * framebuffer_release - marks the structure available for freeing
- *
- * @info: frame buffer info structure
- *
- * Drop the reference count of the device embedded in the
- * framebuffer info structure.
- *
- */
-void framebuffer_release(struct fb_info *info)
-{
- if (!info)
- return;
- kfree(info->apertures);
- kfree(info);
-}
-EXPORT_SYMBOL(framebuffer_release);
-
static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
{
int err;
--
2.31.1
[RFC PATCH 3/4] fbdev: Split frame buffer support in FB and FB_CORE symbols
Currently the CONFIG_FB option has to be enabled even if no legacy fbdev drivers are needed, in order to have support for a framebuffer console. The DRM subsystem has a fbdev emulation layer, but depends on CONFIG_FB and so it can only be enabled if that dependency is enabled as well. That means fbdev drivers have to be explicitly disabled if users want to enable CONFIG_FB, only to use fbcon with the DRM fbdev emulation layer. This patch introduces a CONFIG_FB_CORE option that could be enabled just to have the core support needed for CONFIG_DRM_FBDEV_EMULATION, allowing CONFIG_FB to be disabled (and automatically disabling all fbdev drivers). The fbsysfs.o object is left out of fb_core.o, because that's not needed for KMS drivers and the DRM fbdev emulation layer. Signed-off-by: Javier Martinez Canillas --- arch/x86/Makefile | 2 +- arch/x86/video/Makefile | 2 +- drivers/video/console/Kconfig | 2 +- drivers/video/fbdev/Kconfig | 57 ++- drivers/video/fbdev/core/Makefile | 13 --- include/linux/fb.h| 14 6 files changed, 59 insertions(+), 31 deletions(-) diff --git a/arch/x86/Makefile b/arch/x86/Makefile index 44dd071d836..dc409539218 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -229,7 +229,7 @@ drivers-$(CONFIG_PCI)+= arch/x86/pci/ # suspend and hibernation support drivers-$(CONFIG_PM) += arch/x86/power/ -drivers-$(CONFIG_FB) += arch/x86/video/ +drivers-$(CONFIG_FB_CORE) += arch/x86/video/ # boot loader support. Several targets are kept for legacy purposes diff --git a/arch/x86/video/Makefile b/arch/x86/video/Makefile index 11640c11611..cb0735bd662 100644 --- a/arch/x86/video/Makefile +++ b/arch/x86/video/Makefile @@ -1,2 +1,2 @@ # SPDX-License-Identifier: GPL-2.0-only -obj-$(CONFIG_FB) += fbdev.o +obj-$(CONFIG_FB_CORE) += fbdev.o diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig index 840d9813b0b..0c562ed3495 100644 --- a/drivers/video/console/Kconfig +++ b/drivers/video/console/Kconfig @@ -71,7 +71,7 @@ config DUMMY_CONSOLE_ROWS config FRAMEBUFFER_CONSOLE bool "Framebuffer Console support" - depends on FB && !UML + depends on FB_CORE && !UML select VT_HW_CONSOLE_BINDING select CRC32 select FONT_SUPPORT diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index d33c5cd684c..cb9e8b503a5 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -9,10 +9,8 @@ config FB_CMDLINE config FB_NOTIFY bool -menuconfig FB - tristate "Support for frame buffer devices" - select FB_CMDLINE - select FB_NOTIFY +menuconfig FB_CORE + tristate "Core support for frame buffer devices" help The frame buffer device provides an abstraction for the graphics hardware. It represents the frame buffer of some video hardware and @@ -36,6 +34,19 @@ menuconfig FB <http://www.munted.org.uk/programming/Framebuffer-HOWTO-1.3.html> for more information. + This options enables the core support for frame buffer devices. + +menuconfig FB + tristate "Support for frame buffer device drivers" + depends on FB_CORE + select FB_CMDLINE + select FB_NOTIFY + help + This enables support for frame buffer devices (fbdev) drivers. + + The DRM subsystem provides an option to emulate fbdev devices but + this option allows legacy fbdev drivers to be enabled as well. + Say Y here and to the driver for your graphics board below if you are compiling a kernel for a non-x86 architecture. @@ -47,7 +58,7 @@ menuconfig FB config FIRMWARE_EDID bool "Enable firmware EDID" - depends on FB + depends on FB_CORE help This enables access to the EDID transferred from the firmware. On the i386, this is from the Video BIOS. Enable this if DDC/I2C @@ -62,7 +73,7 @@ config FIRMWARE_EDID config FB_DDC tristate - depends on FB + depends on FB_CORE select I2C_ALGOBIT select I2C @@ -75,7 +86,7 @@ config FB_BOOT_VESA_SUPPORT config FB_CFB_FILLRECT tristate - depends on FB + depends on FB_CORE help Include the cfb_fillrect function for generic software rectangle filling. This is used by drivers that don't provide their own @@ -83,7 +94,7 @@ config FB_CFB_FILLRECT config FB_CFB_COPYAREA tristate - depends on FB + depends on FB_CORE help Include the cfb_copyarea function for generic software area copying. This is used by drivers that don't provide their own (accelerated) @@ -91,7 +102,7 @@ config FB_CFB_COPYAREA config FB_CFB_IMAGEBLIT trist
[RFC PATCH 4/4] drm: Make fbdev emulation depend on FB_CORE instead of FB
Now that the fbdev core has been split in FB_CORE and FB, make DRM fbdev emulation layer to just depend on the former. This allows to disable the CONFIG_FB option if is not needed, which will avoid the need to explicitly disable all the legacy fbdev drivers. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 8fc40317f2b..7806adb02f1 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -103,7 +103,7 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS config DRM_FBDEV_EMULATION bool "Enable legacy fbdev support for your modesetting driver" depends on DRM - depends on FB + depends on FB_CORE select DRM_KMS_HELPER select FB_CFB_FILLRECT select FB_CFB_COPYAREA -- 2.31.1
[PATCH] drm/rockchip: remove early framebuffers before registering the fbdev
There are drivers that register framebuffer devices very early in the boot
process and make use of the existing framebuffer as setup by the firmware.
If one of those drivers has registered a fbdev, then the fbdev registered
by a DRM driver won't be bound to the framebuffer console. To avoid that,
remove any early framebuffer before registering a DRM framebuffer device.
By doing that, the fb mapped to the console is switched correctly from the
early fbdev to the one registered by the rockchip DRM driver:
[ 40.752420] fb0: switching to rockchip-drm-fb from EFI VGA
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
index 2fdc455c4ad..e3e5b63fdcc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
@@ -4,6 +4,7 @@
* Author:Mark Yao
*/
+#include
#include
#include
#include
@@ -124,6 +125,15 @@ int rockchip_drm_fbdev_init(struct drm_device *dev)
drm_fb_helper_prepare(dev, helper, &rockchip_drm_fb_helper_funcs);
+ /* Remove early framebuffers (e.g: simplefb or efifb) */
+ ret = drm_aperture_remove_framebuffers(false, "rockchip-drm-fb");
+ if (ret) {
+ DRM_DEV_ERROR(dev->dev,
+ "Failed to remove early framebuffers - %d.\n",
+ ret);
+ return ret;
+ }
+
ret = drm_fb_helper_init(dev, helper);
if (ret < 0) {
DRM_DEV_ERROR(dev->dev,
--
2.31.1
[PATCH v2] drm/rockchip: remove existing generic drivers to take over the device
There are drivers that register framebuffer devices very early in the boot
process and make use of the existing framebuffer as setup by the firmware.
If one of those drivers has registered a fbdev, then the fallback fbdev of
the DRM driver won't be bound to the framebuffer console. To avoid that,
remove any existing generic driver and take over the graphics device.
By doing that, the fb mapped to the console is switched correctly from the
early fbdev to the one registered by the rockchip DRM driver:
[ 40.752420] fb0: switching to rockchip-drm-fb from EFI VGA
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Move drm_aperture_remove_framebuffers() call to .bind callback (tzimmermann).
- Adapt subject line, commit message, etc accordingly.
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 212bd87c0c4..b730b8d5d94 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -16,6 +16,7 @@
#include
#include
+#include
#include
#include
#include
@@ -114,6 +115,15 @@ static int rockchip_drm_bind(struct device *dev)
struct rockchip_drm_private *private;
int ret;
+ /* Remove existing drivers that may own the framebuffer memory. */
+ ret = drm_aperture_remove_framebuffers(false, "rockchip-drm-fb");
+ if (ret) {
+ DRM_DEV_ERROR(dev,
+ "Failed to remove existing framebuffers - %d.\n",
+ ret);
+ return ret;
+ }
+
drm_dev = drm_dev_alloc(&rockchip_drm_driver, dev);
if (IS_ERR(drm_dev))
return PTR_ERR(drm_dev);
--
2.31.1
Re: [PATCH v2] drm/rockchip: remove existing generic drivers to take over the device
Hello Thomas, On 5/16/21 12:30 PM, Thomas Zimmermann wrote: > > > Am 16.05.21 um 09:48 schrieb Javier Martinez Canillas: >> There are drivers that register framebuffer devices very early in the boot >> process and make use of the existing framebuffer as setup by the firmware. >> >> If one of those drivers has registered a fbdev, then the fallback fbdev > of >> the DRM driver won't be bound to the framebuffer console. To avoid that, >> remove any existing generic driver and take over the graphics device. >> >> By doing that, the fb mapped to the console is switched correctly from the >> early fbdev to the one registered by the rockchip DRM driver: >> >> [ 40.752420] fb0: switching to rockchip-drm-fb from EFI VGA >> >> Signed-off-by: Javier Martinez Canillas > > Acked-by: Thomas Zimmermann > > Ping me if no one else merges the patch. > Sure, I will. And thanks a lot for your review! > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Software Engineer New Platform Technologies Enablement team RHEL Engineering
Re: [PATCH v3 0/2] allow simple{fb, drm} drivers to be used on non-x86 EFI platforms
On 6/25/21 3:09 PM, Javier Martinez Canillas wrote:
> The simplefb and simpledrm drivers match against a "simple-framebuffer"
> device, but for aarch64 this is only registered when using Device Trees
> and there's a node with a "simple-framebuffer" compatible string.
>
> There is no code to register a "simple-framebuffer" platform device when
> using EFI instead. In fact, the only platform device that's registered in
> this case is an "efi-framebuffer", which means that the efifb driver is
> the only driver supported to have an early console with EFI on aarch64.
>
> The x86 architecture platform has a Generic System Framebuffers (sysfb)
> support, that register a system frambuffer platform device. It either
> registers a "simple-framebuffer" for the simple{fb,drm} drivers or legacy
> VGA/EFI FB devices for the vgafb/efifb drivers.
>
> The sysfb is generic enough to be reused by other architectures and can be
> moved out of the arch/x86 directory to drivers/firmware, allowing the EFI
> logic used by non-x86 architectures to be folded into sysfb as well.
>
Any more comments on this series? It would be nice for this to land so the
simpledrm driver could be used on aarch64 EFI systems as well.
The patches have already been acked by x86 and DRM folks.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Re: [PATCH 2/9] fbdev: Put mmap for deferred I/O into drivers
On 3/3/22 21:58, Thomas Zimmermann wrote: > The fbdev mmap function fb_mmap() unconditionally overrides the > driver's implementation if deferred I/O has been activated. This > makes it hard to implement mmap with anything but a vmalloc()'ed > software buffer. That is specifically a problem for DRM, where > video memory is maintained by a memory manager. > > Leave the mmap handling to drivers and expect them to call the > helper for deferred I/O by thmeselves. > > Signed-off-by: Thomas Zimmermann > --- [snip] > > + /* > + * FB deferred I/O want you to handle mmap in your drivers. At a > + * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap(). > + */ > + if (WARN_ON_ONCE(info->fbdefio)) > + return -ENODEV; > + Maybe part of that comment could be printed as a WARN_ON_ONCE() message ? Regardless, the patch looks good to me: Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 3/9] fbdev: Track deferred-I/O pages in pageref struct
On 3/3/22 21:58, Thomas Zimmermann wrote:
> Store the per-page state for fbdev's deferred I/O in struct
> fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
> that have to be flushed out to video memory. Update all affected
> drivers.
>
> Like with pages before, fbdev acquires a pageref when an mmaped page
> of the framebuffer is being written to. It holds the pageref in a
> list of all currently written pagerefs until it flushes the written
> pages to video memory. Writeback occurs periodically. After writeback
> fbdev releases all pagerefs and builds up a new dirty list until the
> next writeback occurs.
>
> Using pagerefs has a number of benefits.
>
> For pages of the framebuffer, the deferred I/O code used struct
> page.lru as an entry into the list of dirty pages. The lru field is
> owned by the page cache, which makes deferred I/O incompatible with
> some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
> struct fb_deferred_io_pageref now provides an entry into a list of
> dirty framebuffer pages, free'ing lru for use with the page cache.
>
> Drivers also assumed that struct page.index is the page offset into
> the framebuffer. This is not true for DRM buffers, which are located
> at various offset within a mapped area. struct fb_deferred_io_pageref
> explicitly stores an offset into the framebuffer. struct page.index
> is now only the page offset into the mapped area.
>
> These changes will allow DRM to use fbdev deferred I/O without an
> intermediate shadow buffer.
>
As mentioned, this is a very nice cleanup.
> Signed-off-by: Thomas Zimmermann
> ---
[snip]
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> index 33f355907fbb..a35f695727c9 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> @@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
> struct vmw_fb_par *par = info->par;
> unsigned long start, end, min, max;
> unsigned long flags;
> - struct page *page;
> + struct fb_deferred_io_pageref *pageref;
> int y1, y2;
>
> min = ULONG_MAX;
> max = 0;
> - list_for_each_entry(page, pagelist, lru) {
> + list_for_each_entry(pageref, pagelist, list) {
> + struct page *page = pageref->page;
> start = page->index << PAGE_SHIFT;
Do you think that may be worth it to also decouple the struct page.index and
store the index as a struct fb_deferred_io_pageref.index field ?
It's unlikely that this would change but it feels as if the layers are more
separated that way, since no driver will access struct page fields directly.
The mapping would be 1:1 anyways just like it's the case for the page offset.
In fact, that could allow to even avoid declaring a struct page *page here.
[snip]
> @@ -340,7 +340,8 @@ static void fbtft_deferred_io(struct fb_info *info,
> struct list_head *pagelist)
> spin_unlock(&par->dirty_lock);
>
> /* Mark display lines as dirty */
> - list_for_each_entry(page, pagelist, lru) {
> + list_for_each_entry(pageref, pagelist, list) {
> + struct page *page = pageref->page;
Same here and the other drivers. You only need the page for the index AFAICT.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 4/9] fbdev: Export helper for implementing page_mkwrite
On 3/3/22 21:58, Thomas Zimmermann wrote: > Refactor the page-write handler and export it as helper function > fb_deferred_io_page_mkwrite(). Drivers that implement struct > vm_operations_struct.page_mkwrite for deferred I/O should use the > function to let fbdev track written pages of mmap'ed framebuffer > memory. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 5/9] drm/fb-helper: Separate deferred I/O from shadow buffers
On 3/3/22 21:58, Thomas Zimmermann wrote: > DRM drivers will be able to handle deferred I/O by themselves. So > a driver will be able to use deferred I/O without an intermediate > shadow buffer. > > Prepare fbdev emulation by separating shadow buffers and deferred > I/O from each other. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 6/9] drm/fb-helper: Provide callback to create fbdev dumb buffers
On 3/3/22 21:58, Thomas Zimmermann wrote:
> Provide struct drm_driver.dumb_create_fbdev, a callback hook for
> fbdev dumb buffers. Wire up fbdev and client helpers to use the new
> interface, if present.
>
> This acknowledges the fact that fbdev buffers are different. The most
> significant difference to regular GEM BOs is in mmap semantics. Fbdev
> userspace treats the pages as video memory, which makes it impossible
> to ever move the mmap'ed buffer. Hence, drivers ussually have to pin
> the BO permanently or install an intermediate shadow buffer for mmap.
>
> So far, fbdev memory came from dumb buffers and DRM drivers had no
> means of detecting this without reimplementing a good part of the fbdev
> code. With the new callback, drivers can perma-pin fbdev buffer objects
> if needed.
>
> Several drivers also require damage handling, which fbdev implements
> with its deferred I/O helpers. The new callback allows a driver's memory
> manager to set up a suitable mmap.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_client.c| 14 +++
> drivers/gpu/drm/drm_crtc_internal.h | 3 +++
> drivers/gpu/drm/drm_dumb_buffers.c | 36 +
> drivers/gpu/drm/drm_fb_helper.c | 26 +
> include/drm/drm_client.h| 3 ++-
> include/drm/drm_drv.h | 17 ++
> 6 files changed, 84 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index af3b7395bf69..c964064651d5 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -247,7 +247,8 @@ static void drm_client_buffer_delete(struct
> drm_client_buffer *buffer)
> }
>
> static struct drm_client_buffer *
> -drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32
> height, u32 format)
> +drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32
> height, u32 format,
> + bool fbdev)
> {
> const struct drm_format_info *info = drm_format_info(format);
> struct drm_mode_create_dumb dumb_args = { };
> @@ -265,7 +266,10 @@ drm_client_buffer_create(struct drm_client_dev *client,
> u32 width, u32 height, u
> dumb_args.width = width;
> dumb_args.height = height;
> dumb_args.bpp = info->cpp[0] * 8;
> - ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
> + if (fbdev)
Maybe if (defined(CONFIG_DRM_FBDEV_EMULATION) && fbdev) ?
> + ret = drm_mode_create_dumb_fbdev(dev, &dumb_args, client->file);
And drm_mode_create_dumb_fbdev() could just be made a stub if
CONFIG_DRM_FBDEV_EMULATION isn't enabled.
I believe the only usage of the DRM client API currently is the fbdev
emulation layer anyways? But still makes sense I think to condtionally
compile that since drm_client.o is built in the drm.ko module and the
drm_fb_helper.o only included if fbdev emulation has been enabled.
> + else
> + ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
> if (ret)
> goto err_delete;
>
> @@ -402,6 +406,7 @@ static int drm_client_buffer_addfb(struct
> drm_client_buffer *buffer,
> * @width: Framebuffer width
> * @height: Framebuffer height
> * @format: Buffer format
> + * @fbdev: True if the client provides an fbdev device, or false otherwise
> *
An emulated fbdev device ?
Other than those small nit,
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 7/9] drm/fb-helper: Provide fbdev deferred-I/O helpers
On 3/3/22 21:58, Thomas Zimmermann wrote: > Add drm_fb_helper_vm_page_mkwrite(), a helper to track pages written > by fbdev userspace. DRM drivers should use this function to implement > fbdev deferred I/O. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers
On 3/3/22 21:58, Thomas Zimmermann wrote:
> Implement struct drm_driver.dumb_create_fbdev for GEM SHMEM. The
> created buffer object returned by this function implements deferred
> I/O for its mmap operation.
>
> Add this feature to a number of drivers that use GEM SHMEM helpers
> as shadow planes over their regular video memory. The new macro
> DRM_GEM_SHMEM_DRIVER_OPS_WITH_SHADOW_PLANES sets the regular GEM
> functions and dumb_create_fbdev in struct drm_driver. Fbdev emulation
> on these drivers will now mmap the GEM SHMEM pages directly with
> deferred I/O without an intermediate shadow buffer.
>
> Signed-off-by: Thomas Zimmermann
> ---
[snip]
> @@ -49,8 +50,20 @@ static const struct drm_gem_object_funcs
> drm_gem_shmem_funcs = {
> .vm_ops = &drm_gem_shmem_vm_ops,
> };
>
> +static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {
> + .free = drm_gem_shmem_object_free,
> + .print_info = drm_gem_shmem_object_print_info,
> + .pin = drm_gem_shmem_object_pin,
> + .unpin = drm_gem_shmem_object_unpin,
> + .get_sg_table = drm_gem_shmem_object_get_sg_table,
> + .vmap = drm_gem_shmem_object_vmap,
> + .vunmap = drm_gem_shmem_object_vunmap,
> + .mmap = drm_gem_shmem_object_mmap_fbdev,
> + .vm_ops = &drm_gem_shmem_vm_ops_fbdev,
The drm_gem_shmem_funcs_fbdev is the same than drm_gem_shmem_funcs but
.mmap and .vm_ops callbacks. Maybe adding a macro to declare these two
struct drm_gem_object_funcs could make easier to maintain it long term ?
> +};
> +
> static struct drm_gem_shmem_object *
> -__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
> +__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private,
> bool fbdev)
> {
> struct drm_gem_shmem_object *shmem;
> struct drm_gem_object *obj;
> @@ -70,8 +83,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t
> size, bool private)
> obj = &shmem->base;
> }
>
> - if (!obj->funcs)
> - obj->funcs = &drm_gem_shmem_funcs;
> + if (!obj->funcs) {
> + if (fbdev)
Same question than in patch #6, maybe
if (defined(CONFIG_DRM_FBDEV_EMULATION) && fbdev) ?
[snip]
> + */
> +int drm_gem_shmem_dumb_create_fbdev(struct drm_file *file, struct drm_device
> *dev,
> + struct drm_mode_create_dumb *args)
> +{
> +#if defined(CONFIG_DRM_FBDEV_EMULATION)
> + return __drm_gem_shmem_dumb_create(file, dev, true, args);
> +#else
> + return -ENOSYS;
> +#endif
> +}
I believe the correct errno code here should be -ENOTSUPP.
[snip]
> + /* We don't use vmf->pgoff since that has the fake offset */
> + page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
I see this (vmf->address - vma->vm_start) calculation in many places of
this series. Maybe we could add a macro to calculate the offset instead ?
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 9/9] drm/virtio: Implement dumb_create_fbdev with GEM SHMEM helpers
On 3/3/22 21:58, Thomas Zimmermann wrote:
> Implement struct drm_driver.dumb_create_fbdev with the helpers
> provided by GEM SHMEM. Fbdev deferred I/O will now work without
> an intermediate shadow buffer for mmap.
>
> As the virtio driver replaces several of the regular GEM SHMEM
> functions with its own implementation, some additional code is
> necessary make fbdev optimization work. Especially, the driver
> has to provide buffer-object functions for fbdev. Add the hook
> struct drm_driver.gem_create_object_fbdev, which is similar to
> struct drm_driver.gem_create_object and allows for the creation
> of dedicated fbdev buffer objects. Wire things up within GEM
> SHMEM.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++-
> drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +
> drivers/gpu/drm/virtio/virtgpu_drv.h| 2 +
> drivers/gpu/drm/virtio/virtgpu_object.c | 54 +++--
> include/drm/drm_drv.h | 10 +
> 5 files changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index ab7cb7d896c3..225aa17895bd 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -71,7 +71,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t
> size, bool private, bool f
>
> size = PAGE_ALIGN(size);
>
> - if (dev->driver->gem_create_object) {
> + if (dev->driver->gem_create_object_fbdev && fbdev) {
Same comment here to check if fbdev emulation is enabled or maybe is not
worht it ? But I think this hints the compiler to optimize the if branch.
[snip]
> +}
> +#else
> +struct drm_gem_object *virtio_gpu_create_object_fbdev(struct drm_device *dev,
> + size_t size)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
As mentioned, I believe this should be ERR_PTR(-ENOTSUPP) instead.
Feel free to ignore all this nits if you consider that are not applicable.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 1/2] drm: ssd130x: Fix COM scan direction register mask
Hello Chen-Yu,
Thanks a lot for your patch.
On 3/8/22 17:07, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai
>
> The SSD130x's command to toggle COM scan direction uses bit 3 and only
> bit 3 to set the direction of the scanout. The driver has an incorrect
> GENMASK(3, 2), causing the setting to be set on bit 2, rendering it
> ineffective.
>
> Fix the mask to only bit 3, so that the requested setting is applied
> correctly.
>
Sigh, you are correct. I thought that triple checked the datasheet
when writing this but I got it wrong anyways...
> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Chen-Yu Tsai
> ---
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/2] drm: ssd130x: Always apply segment remap setting
On 3/8/22 17:07, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai
>
> Currently the ssd130x driver only sets the segment remap setting when
> the device tree requests it; it however does not clear the setting if
> it is not requested. This leads to the setting incorrectly persisting
> if the hardware is always on and has no reset GPIO wired. This might
> happen when a developer is trying to find the correct settings for an
> unknown module, and cause the developer to get confused because the
> settings from the device tree are not consistently applied.
>
> Make the driver apply the segment remap setting consistently, setting
> the value correctly based on the device tree setting. This also makes
> this setting's behavior consistent with the other settings, which are
> always applied.
>
Nice catch. This is certainly much better. Thanks!
> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Chen-Yu Tsai
> ---
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 3/9] fbdev: Track deferred-I/O pages in pageref struct
Hello Thomas, On 3/9/22 09:36, Thomas Zimmermann wrote: [snip] > > I thought about using pageref->offset in fbdev drivers as well. It would > be more correct, but didn't want to add unnecessary churn. Especially > since I cannot test most of the fbdev drivers. If you think it's worth > it, I'd change the drivers as well. > Thanks for the explanation. No, I don't think is worth it or at least as as part of this series. > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers
On 3/9/22 09:47, Thomas Zimmermann wrote:
[snip]
>>>
>>> +static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {
>>> + .free = drm_gem_shmem_object_free,
>>> + .print_info = drm_gem_shmem_object_print_info,
>>> + .pin = drm_gem_shmem_object_pin,
>>> + .unpin = drm_gem_shmem_object_unpin,
>>> + .get_sg_table = drm_gem_shmem_object_get_sg_table,
>>> + .vmap = drm_gem_shmem_object_vmap,
>>> + .vunmap = drm_gem_shmem_object_vunmap,
>>> + .mmap = drm_gem_shmem_object_mmap_fbdev,
>>> + .vm_ops = &drm_gem_shmem_vm_ops_fbdev,
>>
>> The drm_gem_shmem_funcs_fbdev is the same than drm_gem_shmem_funcs but
>> .mmap and .vm_ops callbacks. Maybe adding a macro to declare these two
>> struct drm_gem_object_funcs could make easier to maintain it long term ?
>
> I see you point, but I'm undecided about this. Such macros also add some
> amount of obfuscation. I'm not sure if it's worth it for an internal
> constant. And since the fbdev buffer is never exported, we could remove
> some of the callbacks. AFAICT we never call pin, unpin or get_sg_table.
>
Yeah, that's true too. It was just a suggestion, I'm OK with patch as is.
> Best regards
> Thomas
>
>>
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 9/9] drm/virtio: Implement dumb_create_fbdev with GEM SHMEM helpers
On 3/9/22 09:52, Thomas Zimmermann wrote:
[snip]
>>> +struct drm_gem_object *virtio_gpu_create_object_fbdev(struct drm_device
>>> *dev,
>>> + size_t size)
>>> +{
>>> + return ERR_PTR(-ENOSYS);
>>> +}
>>
>> As mentioned, I believe this should be ERR_PTR(-ENOTSUPP) instead.
>
> I've been wondering about this as well. I finally went with the rules at
> [1]. All the variants of ENOTOP/ENOTSUPP seem to be for specific use
> cases, such as a certain feature is not implemented be a specific
> interface (e.g., sockets for EOPNOTSUPP). ENOSYS is the only general
> error that indicates that an entire interface is missing. Even though
> checkpatch.pl warns that it's only for system calls.
>
> Best regards
> Thomas
>
> [1] https://www.cs.helsinki.fi/linux/linux-kernel/2002-30/1135.html
>
Thanks for the link. It would be good to have an authoritative guideline
about this in the kernel documentation (and make checkpatch.pl aware).
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v6 2/6] drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()
Hello Geert,
Thanks a lot for your feedback.
On 3/8/22 17:13, Geert Uytterhoeven wrote:
[snip]
>> +
>> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src,
>> unsigned int pixels,
>
> "pixels" is not the number of pixels to process, but the number of
> bytes in the monochrome destination buffer.
>
Right, that parameter name is misleading / incorrect indeed. Probably
should be changed by dst_pitch or dst_stride.
>> + unsigned int start_offset,
>> unsigned int end_len)
>> +{
>> + unsigned int xb, i;
>> +
>> + for (xb = 0; xb < pixels; xb++) {
>> + unsigned int start = 0, end = 8;
>> + u8 byte = 0x00;
>> +
>> + if (xb == 0 && start_offset)
>> + start = start_offset;
>> +
>> + if (xb == pixels - 1 && end_len)
>> + end = end_len;
>> +
>> + for (i = start; i < end; i++) {
>> + unsigned int x = xb * 8 + i;
>> +
>> + byte >>= 1;
>> + if (src[x] >> 7)
>> + byte |= BIT(7);
>> + }
>> + *dst++ = byte;
>> + }
>
> The above is IMHO very hard to read.
> I think it can be made simpler by passing the total number of pixels
> to process instead of "pixels" (which is bytes) and "end_len".
>
Agreed that's hard to read. I think is better if you propose a patch
with your idea to make it simpler.
[snip]
>> +void drm_fb_xrgb_to_mono_reversed(void *dst, unsigned int dst_pitch,
>> const void *vaddr,
>> + const struct drm_framebuffer *fb,
>> const struct drm_rect *clip)
>> +{
>> + unsigned int linepixels = drm_rect_width(clip);
>> + unsigned int lines = clip->y2 - clip->y1;
>
> drm_rect_height(clip)?
>
Yes, unsure why didn't use it since used drm_rect_width() for the width.
>> + unsigned int cpp = fb->format->cpp[0];
>> + unsigned int len_src32 = linepixels * cpp;
>> + struct drm_device *dev = fb->dev;
>> + unsigned int start_offset, end_len;
>> + unsigned int y;
>> + u8 *mono = dst, *gray8;
>> + u32 *src32;
>> +
>> + if (drm_WARN_ON(dev, fb->format->format != DRM_FORMAT_XRGB))
>> + return;
>> +
>> + /*
>> +* The reversed mono destination buffer contains 1 bit per pixel
>> +* and destination scanlines have to be in multiple of 8 pixels.
>> +*/
>> + if (!dst_pitch)
>> + dst_pitch = DIV_ROUND_UP(linepixels, 8);
>
> This is not correct when clip->x1 is not a multiple of 8 pixels.
> Should be:
>
> DIV_ROUND_UP(linepixels + clip->x1 % 8, 8);
>
Agreed.
>> +
>> + drm_WARN_ONCE(dev, dst_pitch % 8 != 0, "dst_pitch is not a multiple
>> of 8\n");
>
> This triggers for me: dst_pitch = 1.
> Which is perfectly fine, when flashing an 8-pixel wide cursor ;-)
>
Indeed. I think we should just drop that warn.
Do you want me to post patches for all these or would you do it
when simplifying the drm_fb_gray8_to_mono_reversed_line() logic ?
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays
On 3/8/22 17:30, Geert Uytterhoeven wrote:
> Hi Javier,
>
> On Mon, Feb 14, 2022 at 2:37 PM Javier Martinez Canillas
> wrote:
>> This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
>> OLED display controllers.
>>
>> It's only the core part of the driver and a bus specific driver is needed
>> for each transport interface supported by the display controllers.
>>
>> Signed-off-by: Javier Martinez Canillas
>
> Thanks for your patch, which is now commit a61732e808672cfa ("drm:
> Add driver for Solomon SSD130x OLED displays") in drm/drm-next
>
> Sorry for the delay, but finally I gave it a try on my Adafruit
> FeatherWing 128x32 OLED.
> Some of the weird issues (cursor disappears after printing some text,
> more text also doesn't appear until I clear the display) are still there.
I see. Thought that I tested using it as a console and it did work
correctly for me. I'll do more tests again.
> Unfortunately a regression was introduced since your v3: printed
> text is mirrored upside-down. I.e. "E" is rendered correctly, but "L"
> turns into "Γ" (Greek Gamma).
> I suspect something went wrong with the display initialization
> sequence.
>
Could you please try Chen-Yu's fix for the COM scan direction mask ?
https://lists.freedesktop.org/archives/dri-devel/2022-March/345915.html
I made a mistake when converting to use the GENMASK() and FIELD_PREP()
macros in v4 as suggested by Andy. The SSD130X_SET_COM_SCAN_DIR_MASK
wasn't correct which would explain the output to be vertically flipped.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 01/10] drm/fourcc: Add drm_format_info_bpp() helper
Hello Geert, On 3/7/22 21:52, Geert Uytterhoeven wrote: > Add a helper to retrieve the actual number of bits per pixel for a > plane, taking into account the number of characters and pixels per > block for tiled formats. > > Signed-off-by: Geert Uytterhoeven > --- Patch looks good to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 02/10] drm/fourcc: Add drm_format_info.is_color_indexed flag
On 3/7/22 21:52, Geert Uytterhoeven wrote: > Add a flag to struct drm_format_info to indicate if a format is > color-indexed, similar to the existing .is_yuv flag. > > This way generic code and drivers can just check this flag, instead of > checking against a list of fourcc formats. > > Signed-off-by: Geert Uytterhoeven > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 03/10] drm/client: Use actual bpp when allocating frame buffers
On 3/7/22 21:52, Geert Uytterhoeven wrote: > When allocating a frame buffer, the number of bits per pixel needed is > derived from the deprecated drm_format_info.cpp[] field. While this > works for formats using less than 8 bits per pixel, it does lead to a > large overallocation. > > Reduce memory consumption by using the actual number of bits per pixel > instead. > > Signed-off-by: Geert Uytterhoeven > Acked-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 04/10] drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB
On 3/7/22 21:52, Geert Uytterhoeven wrote: > When userspace queries the properties of a frame buffer, the number of > bits per pixel is derived from the deprecated drm_format_info.cpp[] > field, which does not take into account block sizes. > > Fix this by using the actual number of bits per pixel instead. > > Signed-off-by: Geert Uytterhoeven > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]
On 3/7/22 21:52, Geert Uytterhoeven wrote: > Introduce fourcc codes for color-indexed frame buffer formats with two, > four, and sixteen colors, and provide a mapping from bit per pixel and > depth to fourcc codes. > > As the number of bits per pixel is less than eight, these rely on proper > block handling for the calculation of bits per pixel and pitch. > > Signed-off-by: Geert Uytterhoeven > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]
On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add support for color-indexed frame buffer formats with two, four, and
> sixteen colors to the DRM framebuffer helper functions:
> 1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
> 2. For color-indexed modes, the length of the color bitfields must be
> set to the color depth, else the logo code may pick a logo with too
> many colors. Drop the incorrect DAC width comment, which
> originates from the i915 driver.
> 3. Accept C[124] modes when validating or filling in struct
> fb_var_screeninfo, and use the correct number of bits per pixel.
> 4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
> modes.
>
> Signed-off-by: Geert Uytterhoeven
[snip]
> static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> -uint32_t depth)
> +bool is_color_indexed)
> {
> info->fix.type = FB_TYPE_PACKED_PIXELS;
> - info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> - FB_VISUAL_TRUECOLOR;
> + info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> + : info->fix.visual;
Shouldn't this be the following instead ?
+ info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
+ : FB_VISUAL_TRUECOLOR;
Other than that the patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 RFC 07/10] drm/gem-fb-helper: Use actual bpp for size calculations
On 3/7/22 21:52, Geert Uytterhoeven wrote: > The AFBC helpers derive the number of bits per pixel from the deprecated > drm_format_info.cpp[] field, which does not take into account block > sizes. > > Fix this by using the actual number of bits per pixel instead. > > Signed-off-by: Geert Uytterhoeven > --- > RFC, as this code path was untested. > Looks good to me but haven't tested either. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 RFC 08/10] drm/fourcc: Document that single-channel "red" can be any color
On 3/7/22 21:52, Geert Uytterhoeven wrote: > Traditionally, the first channel has been called the "red" channel, but > the fourcc values for single-channel "red" formats can also be used for > other light-on-dark displays, like grayscale. > > Signed-off-by: Geert Uytterhoeven > --- Yes, I learned that "Red" actually meant just a color channel that may not be red in one of the thread about fourcc formats. So I agree that would be good to have a comment about this. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 RFC 09/10] drm/fourcc: Add DRM_FORMAT_R[124]
On 3/7/22 21:52, Geert Uytterhoeven wrote: > Introduce fourcc codes for single-channel light-on-dark frame buffer > formats with two, four, and sixteen intensity levels. > > As the number of bits per pixel is less than eight, these rely on proper > block handling for the calculation of bits per pixel and pitch. > > Signed-off-by: Geert Uytterhoeven > --- IIRC the agreement was that it was worth to have both Cx and Rx fourcc formats separately, so this patch makes sense to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 RFC 10/10] drm/fourcc: Add DRM_FORMAT_D[1248]
On 3/7/22 21:52, Geert Uytterhoeven wrote: > As Rn is light-on-dark, and Cn can be any colors, there are currently > no fourcc codes to describe dark-on-light displays. > > Introduce fourcc codes for a single-channel dark-on-light frame buffer > format with two, four, sixteen, or 256 darkness levels. > > As the number of bits per pixel may be less than eight, some of these > formats rely on proper block handling for the calculation of bits per > pixel and pitch. > > Signed-off-by: Geert Uytterhoeven > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays
Hello Geert, On 3/9/22 13:56, Geert Uytterhoeven wrote: > Hi Javier, > > On Wed, Mar 9, 2022 at 1:14 PM Javier Martinez Canillas > wrote: >> On 3/8/22 17:30, Geert Uytterhoeven wrote: >>> Unfortunately a regression was introduced since your v3: printed >>> text is mirrored upside-down. I.e. "E" is rendered correctly, but "L" >>> turns into "Γ" (Greek Gamma). >>> I suspect something went wrong with the display initialization >>> sequence. >>> >> >> Could you please try Chen-Yu's fix for the COM scan direction mask ? >> >> https://lists.freedesktop.org/archives/dri-devel/2022-March/345915.html >> >> I made a mistake when converting to use the GENMASK() and FIELD_PREP() >> macros in v4 as suggested by Andy. The SSD130X_SET_COM_SCAN_DIR_MASK >> wasn't correct which would explain the output to be vertically flipped. > > Thanks, confirmed fixed. > Great, thanks a lot for testing and the confirmation! -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 1/2] drm: ssd130x: Fix COM scan direction register mask
On 3/9/22 13:56, Geert Uytterhoeven wrote:
> On Wed, Mar 9, 2022 at 2:57 AM Chen-Yu Tsai wrote:
>> From: Chen-Yu Tsai
>>
>> The SSD130x's command to toggle COM scan direction uses bit 3 and only
>> bit 3 to set the direction of the scanout. The driver has an incorrect
>> GENMASK(3, 2), causing the setting to be set on bit 2, rendering it
>> ineffective.
>>
>> Fix the mask to only bit 3, so that the requested setting is applied
>> correctly.
>>
>> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
>> Signed-off-by: Chen-Yu Tsai
>
> Thanks, this fixes the vertically-mirrored display on my Adafruit
> FeatherWing 128x32 OLED.
> Tested-by: Geert Uytterhoeven
> Reviewed-by: Geert Uytterhoeven
>
Thanks for the testing and review. I've pushed both patches to drm-misc-next.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays
Hello Geert,
On 3/9/22 21:04, Geert Uytterhoeven wrote:
[snip]
>> +
>> + /* Last page may be partial */
>> + if (8 * (i + 1) > ssd130x->height)
>> + m = ssd130x->height % 8;
>> + for (j = x; j < x + width; j++) {
>> + u8 data = 0;
>> +
>> + for (k = 0; k < m; k++) {
>> + u8 byte = buf[(8 * i + k) * line_length + j
>> / 8];
>
> As buf does not point to (0, 0), the above is not correct if rect.x1 !=
> 0 or rect.y1 != 0. After fixing that, writing more than one text line
> to the console works, but I still see an issue with updates where the
> rectangle size and/or position are not aligned to 8 pixels horizontally.
> Will do more investigation, and send fixes...
>
Right, I believe this is a consequence of introducing shadow buffers at
some point and not adjusting the logic in this function.
Thanks a lot for tracking down the issues and working on fixes for them!
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()
Hello Geert, On 3/14/22 14:40, Geert Uytterhoeven wrote: > Hi Javier, > > On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas > wrote: >> Add support to convert 8-bit grayscale to reversed monochrome for drivers >> that control monochromatic displays, that only have 1 bit per pixel depth. >> >> This helper function was based on repaper_gray8_to_mono_reversed() from >> the drivers/gpu/drm/tiny/repaper.c driver. >> >> Signed-off-by: Javier Martinez Canillas > >> --- a/drivers/gpu/drm/drm_format_helper.c >> +++ b/drivers/gpu/drm/drm_format_helper.c >> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int >> dst_pitch, uint32_t dst_for >> return -EINVAL; >> } >> EXPORT_SYMBOL(drm_fb_blit_toio); >> + >> +/** >> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome >> + * @dst: reversed monochrome destination buffer > > What's the meaning of "reversed"? Originally I took this helper from the repaper driver and it was called repaper_gray8_to_mono_reversed(), so the naming just got carried over. > During the last few days, I've been balancing between (a) "reverse > video" and (b) "reverse bit order", but none of them seems to be true. > > (a) The code maps 0-127 to 0 and 8-255 to 1, which just reduces from > 256 to 2 grayscale levels, without inversion. The result is also > white-on-black on my ssd130x OLED. > (b) On little-endian, the CFB drawops use little-endian bit order, > which is what ends up in "byte" in the code below. > As far as I understand is (a) from the options since the repaper display uses black-on-white. But as you pointed out the ssd130x OLEDs instead do white-on-black. I should had probably just name the helper drm_fb_gray8_to_monochrome() or maybe drm_fb_gray8_to_gray1() ? -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH -next] drm/solomon: Make DRM_SSD130X depends on MMU
Hello YueHaibing,
Thanks for the patch.
On 3/12/22 07:34, YueHaibing wrote:
> WARNING: unmet direct dependencies detected for DRM_GEM_SHMEM_HELPER
> Depends on [n]: HAS_IOMEM [=y] && DRM [=m] && MMU [=n]
> Selected by [m]:
> - DRM_SSD130X [=m] && HAS_IOMEM [=y] && DRM [=m]
>
> DRM_GEM_SHMEM_HELPER depends on MMU, DRM_SSD130X should also depends on MMU.
>
> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: YueHaibing
> ---
Indeed. All the DRM drivers that select DRM_GEM_SHMEM_HELPER depend on MMU.
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 1/5] drm/format-helper: Rename drm_fb_xrgb8888_to_mono_reversed()
Hello Geert,
Thanks for your patch.
On 3/15/22 12:07, Geert Uytterhoeven wrote:
> There is no "reversed" handling in drm_fb_xrgb_to_mono_reversed():
> the function just converts from color to grayscale, and reduces the
> number of grayscale levels from 256 to 2 (i.e. brightness 0-127 is
> mapped to 0, 128-255 to 1). All "reversed" handling is done in the
> repaper driver, where this function originated.
>
> Hence make this clear by renaming drm_fb_xrgb_to_mono_reversed() to
> drm_fb_xrgb_to_mono(), and documenting the black/white pixel
> mapping.
>
> Fixes: bcf8b616deb87941 ("drm/format-helper: Add
> drm_fb_xrgb_to_mono_reversed()")
> Signed-off-by: Geert Uytterhoeven
> ---
As you mentioned the function originally came from the repaper driver
(that uses white-on-black) and I wrongly assumed that the destination
buffer for the OLED panels were also monochrome reversed.
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/5] drm/format-helper: Fix XRGB888 to monochrome conversion
On 3/15/22 12:07, Geert Uytterhoeven wrote:
> The conversion functions drm_fb_xrgb_to_mono() and
> drm_fb_gray8_to_mono_line() do not behave correctly when the
> horizontal boundaries of the clip rectangle are not multiples of 8:
> a. When x1 % 8 != 0, the calculated pitch is not correct,
> b. When x2 % 8 != 0, the pixel data for the last byte is wrong.
>
Thanks a lot for tracking down and fixing these issues.
> Simplify the code and fix (a) by:
> 1. Removing start_offset, and always storing the first pixel in the
> first bit of the monochrome destination buffer.
> Drivers that require the first pixel in a byte to be located at an
> x-coordinate that is a multiple of 8 can always align the clip
> rectangle before calling drm_fb_xrgb_to_mono().
> Note that:
>- The ssd130x driver does not need the alignment, as the
>monochrome buffer is a temporary format,
>- The repaper driver always updates the full screen, so the clip
>rectangle is always aligned.
> 2. Passing the number of pixels to drm_fb_gray8_to_mono_line(),
> instead of the number of bytes, and the number of pixels in the
> last byte.
>
> Fix (b) by explicitly setting the target bit, instead of always setting
> bit 7 and shifting the value in each loop iteration.
>
> Remove the bogus pitch check, which operates on bytes instead of pixels,
> and triggers when e.g. flashing the cursor on a text console with a font
> that is 8 pixels wide.
>
> Drop the confusing comment about scanlines, as a pitch in bytes always
> contains a multiple of 8 pixels.
>
> While at it, use the drm_rect_height() helper instead of open-coding the
> same operation.
>
> Update the comments accordingly.
>
> Fixes: bcf8b616deb87941 ("drm/format-helper: Add
> drm_fb_xrgb_to_mono_reversed()")
> Signed-off-by: Geert Uytterhoeven
> ---
Acked-by: Javier Martinez Canillas
I just have a small comment below.
[snip]
> +static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int
> pixels)
> +{
> + while (pixels) {
> + unsigned int i, bits = min(pixels, 8U);
> + u8 byte = 0;
>
> - byte >>= 1;
> - if (src[x] >> 7)
> - byte |= BIT(7);
> + for (i = 0; i < bits; i++, pixels--) {
I think is worth to add a comment here explaining that the pixel is set to
1 for brightness > 127 and to 0 for brightness < 128. Or as kernel-doc for
this helper function.
> + if (*src++ & BIT(7))
Pekka also mentioned that if (*src++ > 127) would make this easier to read.
> + byte |= BIT(i);
> }
> *dst++ = byte;
> }
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 3/5] drm: ssd130x: Fix rectangle updates
On 3/15/22 12:07, Geert Uytterhoeven wrote:
> The rectangle update functions ssd130x_fb_blit_rect() and
> ssd130x_update_rect() do not behave correctly when x1 != 0 or y1 !=
> 0, or when y1 or y2 are not aligned to display page boundaries.
> E.g. when used as a text console, only the first line of text is shown
> on the display.
>
> 1. The buffer passed by ssd130x_fb_blit_rect() points to the first
> byte of monochrome bitmap data, and thus has its origin at (x1,
> y1), while ssd130x_update_rect() assumes it is at (0, 0).
> Fix ssd130x_update_rect() by changing the vertical and horizontal
> loop ranges, and adding the offsets only when needed.
>
> 2. In ssd130x_fb_blit_rect(), align y1 and y2 to the display page
> boundaries before doing the color conversion, so the full page
> is converted and updated.
> Remove the correction for an unaligned y1 from
> ssd130x_update_rect(), and add a check to make sure y1 is aligned.
>
> Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Geert Uytterhoeven
> ---
Thanks for fixing this too.
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 4/5] drm: ssd130x: Reduce temporary buffer sizes
On 3/15/22 12:07, Geert Uytterhoeven wrote:
> ssd130x_clear_screen() allocates a temporary buffer sized to hold one
> byte per pixel, while it only needs to hold one bit per pixel.
>
> ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one
> byte per pixel for the whole frame buffer, while it only needs to hold
> one bit per pixel for the part that is to be updated.
> Pass dst_pitch to drm_fb_xrgb_to_mono_reversed(), as we have already
Just drm_fb_xrgb_to_mono() since you already fixed the name in patch 1/5.
> calculated it anyway.
>
> Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Geert Uytterhoeven
> ---
Indeed. I haven't noticed that got the calculation wrong until you pointed out.
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 5/5] drm/repaper: Reduce temporary buffer size in repaper_fb_dirty()
On 3/15/22 12:07, Geert Uytterhoeven wrote:
> As the temporary buffer is no longer used to store 8-bit grayscale data,
> its size can be reduced to the size needed to store the monochrome
> bitmap data.
>
> Fixes: 24c6bedefbe71de9 ("drm/repaper: Use format helper for xrgb to
> monochrome conversion")
> Signed-off-by: Geert Uytterhoeven
> ---
> Untested due to lack of hardware.
>
Patch looks good to me but I also don't have this hardware.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH -next] drm/solomon: Make DRM_SSD130X depends on MMU
On 3/15/22 10:18, Javier Martinez Canillas wrote:
> Hello YueHaibing,
>
> Thanks for the patch.
>
> On 3/12/22 07:34, YueHaibing wrote:
>> WARNING: unmet direct dependencies detected for DRM_GEM_SHMEM_HELPER
>> Depends on [n]: HAS_IOMEM [=y] && DRM [=m] && MMU [=n]
>> Selected by [m]:
>> - DRM_SSD130X [=m] && HAS_IOMEM [=y] && DRM [=m]
>>
>> DRM_GEM_SHMEM_HELPER depends on MMU, DRM_SSD130X should also depends on MMU.
>>
>> Fixes: a61732e80867 ("drm: Add driver for Solomon SSD130x OLED displays")
>> Signed-off-by: YueHaibing
>> ---
>
> Indeed. All the DRM drivers that select DRM_GEM_SHMEM_HELPER depend on MMU.
>
> Acked-by: Javier Martinez Canillas
>
Pushed to drm-misc (drm-misc-next). Thanks!
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 1/2] fbdev: Fix sys_imageblit() for arbitrary image widths
Hello Thomas,
On 3/13/22 20:29, Thomas Zimmermann wrote:
> Commit 6f29e04938bf ("fbdev: Improve performance of sys_imageblit()")
> broke sys_imageblit() for image width that are not aligned to 8-bit
> boundaries. Fix this by handling the trailing pixels on each line
> separately. The performance improvements in the original commit do not
> regress by this change.
>
> Signed-off-by: Thomas Zimmermann
> Fixes: 6f29e04938bf ("fbdev: Improve performance of sys_imageblit()")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Sam Ravnborg
> ---
Looks good to me. Also Marek and Geert mentioned that fixes the issue
they were seeing.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/2] fbdev: Fix cfb_imageblit() for arbitrary image widths
On 3/13/22 20:29, Thomas Zimmermann wrote:
> Commit 0d03011894d2 ("fbdev: Improve performance of cfb_imageblit()")
> broke cfb_imageblit() for image widths that are not aligned to 8-bit
> boundaries. Fix this by handling the trailing pixels on each line
> separately. The performance improvements in the original commit do not
> regress by this change.
>
> Signed-off-by: Thomas Zimmermann
> Fixes: 0d03011894d2 ("fbdev: Improve performance of cfb_imageblit()")
> Reported-by: Marek Szyprowski
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Sam Ravnborg
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH] fbdev: defio: fix the pagelist corruption
Hello Chuansheng,
On 3/17/22 06:46, Chuansheng Liu wrote:
> Easily hit the below list corruption:
> ==
> list_add corruption. prev->next should be next (c0ceb090), but
> was ec604507edc8. (prev=ec604507edc8).
> WARNING: CPU: 65 PID: 3959 at lib/list_debug.c:26
> __list_add_valid+0x53/0x80
> CPU: 65 PID: 3959 Comm: fbdev Tainted: G U
> RIP: 0010:__list_add_valid+0x53/0x80
> Call Trace:
>
> fb_deferred_io_mkwrite+0xea/0x150
> do_page_mkwrite+0x57/0xc0
> do_wp_page+0x278/0x2f0
> __handle_mm_fault+0xdc2/0x1590
> handle_mm_fault+0xdd/0x2c0
> do_user_addr_fault+0x1d3/0x650
> exc_page_fault+0x77/0x180
> ? asm_exc_page_fault+0x8/0x30
> asm_exc_page_fault+0x1e/0x30
> RIP: 0033:0x7fd98fc8fad1
> ==
>
> Figure out the race happens when one process is adding &page->lru into
> the pagelist tail in fb_deferred_io_mkwrite(), another process is
> re-initializing the same &page->lru in fb_deferred_io_fault(), which is
> not protected by the lock.
>
> This fix is to init all the page lists one time during initialization,
> it not only fixes the list corruption, but also avoids INIT_LIST_HEAD()
> redundantly.
>
> Fixes: 105a940416fc ("fbdev/defio: Early-out if page is already
> enlisted")
> Cc: Thomas Zimmermann
> Signed-off-by: Chuansheng Liu
> ---
This makes sense to me. If you address Geert comment and post a v2,
feel free to add:
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 0/5] drm: Fix monochrome conversion for sdd130x
Hello Geert, On 3/17/22 09:18, Geert Uytterhoeven wrote: > Hi all, > > This patch series contains fixes and improvements for the XRGB888 to > monochrome conversion in the DRM core, and for its users. > > 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 a text console with 4x6, 7x14 and 8x8 fonts. > > Thanks! > > Geert Uytterhoeven (5): > drm/format-helper: Rename drm_fb_xrgb_to_mono_reversed() > drm/format-helper: Fix XRGB888 to monochrome conversion > drm/ssd130x: Fix rectangle updates > drm/ssd130x: Reduce temporary buffer sizes > drm/repaper: Reduce temporary buffer size in repaper_fb_dirty() > Thanks for re-spinning this series and again for fixing my bugs! I pushed patches 1-4 to drm-misc (drm-misc-next) but left patch 5 since would like to give Noralf the opportunity to review/test before pushing. By the way, you should probably request commit access to the drm-misc tree: https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/vmwgfx: Propagate error on failed ioctl
Hello Philipp, On 3/13/22 06:06, Philipp Sieweck wrote: > The cases of vmw_user_bo_synccpu_grab failing with -ERESTARTSYS or -EBUSY > are handled in vmw_user_bo_synccpu_ioctl by not printing an error message. > However, the error value gets discarded, indicating success. This leads > to rendering glitches and a reported drm error on the next ioctl call to > release the handle. > > This patch propagates the error value from vmw_user_synccpu_grab. > > Signed-off-by: Philipp Sieweck > --- > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 2 ++ > 1 file changed, 2 insertions(+) > Patch looks good to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 1/8] drm: Put related statements next to each other in Makefile
Hello Thomas, On 3/22/22 20:27, Thomas Zimmermann wrote: > Give the Makefile a bit more structure by putting rules for core, > helpers, drivers, etc next to each other. > > Signed-off-by: Thomas Zimmermann > --- This is a nice cleanup. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 2/8] drm: Rename dp/ to display/
On 3/22/22 20:27, Thomas Zimmermann wrote: > Rename dp/ to display/ to account for additional display-related > helpers, such as HDMI. Update all related include statements. No > functional changes. > > Various drivers, such as i915 and amdgpu, use similar naming scheme > by putting code for video-output standards into a local display/ > directory. The new directory's name is aligned with that policy. > It is really a policy or just a convention ? > Signed-off-by: Thomas Zimmermann > --- > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 3/8] drm/display: Introduce a DRM display-helper module
On 3/22/22 20:27, Thomas Zimmermann wrote: > Replace the DP-helper module with a display-helper module. Update > all related Kconfig and Makefile rules. > > Besides the existing code for DisplayPort, the new module will > contain helpers for other video-output standards, such as HDMI. > Drivers will still be able to select the required video-output > helpers. Linking all such code into a single module avoids the > proliferation of small kernel modules. > > Signed-off-by: Thomas Zimmermann > --- [snip] > +config DRM_DISPLAY_HELPER > + tristate > + depends on DRM > + help > + DRM helpers for display adapters. > + > config DRM_DP_HELPER > tristate > depends on DRM > + select DRM_DISPLAY_HELPER > help > DRM helpers for DisplayPort. > I was about to ask why this would still be needed but then re-read the commit message that says drivers will still be able to select required video-output helpers. That makes sense since the fact that all helpers will be in the same module would be transparent to drivers. > diff --git a/drivers/gpu/drm/display/Makefile > b/drivers/gpu/drm/display/Makefile > index 75faffc706b1..90f12e9b4735 100644 > --- a/drivers/gpu/drm/display/Makefile > +++ b/drivers/gpu/drm/display/Makefile > @@ -2,8 +2,9 @@ > > obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o > > -drm_dp_helper-y := drm_dp.o drm_dp_dual_mode_helper.o drm_dp_helper_mod.o > drm_dp_mst_topology.o > -drm_dp_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > -drm_dp_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o > +drm_display_helper-y := drm_display_helper_mod.o > +drm_display_helper-$(CONFIG_DRM_DP_HELPER) := drm_dp_helper.o > drm_dp_dual_mode_helper.o drm_dp_mst_topology.o > +drm_display_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > +drm_display_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o > > -obj-$(CONFIG_DRM_DP_HELPER) += drm_dp_helper.o > +obj-$(CONFIG_DRM_DISPLAY_HELPER) += drm_display_helper.o The drm_dp_helper.ko module has some parameters and this change will break existing kernel cmdline that are using it: $ modinfo drivers/gpu/drm/dp/drm_dp_helper.ko | grep parm | cut -d : -f2 drm_dp_cec_unregister_delay dp_aux_i2c_speed_khz dp_aux_i2c_transfer_size I don't know whether those are considered a kernel ABI or not though, and some already changed when the DP helpers were moved from drm_kms_helper.ko -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 1/2] fbdev: Improve performance of sys_fillrect()
Hello Thomas,
On 2/17/22 11:34, Thomas Zimmermann wrote:
> Improve the performance of sys_fillrect() by using word-aligned
> 32/64-bit mov instructions. While the code tried to implement this,
> the compiler failed to create fast instructions. The resulting
> binary instructions were even slower than cfb_fillrect(), which
> uses the same algorithm, but operates on I/O memory.
>
> A microbenchmark measures the average number of CPU cycles
> for sys_fillrect() after a stabilizing period of a few minutes
> (i7-4790, FullHD, simpledrm, kernel with debugging). The value
> for CFB is given as a reference.
>
> sys_fillrect(), new: 26586 cycles
> sys_fillrect(), old: 166603 cycles
> cfb_fillrect(): 41012 cycles
>
> In the optimized case, sys_fillrect() is now ~6x faster than before
> and ~1.5x faster than the CFB implementation.
>
Wow, that's a big speedup!
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/video/fbdev/core/sysfillrect.c | 16 +++-
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/sysfillrect.c
> b/drivers/video/fbdev/core/sysfillrect.c
> index 33ee3d34f9d2..bcdcaeae6538 100644
> --- a/drivers/video/fbdev/core/sysfillrect.c
> +++ b/drivers/video/fbdev/core/sysfillrect.c
> @@ -50,19 +50,9 @@ bitfill_aligned(struct fb_info *p, unsigned long *dst, int
> dst_idx,
>
> /* Main chunk */
> n /= bits;
> - while (n >= 8) {
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - n -= 8;
> - }
> - while (n--)
> - *dst++ = pat;
> + memset_l(dst, pat, n);
> + dst += n;
> +
Also the code is much more simpler / easy to read now. Amazing patch.
Reviewed-by: Javier Martinez Canillas
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/2] fbdev: Improve performance of sys_imageblit()
Hello Thomas, On 2/17/22 11:34, Thomas Zimmermann wrote: > Improve the performance of sys_imageblit() by manually unrolling > the inner blitting loop and moving some invariants out. The compiler > failed to do this automatically. The resulting binary code was even > slower than the cfb_imageblit() helper, which uses the same algorithm, > but operates on I/O memory. > > A microbenchmark measures the average number of CPU cycles > for sys_imageblit() after a stabilizing period of a few minutes > (i7-4790, FullHD, simpledrm, kernel with debugging). The value > for CFB is given as a reference. > > sys_imageblit(), new: 25934 cycles > sys_imageblit(), old: 35944 cycles > cfb_imageblit(): 30566 cycles > > In the optimized case, sys_imageblit() is now ~30% faster than before > and ~20% faster than cfb_imageblit(). > > Signed-off-by: Thomas Zimmermann > --- This patch looks good to me as well. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/simpledrm: Add "panel orientation" property on non-upright mounted LCD panels
Hello Hans, On 2/21/22 23:00, Hans de Goede wrote: > Some devices use e.g. a portrait panel in a standard laptop casing made > for landscape panels. efifb calls drm_get_panel_orientation_quirk() and > sets fb_info.fbcon_rotate_hint to make fbcon rotate the console so that > it shows up-right instead of on its side. > > When switching to simpledrm to fbcon renders on its side. Call the This sentence sounds a little off to me. Did you mean: "the fbcon renders on its side." ? Maybe you can say something like the following: When switching to simpledrm, fbcon attachs to the fbdev emulated by the DRM core instead. And the fb_info.fbcon_rotate_hint field is set by the emulation layer, if panel orientation was set for a connector. > drm_connector_set_panel_orientation_with_quirk() helper to add > a "panel orientation" property on devices listed in the quirk table, > to make the fbcon (and aware userspace apps) rotate the image to > display properly. > > Cc: Javier Martinez Canillas > Signed-off-by: Hans de Goede > --- The patch looks good to me. Thanks a lot for fixing this Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 1/2] drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on Asus TF103C
Hello Hans,
On 2/21/22 23:06, Hans de Goede wrote:
> Vtotal is wrong in the BIOS supplied modeline for the DSI panel on
> the Asus TF103C leading to the last line of the display being shown
> as the first line.
>
> The factory installed Android has a hardcoded modeline in its kernel,
> causing it to not suffer from this BIOS bug;
>
> and the Android boot-splash which uses the EFI FB which does have this bug
> has the last line all black causing the bug to not be visible.
>
> This commit introduces a generic DMI based mechanism for doing modeline
> fixups, in case we need similar fixups on other models in the future.
>
> Signed-off-by: Hans de Goede
> ---
> drivers/gpu/drm/i915/display/vlv_dsi.c | 36 ++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
> b/drivers/gpu/drm/i915/display/vlv_dsi.c
> index 06ef822c27bd..66f5cf32bb66 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -23,6 +23,7 @@
> * Author: Jani Nikula
> */
>
> +#include
> #include
>
> #include
> @@ -1831,6 +1832,33 @@ static void vlv_dphy_param_init(struct intel_dsi
> *intel_dsi)
> intel_dsi_log_params(intel_dsi);
> }
>
> +typedef void (*vlv_dsi_mode_fixup_func)(struct drm_connector *connector,
> + struct drm_display_mode *fixed_mode);
> +
> +/*
> + * Vtotal is wrong on the Asus TF103C leading to the last line of the display
> + * being shown as the first line. The factory installed Android has a
> hardcoded
> + * modeline, causing it to not suffer from this BIOS bug.
> + */
> +static void vlv_dsi_asus_tf103c_mode_fixup(struct drm_connector *connector,
> +struct drm_display_mode *fixed_mode)
> +{
> + fixed_mode->vtotal = 816;
> + fixed_mode->crtc_vtotal = 816;
> +}
> +
> +static const struct dmi_system_id dmi_mode_fixup_table[] = {
> + {
> + /* Asus Transformer Pad TF103C */
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "TF103C"),
> + },
> + .driver_data = (void *)vlv_dsi_asus_tf103c_mode_fixup,
> + },
> + { }
> +};
> +
There's nothing driver specific in this mechanism so I wonder if would
be better to add it as a DRM helper, for others drivers to use it too.
Maybe in drivers/gpu/drm/drm_modeset_helper.c or a drm_modeset_quirks.c
like we have for drivers/gpu/drm/drm_panel_orientation_quirks.c ?
The patch looks good to me, regardless where you decide to add it.
Reviewed-by: Javier Martinez Canillas
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 2/2] drm/i915/vlv_dsi: Add DMI quirk for wrong panel size on Lenovo Yoga Tablet 2 series
On 2/21/22 23:06, Hans de Goede wrote: > On the Lenovo Yoga Tablet 2 830 / 1050 the VBT contains a bogus > 192mm x 120mm size. This is especially a problem on the 8" 830 version > which uses a 10:16 portrait screen where as the bogus size is 16:10. > > Add a DMI quirk to override the wrong panel size with the correct one. > Note both the 10" 1050 models as well as the 8" 830 models use the same > mainboard and thus the same DMI strings. The 10" 1050 uses a 1920x1200 > landscape screen, where as the 8" 830 uses a 1200x1920 portrait screen, > so the quirk handling uses the display resolution to detect the model. > > Signed-off-by: Hans de Goede > --- Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
Hello Michal, On 2/18/22 17:04, Michal Suchanek wrote: > Since switch to simplefb/simpledrm VESA graphic modes are no longer > available with legacy BIOS. > Maybe you can mention that is the "vga=" kernel command line parameter since that may be more evident to people reading the commit message ? > The x86 realmode boot code enables the VESA graphic modes when option > FB_BOOT_VESA_SUPPORT is enabled. > > To enable use of VESA modes with simplefb in legacy BIOS boot mode drop I think you meant "VESA modes with the sysfb driver" ? or something like that since otherwise it seems that you meant to use it with the simplefb (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the "vga=" param as far as I understand (it just uses whatever was setup). The name sysfb_simplefb is really horrible, because it is too confusing and probably we should change it at some point... Patch itself looks good to me though. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 17:45, Michal Suchánek wrote: [snip] >>> >>> To enable use of VESA modes with simplefb in legacy BIOS boot mode drop >> >> I think you meant "VESA modes with the sysfb driver" ? or something like >> that since otherwise it seems that you meant to use it with the simplefb >> (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the >> "vga=" param as far as I understand (it just uses whatever was setup). > > And the vga= is whatever was set up by the realmode code. And the config > option for realmode code to do that is selected by vesafb and not > simplefb so it does not wotk for simplefb/simpledrm/whatewer when efifib > is not built into the kernel. > Yes, that's what I tried to say. But your commit message says "To enable use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't accurate AFAIU (unless you meant sysfb instead). Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 17:54, Javier Martinez Canillas wrote: > On 2/23/22 17:45, Michal Suchánek wrote: > > [snip] > >>>> >>>> To enable use of VESA modes with simplefb in legacy BIOS boot mode drop >>> >>> I think you meant "VESA modes with the sysfb driver" ? or something like >>> that since otherwise it seems that you meant to use it with the simplefb >>> (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the >>> "vga=" param as far as I understand (it just uses whatever was setup). >> >> And the vga= is whatever was set up by the realmode code. And the config >> option for realmode code to do that is selected by vesafb and not >> simplefb so it does not wotk for simplefb/simpledrm/whatewer when efifib >> is not built into the kernel. >> > > Yes, that's what I tried to say. But your commit message says "To enable > use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't > accurate AFAIU (unless you meant sysfb instead). In fact, probably the subject line should also be something like following: firmware: sysfb: Enable boot time VESA graphic mode selection Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 18:12, Michal Suchánek wrote: > On Wed, Feb 23, 2022 at 05:54:54PM +0100, Javier Martinez Canillas wrote: [snip] >> >> Yes, that's what I tried to say. But your commit message says "To enable >> use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't >> accurate AFAIU (unless you meant sysfb instead). > > config SYSFB_SIMPLEFB > bool "Mark VGA/VBE/EFI FB as generic system framebuffer" > depends on SYSFB > + select BOOT_VESA_SUPPORT if X86 > > This to me means that it's simplefb specifically that requires it, not sysfb. > More precisely SYSFB_SIMPLEFB which is the simplefb implementation on top of > legacy BIOS. > Ok, I see what you meant. The fact that simplefb is what's named to the part of the sysfb driver that register the "simple-framebuffer" platform device and also the name of the fbdev driver that matches the "simple-framebuffer" is too confusing. My point about the subject line remains thought, I would use something like: firmware: sysfb: Enable boot time VESA graphic mode selection for simplefb But I'll stop bike-shedding this. I don't mind if you keep the current line and feel free to keep my r-b tag. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 19:23, Michal Suchánek wrote: [snip] >> My point about the subject line remains thought, I would use something like: >> >> firmware: sysfb: Enable boot time VESA graphic mode selection for simplefb > > I see where the confusion comes from. > Yeah. And just to clarify, the "simplefb" in the subject line I proposed was about the sysfb simplefb and not the fbdev simplefb :) > The efifb (and probably vesafb) has implicit unstated dependency on > sysfb. So the drivers that select BOOT_VESA_SUPPORT should instead > depend on SYSFB, and then SYSFB can select BOOT_VESA_SUPPORT, and it > will look much saner. > That indeed would be much nicer. And I agree with you that there's an implicit dependency that should be made explicit since SYSFB is what registers the "efi-framebuffer" or "vesa-framebuffer" if SYSFB_SIMPLEFB is not enabled. Should SYSFB should only select BOOT_VESA_SUPPORT if x86 ? I know that in practice shouldn't matter because BOOT_VESA_SUPPORT is under x86 but I guess is more correct if that's the case. And I think that FB_SIMPLE should depend on SYSFB_SIMPLEFB if !OF (since a "simple-framebuffer" platform device could be registered by OF if a Device Tree node with compatible "simple-framebuffer" exists). Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] drm/repaper: Use format helper for xrgb8888 to monochrome conversion
There is now a drm_fb_xrgb_to_mono_reversed() helper function to do
format conversion from XRGB to reversed monochrome.
Use that helper and remove the open coded version in the repaper driver.
Signed-off-by: Javier Martinez Canillas
---
This was only built tested because I don't have access to the hardware.
drivers/gpu/drm/tiny/repaper.c | 24 +---
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index 97a775c48cea..5c74e236b16d 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -508,26 +508,6 @@ static void repaper_get_temperature(struct repaper_epd
*epd)
epd->factored_stage_time = epd->stage_time * factor10x / 10;
}
-static void repaper_gray8_to_mono_reversed(u8 *buf, u32 width, u32 height)
-{
- u8 *gray8 = buf, *mono = buf;
- int y, xb, i;
-
- for (y = 0; y < height; y++)
- for (xb = 0; xb < width / 8; xb++) {
- u8 byte = 0x00;
-
- for (i = 0; i < 8; i++) {
- int x = xb * 8 + i;
-
- byte >>= 1;
- if (gray8[y * width + x] >> 7)
- byte |= BIT(7);
- }
- *mono++ = byte;
- }
-}
-
static int repaper_fb_dirty(struct drm_framebuffer *fb)
{
struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
@@ -560,12 +540,10 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb)
if (ret)
goto out_free;
- drm_fb_xrgb_to_gray8(buf, 0, cma_obj->vaddr, fb, &clip);
+ drm_fb_xrgb_to_mono_reversed(buf, 0, cma_obj->vaddr, fb, &clip);
drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
- repaper_gray8_to_mono_reversed(buf, fb->width, fb->height);
-
if (epd->partial) {
repaper_frame_data_repeat(epd, buf, epd->current_frame,
REPAPER_NORMAL);
--
2.34.1
Re: [PATCH v3 3/5] fbdev: Remove trailing whitespaces from cfbimgblt.c
On 2/23/22 20:38, Thomas Zimmermann wrote: > Fix coding style. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 4/5] fbdev: Improve performance of cfb_imageblit()
On 2/23/22 20:38, Thomas Zimmermann wrote: > Improve the performance of cfb_imageblit() by manually unrolling > the inner blitting loop and moving some invariants out. The compiler > failed to do this automatically. This change keeps cfb_imageblit() > in sync with sys_imagebit(). > > A microbenchmark measures the average number of CPU cycles > for cfb_imageblit() after a stabilizing period of a few minutes > (i7-4790, FullHD, simpledrm, kernel with debugging). > > cfb_imageblit(), new: 15724 cycles > cfb_imageblit(): old: 30566 cycles > > In the optimized case, cfb_imageblit() is now ~2x faster than before. > > v3: > * fix commit description (Pekka) > > Signed-off-by: Thomas Zimmermann > --- Makes sense, improves perf and makes the two more consistent as you mention. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 5/5] drm: Add TODO item for optimizing format helpers
On 2/23/22 20:38, Thomas Zimmermann wrote: > Add a TODO item for optimizing blitting and format-conversion helpers > in DRM and fbdev. There's always demand for faster graphics output. > > Signed-off-by: Thomas Zimmermann > --- After fixing the typos mentioned by Sam: Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3 4/5] fbdev: Improve performance of cfb_imageblit()
Hello Sam,
On 2/23/22 21:25, Sam Ravnborg wrote:
[snip]
>
> Question: What is cfb an abbreviation for anyway?
> Not related to the patch - but if I have known the memory is lost..
>
I was curious so I dug on this. It seems CFB stands for Color Frame Buffer.
Doing a `git grep "(CFB)"` in the linux history repo [0], I get this:
Documentation/isdn/README.diversion: (CFB).
drivers/video/pmag-ba-fb.c: * PMAG-BA TURBOchannel Color Frame Buffer (CFB)
card support,
include/video/pmag-ba-fb.h: * TURBOchannel PMAG-BA Color Frame Buffer (CFB)
card support,
Probably the helpers are called like this because they were for any fbdev
driver but assumed that the framebuffer was always in I/O memory. Later some
drivers were allocating the framebuffer in system memory and still using the
helpers, that were using I/O memory accessors and it's ilegal on some arches.
So the sys_* variants where introduced by commit 68648ed1f58d ("fbdev: add
drawing functions for framebuffers in system RAM") to fix this. The old
ones just kept their name, but probably it should had been renamed to io_*
for the naming to be consistent with the sys_* functions.
[0]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH] drm/todo: Update panic handling todo
Hello Daniel, On 2/24/22 13:59, Daniel Vetter wrote: > Some things changed, and add two useful links. > > Cc: Javier Martinez Canillas > Cc: Pekka Paalanen > Cc: gpicc...@igalia.com > Signed-off-by: Daniel Vetter > --- > Documentation/gpu/todo.rst | 21 + > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 7bf7f2111696..283d35a500bd 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -475,8 +475,12 @@ This is a really varied tasks with lots of little bits > and pieces: >achieved by using an IPI to the local processor. > > * There's a massive confusion of different panic handlers. DRM fbdev > emulation > - helpers have one, but on top of that the fbcon code itself also has one. We > - need to make sure that they stop fighting over each another. > + helpers had their own (long removed), but on top of that the fbcon code > itself > + also has one. We need to make sure that they stop fighting over each > another. The "over each another" sounds a little bit off, shouldn't be "over each other" ? > + This is worked around by checking ``oops_in_progress`` at various entry > points > + into the DRM fbdev emulation helpers. A much cleaner approach here would > be to > + switch fbcon to the `threaded printk support > + <https://lwn.net/Articles/800946/>`_. > > * ``drm_can_sleep()`` is a mess. It hides real bugs in normal operations and >isn't a full solution for panic paths. We need to make sure that it only > @@ -488,16 +492,9 @@ This is a really varied tasks with lots of little bits > and pieces: >even spinlocks (because NMI and hardirq can panic too). We need to either >make sure to not call such paths, or trylock everything. Really tricky. > > -* For the above locking troubles reasons it's pretty much impossible to > - attempt a synchronous modeset from panic handlers. The only thing we could > - try to achive is an atomic ``set_base`` of the primary plane, and hope that > - it shows up. Everything else probably needs to be delayed to some worker or > - something else which happens later on. Otherwise it just kills the box > - harder, prevent the panic from going out on e.g. netconsole. > - > -* There's also proposal for a simplied DRM console instead of the full-blown > - fbcon and DRM fbdev emulation. Any kind of panic handling tricks should > - obviously work for both console, in case we ever get kmslog merged. > +* A clean solution would be an entirely separate panic output support in KMS, > + bypassing the current fbcon support. See `[PATCH v2 0/3] drm: Add panic > handling > + > <https://lore.kernel.org/dri-devel/20190311174218.51899-1-nor...@tronnes.org/>`_. > Having these two links in the docs is very useful. Thanks a lot for adding that. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/repaper: Use format helper for xrgb8888 to monochrome conversion
Hello Noralf, On 2/24/22 15:04, Noralf Trønnes wrote: > > > Den 23.02.2022 20.37, skrev Javier Martinez Canillas: >> There is now a drm_fb_xrgb_to_mono_reversed() helper function to do >> format conversion from XRGB to reversed monochrome. >> >> Use that helper and remove the open coded version in the repaper driver. >> >> Signed-off-by: Javier Martinez Canillas >> --- > > Tested-by: Noralf Trønnes > Reviewed-by: Noralf Trønnes > Thanks a lot for testing and for your review. > Do you have commit rights and will apply this patch? > Yes, I do. Can apply this later today. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 3/5] drm/i915/dsi: Add some debug logging to mipi_exec_i2c
Hello Hans, On 2/25/22 22:49, Hans de Goede wrote: > Add some debug logging to mipi_exec_i2c, to make debugging various > issues seen with it easier. > > Signed-off-by: Hans de Goede > --- > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 4 > 1 file changed, 4 insertions(+) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v4 1/3] sysfb: Make config option dependencies explicit
Hello Michal, On 2/25/22 21:51, Michal Suchanek wrote: > efifb and vesafb requires sysfb implicitly but this is not stated in > Kconfig. Add the dependency. > > With that all drivers that require sysfb depend on it so it can default > to disabled. > > Signed-off-by: Michal Suchanek > --- Thanks for the patch. This makes much more sense to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v4 2/3] sysfb: Enable boot time VESA graphic mode selection
On 2/25/22 21:51, Michal Suchanek wrote:
> Since switch to simplefb/simpledrm VESA graphic mode selection with vga=
> kernel parameter is no longer available with legacy BIOS.
>
> The x86 realmode boot code enables the VESA graphic modes when option
> FB_BOOT_VESA_SUPPORT is enabled.
>
> This option is selected by vesafb but not simplefb/simpledrm.
>
> To enable use of VESA modes with simplefb in legacy BIOS boot mode drop
> dependency of BOOT_VESA_SUPPORT on FB, also drop the FB_ prefix. Select
> the option from sysfb rather than the drivers that depend on it.
>
> The BOOT_VESA_SUPPORT is not specific to framebuffer but rather to x86
> platform, move it from fbdev to x86 Kconfig.
>
> Fixes: e3263ab389a7 ("x86: provide platform-devices for boot-framebuffers")
> Signed-off-by: Michal Suchanek
> Acked-by: Borislav Petkov
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v4 3/3] efifb: Remove redundant efifb_setup_from_dmi stub
On 2/25/22 21:51, Michal Suchanek wrote: > efifb is the only user of efifb_setup_from_dmi which is provided by > sysfb which is selected by efifb. That makes the stub redundant. > > Signed-off-by: Michal Suchanek > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH][next] drm: ssd130x: remove redundant initialization of pointer mode
Hello Colin, Thanks for the patch. On Wed, Mar 2, 2022 at 6:53 PM Colin Ian King wrote: > > Pointer mode is being assigned a value that is never read, it is > being re-assigned later with a new value. The initialization is > redundant and can be removed. > Indeed. Acked-by: Javier Martinez Canillas Best regards, Javier
Re: [PATCH] simpldrm: Enable boot time VESA graphic mode selection.
Hello, On 3/2/22 20:38, Michal Suchánek wrote: > Hello, > > On Wed, Mar 02, 2022 at 08:31:25PM +0100, Thomas Zimmermann wrote: >> Hi, >> >> is this ready to be merged? > > The objections raised so far have been addressed in v4. > > I think this is good to merge. > The v4 patches looks good to me and have provided my Reviewed-by to all of them. > Thanks > > Michal > >> -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH][next] drm: ssd130x: remove redundant initialization of pointer mode
On 3/2/22 19:29, Javier Martinez Canillas wrote: > Hello Colin, > > Thanks for the patch. > > On Wed, Mar 2, 2022 at 6:53 PM Colin Ian King wrote: >> >> Pointer mode is being assigned a value that is never read, it is >> being re-assigned later with a new value. The initialization is >> redundant and can be removed. >> > > Indeed. > > Acked-by: Javier Martinez Canillas > Pushed to drm-misc-next (with fixed drm/ssd130x subject prefix). Thanks! -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] simpldrm: Enable boot time VESA graphic mode selection.
Hello Thomas, On 3/4/22 21:00, Thomas Zimmermann wrote: > Hi, > > I've merged the patches into drm-misc-fixes. Thanks a lot to both of you. > Ard already picked these through the efi tree: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/ > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] simpldrm: Enable boot time VESA graphic mode selection.
On 3/4/22 21:47, Javier Martinez Canillas wrote: > Hello Thomas, > > On 3/4/22 21:00, Thomas Zimmermann wrote: >> Hi, >> >> I've merged the patches into drm-misc-fixes. Thanks a lot to both of you. >> > > Ard already picked these through the efi tree: > > https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/ > I talked with Ard and he kindly dropped these patches from the efi tree. So everything is fine. >> Best regards >> Thomas >> -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 0/9] drm: Support GEM SHMEM fbdev without shadow FB
Hello Thomas, On 3/3/22 21:58, Thomas Zimmermann wrote: > Rework the fbdev deferred-I/O to not interfere with fields of struct > page. Make the code more flexible and implement GEM SHMEM mmap on top > of it. > > This patchset removes the need for a special shadow framebuffer for > fbdev mmap when using GEM SHMEM. SHMEM pages are now mmap'ed from > /dev/fb directly. > Interesting. I wonder if you have any performance improvements after dropping the shadow buffer. > Patches 2 and 3 rework the fbdev deferred I/O code. It now allows > drivers to have better control of the mmap operations. All references > to fields in struct page are gone. The rsp state is help in a > separate pageref structure. > That's a very nice cleanup. This really was a huge layering violation. > Patches 4 to 7 provide callbacks an helpers to implement deferred I/O > with DRM drivers. Specifically, patch 6 introduces a callback to create > a dumb buffer for fbdev. This will be useful for many drivers that > currently cannot use generic fbdev emulation because of special placement > needs of the BO, such as amdgpu or radeon. The drivers can handle the > differences to regular dumb buffers in their new callback implementation. > > Patch 8 extends the GEM SHMEM memory manager with a new helper for fbdev > dumb-buffer creation. The returned BO has it's mmap set up to implement > deferred I/O with SHMEM pages. No additional shadow buffer is requires > any longer. Many drivers can immediatelly benefit from this change. > > Patch 9 extends virtgpu to support fbdev dumb buffers. It's build on > top of GEM SHMEM, but has some modifications that need to be implemented > for fbdev as well. > > There's no immediate fbdev performance improvement from this patchset. > Most of all, it removes unnecessary shadow framebuffers and rsp memcpys. > A shadow fb for a FullHD display is ~8 MiB, which we now save. The patches > do reduce latency between drawing to the fbdev buffer to displaying > on the screen. Watching a video on the fbdev console felt smoother and > had less flickering. > Awesome. And you also answered here the question I had above. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 1/9] drm/simpledrm: Use fbdev defaults for shadow buffering
On 3/3/22 21:58, Thomas Zimmermann wrote: > Don't select shadow buffering for the fbdev console explicitly. The > fbdev emulation's heuristic will enable it for any framebuffer with > .dirty callback. > Indeed it does. Not related to your series but looking at this patch I noticed that drivers/gpu/drm/tiny/bochs.c will be the only driver that sets .prefer_shadow_fbdev after this lands. The driver is using GEM so I wonder if after your series this DRM driver could have a .dirty callback and the field just be dropped? Or there would still be a case where it is needed ? Anyway, just wanted to mention in case I forget later. Your patch looks good to me and I think it could be pushed without waiting for the other patches in the series. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 1/9] drm/simpledrm: Use fbdev defaults for shadow buffering
On 3/8/22 10:56, Thomas Zimmermann wrote: > Hi > > Am 08.03.22 um 10:31 schrieb Javier Martinez Canillas: >> On 3/3/22 21:58, Thomas Zimmermann wrote: >>> Don't select shadow buffering for the fbdev console explicitly. The >>> fbdev emulation's heuristic will enable it for any framebuffer with >>> .dirty callback. >>> >> >> Indeed it does. Not related to your series but looking at this >> patch I noticed that drivers/gpu/drm/tiny/bochs.c will be the >> only driver that sets .prefer_shadow_fbdev after this lands. >> >> The driver is using GEM so I wonder if after your series this >> DRM driver could have a .dirty callback and the field just be >> dropped? Or there would still be a case where it is needed ? > Bochs uses VRAM helpers (i.e., TTM). Fbdev and userspace would directly > write into that buffer memory without a copy. So the dirty function > would be empty. > > Other drivers with VRAM helpers (e.g., hibmc, ast) operate on uncached > I/O memory AFAICT. So they set .prefer_shadow, which also affects > userspace. Bochs uses cached memory and shouldn't need prefer_shadow. > Setting prefer_shadow_fbdev is only there for making the fbdev buffer > object evictable from video memory. > > As it stands, using prefer_shadow_fbdev is the cleanest solution, even > if bochs is the only user of that field. > > Alternatively, we could make it a requirement that qemu provides enough > video memory for bochs to unconditionally pin the fbdev BO there without > ever evicting. I guess, that would mean 32 MiB of VRAM at least. > I see. Thanks a lot for this explanation. > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [RFC PATCH 0/4] Allow to use DRM fbdev emulation layer with CONFIG_FB disabled
Hello Daniel and Thomas, On 8/27/21 10:20 PM, Daniel Vetter wrote: > On Fri, Aug 27, 2021 at 07:50:23PM +0200, Thomas Zimmermann wrote: >> Hi >> >> Am 27.08.21 um 12:00 schrieb Javier Martinez Canillas: >>> This patch series splits the fbdev core support in two different Kconfig >>> symbols: FB and FB_CORE. The motivation for this is to allow CONFIG_FB to >>> be disabled, while still using fbcon with the DRM fbdev emulation layer. >> >> I'm skeptical. DRM's fbdev emulation is not just the console emulation, it's >> a full fbdev device. You can see the related device file as /dev/fb*. >> Providing the file while having CONFIG_FB disabled doesn't make much sense >> to me. I know it's not pretty, but it's consistent at least. >> >> If you want to remove fbdev, you could try to untangle fbdev and the console >> emulation such that DRM can set up a console by itself. Old fbdev drives >> would also set up the console individually. > > Yeah given the horrendous security track record of all that code, and the > maze of handover we have (stuff like flicker free boot and all that) I'm > wondering whether typing a new drmcon wouldn't be faster and a lot more > maintainable. > We talked about a drmcon with Peter Robinson as well but then decided that a way to disable CONFIG_FB but still having the DRM fbdev emulation could be a intermediary step, hence these RFC patches. But yes, I agree that a drmcon would be the proper approach for this, to not need any fbdev support at all. We will just keep the explicit disable for the fbdev drivers then in the meantime. Thanks a lot for your feedback. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [RFC PATCH 0/4] Allow to use DRM fbdev emulation layer with CONFIG_FB disabled
On 8/31/21 2:35 PM, Daniel Vetter wrote: > On Sat, Aug 28, 2021 at 12:02:21AM +0200, Javier Martinez Canillas wrote: [snip] >> >> We talked about a drmcon with Peter Robinson as well but then decided that a >> way to disable CONFIG_FB but still having the DRM fbdev emulation could be a >> intermediary step, hence these RFC patches. >> >> But yes, I agree that a drmcon would be the proper approach for this, to not >> need any fbdev support at all. We will just keep the explicit disable for the >> fbdev drivers then in the meantime. > > I think the only intermediate step would be to disable the fbdev uapi > (char node and anything in sysfs), while still registering against the > fbcon layer so you have a console. > Right, $subject disabled the sysfs interface but left the fbdev chardev. I can try to do a v2 that also disables that interface but just keep the fbcon part. > But looking at the things syzbot finds the really problematic code is all > in the fbcon and console layer in general, and /dev/fb0 seems pretty > solid. > Yes, but still would be an improvement in the sense that no legacy fbdev uAPI will be exposed and so user-space would only depend on the DRM/KMS interface. > I think for a substantial improvement here in robustness what you really > want is > - kmscon in userspace > - disable FB layer > - ideally also disable console/vt layer in the kernel Earlier in the thread it was mentioned that an in-kernel drmcon could be used instead. My worry with kmscon is that moving something as critical as console output to user-space might make harder to troubleshoot early booting issues. And also that will require user-space changes. An in-kernel drmcon could be a drop-in replacement though. > - have a minimal emergency/boot-up log thing in drm, patches for that > floated around a few times > Interesting. Do you have any pointers for this? My search-fu failed me when trying to find these patches. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] Revert "drm/fb-helper: improve DRM fbdev emulation device names"
This reverts commit b3484d2b03e4c940a9598aa841a52d69729c582a. That change attempted to improve the DRM drivers fbdev emulation device names to avoid having confusing names like "simpledrmdrmfb" in /proc/fb. But unfortunately there are user-space programs, such as pm-utils that query that information and so broke after the mentioned commit. Since the names in /proc/fb are used programs that consider it an ABI, let's restore the old names even when this lead to silly naming like the one mentioned above as an example. Reported-by: Johannes Stezenbach Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/drm_fb_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 3ab07832104..8993b02e783 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1737,7 +1737,7 @@ void drm_fb_helper_fill_info(struct fb_info *info, sizes->fb_width, sizes->fb_height); info->par = fb_helper; - snprintf(info->fix.id, sizeof(info->fix.id), "%s", + snprintf(info->fix.id, sizeof(info->fix.id), "%sdrmfb", fb_helper->dev->driver->name); } -- 2.31.1
Re: [PATCH] Revert "drm/fb-helper: improve DRM fbdev emulation device names"
Hello Daniel, On 10/13/21 14:27, Daniel Vetter wrote: >>> >>> info->par = fb_helper; >>> - snprintf(info->fix.id, sizeof(info->fix.id), "%s", > > Please add a comment here that drmfb is uapi because pm-utils matches > against it ... > Sure, I'll do that and send a v2. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v2] Revert "drm/fb-helper: improve DRM fbdev emulation device names"
This reverts commit b3484d2b03e4c940a9598aa841a52d69729c582a.
That change attempted to improve the DRM drivers fbdev emulation device
names to avoid having confusing names like "simpledrmdrmfb" in /proc/fb.
But unfortunately, there are user-space programs such as pm-utils that
match against the fbdev names and so broke after the mentioned commit.
Since the names in /proc/fb are used by tools that consider it an uAPI,
let's restore the old names even when this lead to silly names like the
one mentioned above.
Fixes: b3484d2b03e4 ("drm/fb-helper: improve DRM fbdev emulation device names")
Reported-by: Johannes Stezenbach
Signed-off-by: Javier Martinez Canillas
Reviewed-by: Ville Syrjälä
---
Changes in v2:
- Add a comment explaining that the current /proc/fb names are an uAPI.
- Add a Fixes: tag so it can be cherry-picked by stable kernels.
- Add Ville Syrjälä's Reviewed-by tag.
drivers/gpu/drm/drm_fb_helper.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 8e7a124d6c5a..22bf690910b2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1743,7 +1743,13 @@ void drm_fb_helper_fill_info(struct fb_info *info,
sizes->fb_width, sizes->fb_height);
info->par = fb_helper;
- snprintf(info->fix.id, sizeof(info->fix.id), "%s",
+ /*
+* The DRM drivers fbdev emulation device name can be confusing if the
+* driver name also has a "drm" suffix on it. Leading to names such as
+* "simpledrmdrmfb" in /proc/fb. Unfortunately, it's an uAPI and can't
+* be changed due user-space tools (e.g: pm-utils) matching against it.
+*/
+ snprintf(info->fix.id, sizeof(info->fix.id), "%sdrmfb",
fb_helper->dev->driver->name);
}
--
2.31.1
[RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal
The simpledrm driver allows to use the frame buffer that was set-up by the
firmware. This gives early video output before the platform DRM driver is
probed and takes over.
But it would be useful to have a way to disable this take over by the real
DRM drivers. For example, there may be bugs in the DRM drivers that could
cause the display output to not work correctly.
For those cases, it would be good to keep the simpledrm driver instead and
at least get a working display as set-up by the firmware.
Let's add a drm.remove_fb boolean kernel command line parameter, that when
set to false will prevent the conflicting framebuffers to being removed.
Since the drivers call drm_aperture_remove_conflicting_framebuffers() very
early in their probe callback, this will cause the drivers' probe to fail.
Thanks to Neal Gompa for the suggestion and Thomas Zimmermann for the idea
on how this could be implemented.
Suggested-by: Neal Gompa
Signed-off-by: Javier Martinez Canillas
---
Hello,
I'm sending this as an RFC because I wasn't sure about the correct name for
this module parameter, and also if 'remove_fb=0' is intitutive or instead a
parameter that's enabled is preferred (i.e: 'disable_fb_removal=1').
Best regards,
Javier
drivers/gpu/drm/drm_aperture.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
index 74bd4a76b253..0b454c8f7465 100644
--- a/drivers/gpu/drm/drm_aperture.c
+++ b/drivers/gpu/drm/drm_aperture.c
@@ -14,6 +14,11 @@
#include
#include
+static bool drm_aperture_remove_fb = true;
+module_param_named(remove_fb, drm_aperture_remove_fb, bool, 0600);
+MODULE_PARM_DESC(remove_fb,
+"Allow conflicting framebuffers removal [default=true]");
+
/**
* DOC: overview
*
@@ -283,6 +288,9 @@ static void drm_aperture_detach_drivers(resource_size_t
base, resource_size_t si
* This function removes graphics device drivers which use memory range
described by
* @base and @size.
*
+ * The conflicting framebuffers removal can be disabled by setting the
drm.remove_fb=0 kernel
+ * command line option. When this is disabled, the function will return an
-EINVAL errno code.
+ *
* Returns:
* 0 on success, or a negative errno code otherwise
*/
@@ -292,7 +300,12 @@ int
drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_
#if IS_REACHABLE(CONFIG_FB)
struct apertures_struct *a;
int ret;
+#endif
+
+ if (!drm_aperture_remove_fb)
+ return -EINVAL;
+#if IS_REACHABLE(CONFIG_FB)
a = alloc_apertures(1);
if (!a)
return -ENOMEM;
@@ -322,6 +335,9 @@ EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers);
* for any of @pdev's memory bars. The function assumes that PCI device with
* shadowed ROM drives a primary display and so kicks out vga16fb.
*
+ * The conflicting framebuffers removal can be disabled by setting the
drm.remove_fb=0 kernel
+ * command line option. When this is disabled, the function will return an
-EINVAL errno code.
+ *
* Returns:
* 0 on success, or a negative errno code otherwise
*/
@@ -331,6 +347,9 @@ int drm_aperture_remove_conflicting_pci_framebuffers(struct
pci_dev *pdev,
resource_size_t base, size;
int bar, ret = 0;
+ if (!drm_aperture_remove_fb)
+ return -EINVAL;
+
for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
continue;
--
2.31.1
Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal
Hello Neal, Thanks for your feedback. On 10/22/21 16:56, Neal Gompa wrote: > On Fri, Oct 22, 2021 at 10:40 AM Javier Martinez Canillas > wrote: >> >> The simpledrm driver allows to use the frame buffer that was set-up by the >> firmware. This gives early video output before the platform DRM driver is >> probed and takes over. >> >> But it would be useful to have a way to disable this take over by the real >> DRM drivers. For example, there may be bugs in the DRM drivers that could >> cause the display output to not work correctly. >> >> For those cases, it would be good to keep the simpledrm driver instead and >> at least get a working display as set-up by the firmware. >> >> Let's add a drm.remove_fb boolean kernel command line parameter, that when >> set to false will prevent the conflicting framebuffers to being removed. >> >> Since the drivers call drm_aperture_remove_conflicting_framebuffers() very >> early in their probe callback, this will cause the drivers' probe to fail. >> >> Thanks to Neal Gompa for the suggestion and Thomas Zimmermann for the idea >> on how this could be implemented. >> >> Suggested-by: Neal Gompa >> Signed-off-by: Javier Martinez Canillas >> --- >> Hello, >> >> I'm sending this as an RFC because I wasn't sure about the correct name for >> this module parameter, and also if 'remove_fb=0' is intitutive or instead a >> parameter that's enabled is preferred (i.e: 'disable_fb_removal=1'). >> > > In general, I think the patch is fine, but it might make sense to name > the parameter after the *effect* rather than the *action*. That is, > the effect of this option is that we don't probe and hand over to a > more appropriate hardware DRM driver. > > Since the effect (in DRM terms) is that we don't use platform DRM > modules, perhaps we could name the option one of these: > > * drm.noplatformdrv > * drm.simpledrv > * drm.force_simple > or maybe drm.disable_handover ? Naming is hard... > I'm inclined to say we should use the drm.* namespace for the cmdline > option because that makes it clear what subsystem it affects. The > legacy "nomodeset" option kind of sucked because it didn't really tell > you what that meant, and I'd rather not repeat that mistake. > Yes, agreed. In fact, that is the case for this patch since the param is in the drm module. I just forgot to mention the namespace in the last paragraph of the comment. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
