Re: [PATCH] drm: have config DRM_WERROR depend on !WERROR
Jani Nikula writes:
Hello Jani,
> If WERROR is already enabled, there's no point in enabling DRM_WERROR or
> asking users about it.
>
> Reported-by: Linus Torvalds
> Closes:
> https://lore.kernel.org/r/CAHk-=whxT8D_0j=bjtrvj-O=veojn6gw8gk4j2v+biduntz...@mail.gmail.com
> Fixes: f89632a9e5fa ("drm: Add CONFIG_DRM_WERROR")
> Signed-off-by: Jani Nikula
> ---
The change makes sense to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v2 3/3] drm/panic: Add a kmsg panic screen
Jocelyn Falempe writes:
> Add a kmsg option, which will display the last lines of kmsg,
> and should be similar to fbcon.
> Add a drm.panic_screen module parameter, so you can choose between
> the different panic screens available.
> two options currently, but more will be added later:
> * "user": a short message telling the user to reboot the machine.
> * "kmsg": fill the screen with the last lines of kmsg.
>
> You can even change it at runtime by writing to
> /sys/module/drm/parameters/panic_screen
>
Great!
> v2:
> * use module parameter instead of Kconfig choice
>(Javier Martinez Canillas)
>
> Signed-off-by: Jocelyn Falempe
> ---
> drivers/gpu/drm/Kconfig | 11
> drivers/gpu/drm/drm_panic.c | 108
> 2 files changed, 109 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 9703429de6b9..944815cee080 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -137,6 +137,17 @@ config DRM_PANIC_DEBUG
> This is unsafe and should not be enabled on a production build.
> If in doubt, say "N".
>
> +config DRM_PANIC_SCREEN
> + string "Panic screen formater"
> + default "user"
> + depends on DRM_PANIC
> + help
> + This option enable to choose what will be displayed when a kernel
> + panic occurs. You can choose between "user", a short message telling
> + the user to reboot the system, or "kmsg" which will display the last
> + lines of kmsg.
Maybe I would mention here that this is only about the default, but that
can be changed using the "drm.panic_screen=" kernel cmdline parameter or
writting to the /sys/module/drm/parameters/panic_screen sysfs entry.
[...]
> +static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb)
> +{
> + u32 fg_color = convert_from_xrgb(CONFIG_DRM_PANIC_FOREGROUND_COLOR,
> sb->format->format);
> + u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR,
> sb->format->format);
> + const struct font_desc *font = get_default_font(sb->width, sb->height,
> NULL, NULL);
Dan reported that get_default_font() can return NULL
> + struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height);
> + struct kmsg_dump_iter iter;
> + char kmsg_buf[512];
> + size_t kmsg_len;
> + struct drm_panic_line line;
> + int yoffset = sb->height - font->height - (sb->height % font->height) /
> 2;
> +
> + if (!font)
> + return;
> +
... so you have to calculate yoffset after checking if the font is not NULL.
with that fixed:
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v2 2/3] drm/panic: Add a set_pixel() callback to drm_scanout_buffer
Jocelyn Falempe writes: > This allows drivers to draw the pixel, and handle tiling, or specific > color formats. > > v2: > * Use fg_color for blit() functions (Javier Martinez Canillas) > > Signed-off-by: Jocelyn Falempe > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 1/3] drm/panic: only draw the foreground color in drm_panic_blit()
Jocelyn Falempe writes:
Hello Jocelyn,
> The whole framebuffer is cleared, so it's useless to rewrite the
> background colored pixels. It allows to simplify the drawing
> functions, and prepare the work for the set_pixel() callback.
>
> v2:
> * keep fg16/fg24/fg32 as variable name for the blit function.
> * add drm_panic_is_pixel_fg() to avoid code duplication.
> both suggested by Javier Martinez Canillas
>
> Signed-off-by: Jocelyn Falempe
> ---
> drivers/gpu/drm/drm_panic.c | 67 +
> 1 file changed, 31 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 7ece67086cec..056494ae1ede 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -194,40 +194,42 @@ static u32 convert_from_xrgb(u32 color, u32 format)
> /*
> * Blit & Fill
> */
> +/* check if the pixel at coord x,y is 1 (foreground) or 0 (background) */
> +static bool drm_panic_is_pixel_fg(const u8 *sbuf8, unsigned int spitch, int
> x, int y)
> +{
> + return (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) != 0;
> +}
Thanks for doing this! The code is much easier to follow now IMO.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 3/3] drm/panic: Add a kmsg dump screen
Jocelyn Falempe writes: > On 31/05/2024 11:32, Javier Martinez Canillas wrote: >> Jocelyn Falempe writes: >> >>> Add a kmsg dump option, which will display the last lines of kmsg, >>> and should be similar to fbcon. >>> Add a Kconfig choice for the panic screen, so that the user can >>> choose between this new kmsg dump, or the userfriendly option. >>> >>> Signed-off-by: Jocelyn Falempe >>> --- >>> drivers/gpu/drm/Kconfig | 21 + >>> drivers/gpu/drm/drm_panic.c | 151 +++- >>> 2 files changed, 136 insertions(+), 36 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>> index 9703429de6b9..78d401b55102 100644 >>> --- a/drivers/gpu/drm/Kconfig >>> +++ b/drivers/gpu/drm/Kconfig >>> @@ -137,6 +137,27 @@ config DRM_PANIC_DEBUG >>> This is unsafe and should not be enabled on a production build. >>> If in doubt, say "N". >>> >>> +choice >>> + prompt "Panic screen formater" >>> + default DRM_PANIC_SCREEN_USERFRIENDLY >>> + depends on DRM_PANIC >>> + help >>> + This option enable to choose what will be displayed when a kernel >>> + panic occurs. >>> + >>> + config DRM_PANIC_SCREEN_USERFRIENDLY >>> + bool "Default user friendly message" >>> + help >>> + Only a short message telling the user to reboot the system. >>> + >>> + config DRM_PANIC_SCREEN_KMSG >>> + bool "Display the last lines of kmsg" >>> + help >>> + Display kmsg last lines on panic. >>> + Enable if you are a kernel developer, and want to debug a >>> + kernel panic. >>> +endchoice >> >> Why having it as a compile time option and not a runtime option ? AFAICT >> this could be a drm.panic_format= kernel command line parameter instead. > > Yes, I didn't think about it. That would allow to get more debug > information from a user without rebuilding the kernel. > > I'll prepare a v2 with that. > Awesome, thanks! > I prefer to use a drm.panic_screen=, as "format" might be confusing with > the color format ? > Indeed. I named _format because your prompt had "formater" in it, but agree that _screen makes more sense and is easier to understand. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 3/3] drm/panic: Add a kmsg dump screen
Jocelyn Falempe writes: > Add a kmsg dump option, which will display the last lines of kmsg, > and should be similar to fbcon. > Add a Kconfig choice for the panic screen, so that the user can > choose between this new kmsg dump, or the userfriendly option. > > Signed-off-by: Jocelyn Falempe > --- > drivers/gpu/drm/Kconfig | 21 + > drivers/gpu/drm/drm_panic.c | 151 +++- > 2 files changed, 136 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 9703429de6b9..78d401b55102 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -137,6 +137,27 @@ config DRM_PANIC_DEBUG > This is unsafe and should not be enabled on a production build. > If in doubt, say "N". > > +choice > + prompt "Panic screen formater" > + default DRM_PANIC_SCREEN_USERFRIENDLY > + depends on DRM_PANIC > + help > + This option enable to choose what will be displayed when a kernel > + panic occurs. > + > + config DRM_PANIC_SCREEN_USERFRIENDLY > + bool "Default user friendly message" > + help > + Only a short message telling the user to reboot the system. > + > + config DRM_PANIC_SCREEN_KMSG > + bool "Display the last lines of kmsg" > + help > + Display kmsg last lines on panic. > + Enable if you are a kernel developer, and want to debug a > + kernel panic. > +endchoice Why having it as a compile time option and not a runtime option ? AFAICT this could be a drm.panic_format= kernel command line parameter instead. [...] > -/* > - * Draw the panic message at the center of the screen > - */ > +#if defined(CONFIG_DRM_PANIC_SCREEN_USERFRIENDLY) > + And that could avoid ifdefery in the C file. [...] > +#elif defined(CONFIG_DRM_PANIC_SCREEN_KMSG) > + > +#include > +#include > + I would add these even if guarded by DRM_PANIC_SCREEN_KMSG, to the start of the C file where are the other headers include directives. The patch looks good to me though, so if you prefer to keep it as a build time option: Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 2/3] drm/panic: Add a set_pixel() callback to drm_scanout_buffer
Jocelyn Falempe writes:
> This allows drivers to draw the pixel, and handle tiling, or specific
> color formats.
>
> Signed-off-by: Jocelyn Falempe
> ---
> drivers/gpu/drm/drm_panic.c | 120 +++-
> include/drm/drm_panic.h | 9 +++
> 2 files changed, 85 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 9d95c7eaae83..27e26b9d842c 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -241,40 +241,54 @@ static void drm_panic_blit32(struct iosys_map *dmap,
> unsigned int dpitch,
> iosys_map_wr(dmap, y * dpitch + x *
> sizeof(u32), u32, color);
> }
>
> +static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct
> drm_rect *clip,
> + const u8 *sbuf8, unsigned int spitch, u32
> color)
> +{
> + unsigned int y, x;
> +
> + for (y = 0; y < drm_rect_height(clip); y++)
> + for (x = 0; x < drm_rect_width(clip); x++)
> + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8)))
You have the same check for fb vs bg in all your blit helpers, so maybe
this can be a macro or static inline function instead ? That would also
help with the issue I mentioned about making the logic easier to read.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 1/3] drm/panic: only draw the foreground color in drm_panic_blit()
Jocelyn Falempe writes:
Hello Jocelyn,
> The whole framebuffer is cleared, so it's useless to rewrite the
> background colored pixels. It allows to simplify the drawing
> functions, and prepare the work for the set_pixel() callback.
>
> Signed-off-by: Jocelyn Falempe
> ---
> drivers/gpu/drm/drm_panic.c | 63 +++--
> 1 file changed, 26 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 7ece67086cec..9d95c7eaae83 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -197,37 +197,33 @@ static u32 convert_from_xrgb(u32 color, u32 format)
> static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch,
>const u8 *sbuf8, unsigned int spitch,
>unsigned int height, unsigned int width,
> - u16 fg16, u16 bg16)
> + u16 color)
What about calling this fg16 instead of color? That way is clear that only
the fb is written and not the background ?
> {
> unsigned int y, x;
> - u16 val16;
>
> - for (y = 0; y < height; y++) {
> - for (x = 0; x < width; x++) {
> - val16 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x %
> 8))) ? fg16 : bg16;
> - iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16,
> val16);
> - }
> - }
> + for (y = 0; y < height; y++)
> + for (x = 0; x < width; x++)
I would add here a comment that this check is about determining if a color
is suitable for foreground or background, depending on the luminance
threshold (which I understand is the 0x80 value?).
> + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8)))
> + iosys_map_wr(dmap, y * dpitch + x *
> sizeof(u16), u16, color);
> }
>
> static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch,
>const u8 *sbuf8, unsigned int spitch,
>unsigned int height, unsigned int width,
> - u32 fg32, u32 bg32)
> + u32 color)
> {
> unsigned int y, x;
> - u32 val32;
>
Same here, I would left the variable name as fg32.
[...]
and also here would add a comment or use a variable to make it more readable.
Same comments for drm_panic_blit32().
[...]
> /*
> @@ -256,8 +249,7 @@ static void drm_panic_blit32(struct iosys_map *dmap,
> unsigned int dpitch,
> * @spitch: source pitch in bytes
> * @height: height of the image to copy, in pixels
> * @width: width of the image to copy, in pixels
> - * @fg_color: foreground color, in destination format
> - * @bg_color: background color, in destination format
> + * @color: foreground color, in destination format
Leaving as fg_color would even be consistent with your comment.
Feel free to ignore my comments though if you disagree, the patch looks
good to me regardless.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/display: Fix HDMI state helper dependency
Maxime Ripard writes:
> During the life of the series that introduced the
> DRM_DISPLAY_HDMI_STATE_HELPER option, we reworked the Kconfig option
> dependency setup to rely on depends on with commit f6d2dc03fa85 ("drm:
> Switch DRM_DISPLAY_HDMI_HELPER to depends on") which got reverted later
> on because it was creating too many issues by commit d7c128cb775e
> ("Revert "drm: Switch DRM_DISPLAY_HDMI_HELPER to depends on"").
>
> However, since the series was out of tree at that time,
> DRM_DISPLAY_HDMI_STATE_HELPER wasn't properly updated to take the revert
> into account and is now creating build issues.
>
> Let's switch the depends on to a select to fix this.
>
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405290332.sqtt0ix0-...@intel.com/
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405290438.toyhxmin-...@intel.com/
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405290803.c3178dyt-...@intel.com/
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405291109.pqdqc46g-...@intel.com/
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405291221.a0nstxhe-...@intel.com/
> Fixes: 54cb39e2293b ("drm/connector: hdmi: Create an HDMI sub-state")
> Signed-off-by: Maxime Ripard
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/sun4i: Fix compilation error
Maxime Ripard writes:
Hello Maxime,
> Commit ea64761a54a2 ("drm/sun4i: hdmi: Switch to HDMI connector")
> introduced a dependency that got renamed in a previous version, but
> wasn't properly updated in that driver. Fix the name of the function.
>
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405282205.eu7nuoeq-...@intel.com/
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405282248.u2lhpvck-...@intel.com/
> Fixes: ea64761a54a2 ("drm/sun4i: hdmi: Switch to HDMI connector")
> Signed-off-by: Maxime Ripard
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/tests: fix drm_test_fb_xrgb8888_to_xrgb2101010 on big endian
Dave Airlie writes:
Hello Dave,
> From: Dave Airlie
>
> This test is failing for me on s390x and there is a report on the list from
> ppc64.
>
> This aligns it with the argb test that doesn't fail.
>
> Fixes: 15bda1f8de5d ("drm/tests: Add calls to drm_fb_blit() on supported
> format conversion tests")
> Reported-by: Erhard Furtner
> Signed-off-by: Dave Airlie
> ---
> drivers/gpu/drm/tests/drm_format_helper_test.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c
> b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 08992636ec05..d4ce2d7ced4e 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -991,7 +991,7 @@ static void drm_test_fb_xrgb_to_xrgb2101010(struct
> kunit *test)
> NULL : >dst_pitch;
>
> drm_fb_xrgb_to_xrgb2101010(, dst_pitch, , ,
> >clip, _state);
> - buf = le32buf_to_cpu(test, buf, dst_size / sizeof(u32));
> + buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size /
> sizeof(u32));
> KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
>
> buf = dst.vaddr; /* restore original value of buf */
> @@ -1002,6 +1002,8 @@ static void drm_test_fb_xrgb_to_xrgb2101010(struct
> kunit *test)
> blit_result = drm_fb_blit(, dst_pitch, DRM_FORMAT_XRGB2101010,
> , ,
> >clip, _state);
>
> + buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size /
> sizeof(u32));
> +
> KUNIT_EXPECT_FALSE(test, blit_result);
> KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
> }
Looks good to me, and as you said it makes the test consistent with the
drm_fb_xrgb_to_argb2101010() test that didn't fail for you on s390x.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing
T binding is as you said for flexiblity and make the design future-proof. >>> separate irq >>> Coming back to your questions, with the current scheme the Linux (tidss) >>> would >>> be expected to make sure the CRTC being shared with RTOS is never shutdown >>> and >>> the RTOS plane should never gets masked. >> >> I'm probably missing something then here, but if the Linux side of >> things is expected to keep the current configuration and keep it active >> for it to work, what use-case would it be useful for? >> > > It's just one of the partitioning possibilities that I mentioned here, that > Linux is in control of DSS as a whole and the user want the other host (be it > RTOS or any other core) to control a single plane. For e.g it could be Linux > (with GPU rendering) displaying the graphics and RTOS overlaying a real time > clock or any other signs which need to be displayed in real-time. > But more than the use-case this is inspired by the fact that we want to be > flexible and support in the linux driver whatever partitioning scheme > possibilities are there which are supported in hardware and we let user decide > on the partitioning scheme. > A possible use case here could be if Linux is safer than the other host owning a single plane, right? Then in that case the RTOS could fail but the display pipeline won't be teared down. That is, if your safety tell-tales would be driven by Linux and having other OS dislay the GPU-rendered QT based application on another plane. But as said, for now that's a theorethical use case since the one you mentioned is the opposite. [] >>> >>>> It's not just about interrupts, it's also about how your arbitrate >>>> between what Linux wants and what the RTOS wants. Like if the RTOS still >>>> wants to output something but Linux wants to disable it, how do you >>>> reconcile the two? >>>> >>> >>> The scheme involves static partitioning of display resource which are >>> assigned >>> compile-time to RTOS and Linux. Here the RTOS firmware is compiled with >>> specific ownership/display resources as desired by user and this assignment >>> stays intact. >>> >>> If there is a more complex use-case which requires dynamic >>> assignment/arbitration of resources then I agree those require some sort of >>> IPC scheme but this is not what we target with these series. This series is >>> simply to support static partitioning feature (separate register space, >>> separate irq, firewalling support etc) of TI DSS hardware across the >>> multiple >>> hosts and there are use-cases too for which this scheme suffices. >> >> I think you're right and we have a misunderstanding. My initial >> assumption was that it was to prevent the Linux side of sides from >> screwing up the output if it was to crash. >> >> But it looks like it's not the main point of this series, so could you >> share some use-cases you're trying to address? >> > > The end use-case we have demonstrated right now with this series is a > proof-of-concept display cluster use-case where RTOS boots early on MCU core > (launched at bootloader stage) and initializes the display (using the global > common0 register space and irq) and starts displaying safety tell-tales on one > plane, and once Linux boots up on application processor, > Linux (using common1 register space and irq) controls the other plane with GPU > rendering using a QT based application. And yes, we also support the scenario > where Linux crashes but RTOS being the DSS master and in control of DSS power, > clock domain and global register space is not impacted by the crash. You mention 2 scenarios but are actually the same? Or did I misunderstand? In both cases the RTOS own the display pipeline and Linux can just display using a single plane. That's why I think that agree with Maxime, that a fwkms could be a simpler solution to your use case instead of adding all this complexity to the DSS driver. Yes, I understand the HW supports all this flexibility but there's no real use case yet (you mentioned that don't even have firmware for this single plane owned by the RTOS in the R5F case). The DT binding for a fwkms driver would be trivial, in fact maybe we might even leverage simpledrm for this case and not require a new driver at all. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm/fbdev-dma: Clean up deferred I/O
Thomas Zimmermann writes:
> Call fb_deferred_io_cleanup() upon destroying the framebuffer
> device. Releases the internal memory.
>
> Signed-off-by: Thomas Zimmermann
> Fixes: 808a40b69468 ("drm/fbdev-dma: Implement damage handling and deferred
> I/O")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> ---
> drivers/gpu/drm/drm_fbdev_dma.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/fbdev-shmem: Clean up deferred I/O
Thomas Zimmermann writes:
Hello Thomas,
> Call fb_deferred_io_cleanup() upon destroying the framebuffer
> device. Releases the internal memory.
>
> Signed-off-by: Thomas Zimmermann
> Fixes: 150f431a0831 ("drm/fbdev: Add fbdev-shmem")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm: use "0" instead of "" for deprecated driver date
Jani Nikula writes:
> libdrm does not like the empty string for driver date. Use "0" instead,
> which has been used by virtio previously.
>
> Reported-by: Steven Price
> Closes: https://lore.kernel.org/r/9d0cff47-308e-4b11-a9f3-4157dc26b...@arm.com
> Fixes: 7fb8af6798e8 ("drm: deprecate driver date")
> Signed-off-by: Jani Nikula
> ---
It's a pity that libdrm can't cope with the empty string, using 0 makes sense.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: simpledrm, running display servers, and drivers replacing simpledrm while the display server is running
nerdopolis writes: Hello, > Hi > > So I have been made aware of an apparent race condition of some drivers > taking a bit longer to load, which could lead to a possible race condition of > display servers/greeters using the simpledrm device, and then experiencing > problems once the real driver loads, the simpledrm device that the display > servers are using as their primary GPU goes away. > Plymouth also had this issue and that is the reason why simpledrm is not treated as a KMS device by default (unless plymouth.use-simpledrm used). > For example Weston crashes, Xorg crashes, wlroots seems to stay running, but > doesn't draw anything on the screen, kwin aborts, > This is if you boot on a QEMU machine with the virtio card, with > modprobe.blacklist=virtio_gpu, and then, when the display server is running, > run sudo modprobe virtio-gpu > > Namely, it's been recently reported here: > https://github.com/sddm/sddm/issues/1917[1] and here > https://github.com/systemd/systemd/issues/32509[2] > > My thinking: Instead of simpledrm's /dev/dri/card0 device going away when the > real driver loads, is it possible for simpledrm to instead simulate an unplug > of the fake display/CRTC? > That way in theory, the simpledrm device will now be useless for drawing for > drawing to the screen at that point, since the real driver is now taken over, > but this way here, at least the display server doesn't lose its handles to > the /dev/dri/card0 device, (and then maybe only remove itself once the final > handle to it closes?) > > Is something like this possible to do with the way simpledrm works with the > low level video memory? Or is this not possible? > How it works is that when a native DRM driver is probed, it calls to the drm_aperture_remove_conflicting_framebuffers() to kick out the generic system framebuffer video drivers and the aperture infrastructure does a device (e.g: "simple-framebuffer", "efi-framebuffer", etc) unregistration. So is not only that the /dev/dri/card0 devnode is unregistered but that the underlaying platform device bound to the simpledrm/efifb/vesafb/simplefb drivers are unregistered, and this leads to the drivers being unregistered as well by the Linux device model infrastructure. But also, this seems to be user-space bugs for me and doing anything in the kernel is papering over the real problem IMO. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm: deprecate driver date
Jani Nikula writes: > The driver date serves no useful purpose, because it's hardly ever > updated. The information is misleading at best. > > As described in Documentation/gpu/drm-internals.rst: > > The driver date, formatted as MMDD, is meant to identify the date > of the latest modification to the driver. However, as most drivers > fail to update it, its value is mostly useless. The DRM core prints it > to the kernel log at initialization time and passes it to userspace > through the DRM_IOCTL_VERSION ioctl. > > Stop printing the driver date at init, and start returning the empty > string "" as driver date through the DRM_IOCTL_VERSION ioctl. > > The driver date initialization in drivers and the struct drm_driver date > member can be removed in follow-up. > > Signed-off-by: Jani Nikula > > --- I never understood the value of it and so this patch makes sense to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm/panthor: Add defer probe for firmware load
Steven Price writes: > Hi Javier, > > On 25/04/2024 10:22, Javier Martinez Canillas wrote: >> Steven Price writes: >> >> Hello Steven, >> >>> On 13/04/2024 12:49, Andy Yan wrote: >>>> From: Andy Yan >>>> >>>> The firmware in the rootfs will not be accessible until we >>>> are in the SYSTEM_RUNNING state, so return EPROBE_DEFER until >>>> that point. >>>> This let the driver can load firmware when it is builtin. >>> >>> The usual solution is that the firmware should be placed in the >>> initrd/initramfs if the module is included there (or built-in). The same >>> issue was brought up regarding the powervr driver: >>> >>> https://lore.kernel.org/dri-devel/20240109120604.603700-1-javi...@redhat.com/T/ >>> >>> I'm not sure if that ever actually reached a conclusion though. The >>> question was deferred to Greg KH but I didn't see Greg actually getting >>> involved in the thread. >>> >> >> Correct, there was not conclusion reached in that thread. > > So I think we need a conclusion before we start applying point fixes to > individual drivers. > Agreed. [...] >> >> In the thread you referenced I suggested to add that logic in >> request_firmware() >> (or add a new request_firmware_defer() helper function) that changes the >> request >> firmare behaviour to return -EPROBE_DEFER instead of -ENOENT. > > That would seem like a good feature if it's agreed that deferring on > request_firmware is a good idea. > Yeah. I didn't attempt to type that patch because didn't get an answer from Greg and didn't want to spent the time writing and testing a patch to just be nacked. >> Since as you mentioned, this isn't specific to panthor and an issue that I >> also >> faced with the powervr driver. > > I'm not in any way against the idea of deferring the probe until the > firmware is around - indeed it seems like a very sensible idea in many > respects. But I don't want panthor to be 'special' in this way. > > If the consensus is that the firmware should live with the module (i.e. > either both in the initramfs or both in the rootfs) then the code is > fine as it is. That seemed to be the view of Sima in that thread and > seems reasonable to me - why put the .ko in the initrd if you can't > actually use it until the rootfs comes along? > That's indeed a sensible position for me as well and is what I answered to the user who reported the powervr issue. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm/panthor: Add defer probe for firmware load
Steven Price writes:
Hello Steven,
> On 13/04/2024 12:49, Andy Yan wrote:
>> From: Andy Yan
>>
>> The firmware in the rootfs will not be accessible until we
>> are in the SYSTEM_RUNNING state, so return EPROBE_DEFER until
>> that point.
>> This let the driver can load firmware when it is builtin.
>
> The usual solution is that the firmware should be placed in the
> initrd/initramfs if the module is included there (or built-in). The same
> issue was brought up regarding the powervr driver:
>
> https://lore.kernel.org/dri-devel/20240109120604.603700-1-javi...@redhat.com/T/
>
> I'm not sure if that ever actually reached a conclusion though. The
> question was deferred to Greg KH but I didn't see Greg actually getting
> involved in the thread.
>
Correct, there was not conclusion reached in that thread.
>> Signed-off-by: Andy Yan
>> ---
>>
>> drivers/gpu/drm/panthor/panthor_fw.c | 11 ++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c
>> b/drivers/gpu/drm/panthor/panthor_fw.c
>> index 33c87a59834e..25e375f8333c 100644
>> --- a/drivers/gpu/drm/panthor/panthor_fw.c
>> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
>> @@ -1336,8 +1336,17 @@ int panthor_fw_init(struct panthor_device *ptdev)
>> }
>>
>> ret = panthor_fw_load(ptdev);
>> -if (ret)
>> +if (ret) {
>> +/*
>> + * The firmware in the rootfs will not be accessible until we
>> + * are in the SYSTEM_RUNNING state, so return EPROBE_DEFER until
>> + * that point.
>> + */
>> +if (system_state < SYSTEM_RUNNING)
>
> This should really only be in the case where ret == -ENOENT - any other
> error and the firmware is apparently present but broken in some way, so
> there's no point deferring.
>
> I also suspect we'd need some change in panthor_fw_load() to quieten
> error messages if we're going to defer on this, in which case it might
> make more sense to move this logic into that function.
>
In the thread you referenced I suggested to add that logic in request_firmware()
(or add a new request_firmware_defer() helper function) that changes the request
firmare behaviour to return -EPROBE_DEFER instead of -ENOENT.
Since as you mentioned, this isn't specific to panthor and an issue that I also
faced with the powervr driver.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v2 29/43] drm/renesas/rz-du: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by rz-du. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Biju Das > Tested-by: Biju Das > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 19/43] drm/virtio: Use fbdev-shmem
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: David Airlie > Cc: Gerd Hoffmann > Cc: Gurchetan Singh > Cc: Chia-I Wu > Tested-by: Dmitry Osipenko > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 21/43] drm/fbdev-dma: Implement damage handling and deferred I/O
Thomas Zimmermann writes:
> Hi
>
> Am 16.04.24 um 14:18 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann writes:
>>
[...]
>>> +static int drm_fbdev_dma_fb_mmap(struct fb_info *info, struct
>>> vm_area_struct *vma)
>>> +{
>>> + struct drm_fb_helper *fb_helper = info->par;
>>> + struct drm_framebuffer *fb = fb_helper->fb;
>>> + struct drm_gem_dma_object *dma = drm_fb_dma_get_gem_obj(fb, 0);
>>> +
>>> + if (!dma->map_noncoherent)
>>> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>> I noticed that some drivers do:
>>
>> vma->vm_page_prot =
>> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>
>> I see that vm_get_page_prot() is a per-architecture function, but I don't
>> know about the implications of getting the pgprot_t from the vma->vm_flags
>> set or just using the current vma->vm_page_prot value...
>
> That's in interesting observation. The code in the patch adds a WC flag
> to the existing vm_page_prot. The code in your example first creates a
> new vm_page_prot from the vm_flags field. Fbdev drivers generally use
> the former approach. So where does the original vm_page_prot value come
> from? (I think that's also the question behind your comment.)
>
Yes, also if the vm_flags were set (and where) for this VMA.
> I've looked through the kernel's mmap code from the syscall [1] to the
> place where it invokes the mmap callback. [2] Shortly before doing so,
> mmap_region() set's vm_page_prot from vm_flags like in your example. [3]
> I would assume there's no reason for drivers to call vm_get_page_prot()
> by themselves. DRM drivers specially seem to have the habit of doing so.
>
Got it, makes sense. Thanks for taking a look.
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v2 43/43] drm/fbdev: Clean up fbdev documentation
Thomas Zimmermann writes: > Rewrite some docs that are not up-to-date any longer. Remove the TODO > item for fbdev-generic conversion, as the helper has been replaced. Make > documentation for DMA, SHMEM and TTM emulation available. > > Signed-off-by: Thomas Zimmermann > Cc: Jonathan Corbet > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 42/43] drm/fbdev-generic: Convert to fbdev-ttm
Thomas Zimmermann writes: > Only TTM-based drivers use fbdev-generic. Rename it to fbdev-ttm and > change the symbol infix from _generic_ to _ttm_. Link the source file > into TTM helpers, so that it is only build if TTM-based drivers have > been selected. Select DRM_TTM_HELPER for loongson. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 36/43] drm/tiny/ili9486: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by ili9486. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/tiny/ili9486.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 35/43] drm/tiny/ili9341: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by ili9341. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Kamlesh Gurudasani > --- > drivers/gpu/drm/tiny/ili9341.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 33/43] drm/tiny/ili9163: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by ili9163. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 32/43] drm/tiny/hx8357d: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by hx8357d. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 31/43] drm/rockchip: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by rockchip. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Sandy Huang > Cc: "Heiko Stübner" > Cc: Andy Yan > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 27/43] drm/panel/panel-ilitek-9341: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by panel-ilitek-9341. Avoids > the overhead of fbdev-generic's additional shadow buffering. No > functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Neil Armstrong > Cc: Jessica Zhang > Cc: Sam Ravnborg > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 26/43] drm/mediatek: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by ingenic. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Chun-Kuang Hu > Cc: Philipp Zabel > --- > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 24/43] drm/imx/lcdc: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by lcdc. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Pengutronix Kernel Team > Cc: Fabio Estevam > Cc: NXP Linux Team > --- > drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 23/43] drm/hisilicon/kirin: Use fbdev-dma
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by kirin. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Xinliang Liu > Cc: Tian Tao > Cc: Xinwei Kong > Cc: Sumit Semwal > Cc: Yongqin Liu > Cc: John Stultz > --- > drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 08/43] drm/fbdev: Add fbdev-shmem
Thomas Zimmermann writes: [...] >> >> I guess is OK because is_vmalloc_addr() is always true for this case ? > > It's not a vmalloc'ed address, but see patch 7. Fbdev-shmem uses a new > get_page callback in fb_defio. It provides the necessary page directly > to fb_defio. > Thanks! That was the missing piece of the puzzle. I didn't look at that patch because I noticed that already had a r-b. It makes more sense now :) > >> >> This also made me think why info->fix.smem_len is really needed. Can't we >> make the fbdev core to only look at that if info->screen_size is not set ? > > The fbdev core doesn't use smem_len AFAICT. But smem_len is part of the > fbdev UAPI, so I set it. I assume that programs use it to go to the end > of the framebuffer memory. > I see. Makes sense. > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 21/43] drm/fbdev-dma: Implement damage handling and deferred I/O
Thomas Zimmermann writes:
> Add support for damage handling and deferred I/O to fbdev-dma. This
> enables fbdev-dma to support all DMA-memory-based DRM drivers, even
> such with a dirty callback in their framebuffers.
>
> The patch adds the code for deferred I/O and also sets a dedicated
> helper for struct fb_ops.fb_mmap that support coherent mappings.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_fbdev_dma.c | 65 ++---
> 1 file changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
> index 6c9427bb4053b..8ffd072368bca 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
> @@ -4,6 +4,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -35,6 +36,22 @@ static int drm_fbdev_dma_fb_release(struct fb_info *info,
> int user)
> return 0;
> }
>
> +FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(drm_fbdev_dma,
> +drm_fb_helper_damage_range,
> +drm_fb_helper_damage_area);
> +
Shouldn't this be FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS() instead ?
I know that right now the macros are the same but I believe that it was
added it for a reason ?
> +static int drm_fbdev_dma_fb_mmap(struct fb_info *info, struct vm_area_struct
> *vma)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_framebuffer *fb = fb_helper->fb;
> + struct drm_gem_dma_object *dma = drm_fb_dma_get_gem_obj(fb, 0);
> +
> + if (!dma->map_noncoherent)
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
I noticed that some drivers do:
vma->vm_page_prot =
pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
I see that vm_get_page_prot() is a per-architecture function, but I don't
know about the implications of getting the pgprot_t from the vma->vm_flags
set or just using the current vma->vm_page_prot value...
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v2 20/43] drm/vkms: Use fbdev-shmem
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Rodrigo Siqueira > Cc: Melissa Wen > Cc: "MaÃra Canal" > Cc: Haneen Mohammed > Cc: Daniel Vetter > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 18/43] drm/udl: Use fbdev-shmem
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Dave Airlie > Cc: Sean Paul > Cc: Thomas Zimmermann > --- > drivers/gpu/drm/udl/udl_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 17/43] drm/tiny/simpledrm: Use fbdev-shmem
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > --- > drivers/gpu/drm/tiny/simpledrm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 16/43] drm/tiny/ofdrm: Use fbdev-shmem
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > --- > drivers/gpu/drm/tiny/ofdrm.c | 4 ++-- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 14/43] drm/tiny/cirrus: Use fbdev-shmem
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 13/43] drm/solomon: Use fbdev-shmem
Thomas Zimmermann writes: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Javier Martinez Canillas > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2 08/43] drm/fbdev: Add fbdev-shmem
Thomas Zimmermann writes: Hello Thomas, > Add an fbdev emulation for SHMEM-based memory managers. The code is > similar to fbdev-generic, but does not require an addition shadow "additional" I think ? > buffer for mmap(). Fbdev-shmem operates directly on the buffer object's > SHMEM pages. Fbdev's deferred-I/O mechanism updates the hardware state > on write operations. > > v2: > - use drm_driver_legacy_fb_format() (Geert) > > Signed-off-by: Thomas Zimmermann > --- Patch looks good to me. Reviewed-by: Javier Martinez Canillas Just a couple of questions below: > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_fbdev_shmem.c | 316 ++ Should fbdev-generic then be renamed to fbdev_shmem_shadow or something like that ? [...] > + > + /* screen */ > + info->flags |= FBINFO_VIRTFB; /* system memory */ > + if (!shmem->map_wc) > + info->flags |= FBINFO_READS_FAST; /* signal caching */ > + info->screen_size = sizes->surface_height * fb->pitches[0]; > + info->screen_buffer = map.vaddr; > + info->fix.smem_len = info->screen_size; > + Do I understand correctly that info->fix.smem_start doesn't have to be set because that's only used for I/O memory? Since drm_fbdev_shmem_fb_mmap() calls fb_deferred_io_mmap() which in turn sets vma->vm_ops = _deferred_io_vm_ops and struct vm_operations_struct fb_deferred_io_vm_ops .fault function handler is fb_deferred_io_fault() that calls fb_deferred_io_page() which uses info->fix.smem_start value. I guess is OK because is_vmalloc_addr() is always true for this case ? This also made me think why info->fix.smem_len is really needed. Can't we make the fbdev core to only look at that if info->screen_size is not set ? -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH RESEND] drm/armada: drop unneeded MODULE_ALIAS
Neil Armstrong writes:
Hello Neal,
> On 10/04/2024 10:22, Krzysztof Kozlowski wrote:
>> The MODULE_DEVICE_TABLE already creates proper alias for platform
>> driver. Having another MODULE_ALIAS causes the alias to be duplicated.
>>
>> Signed-off-by: Krzysztof Kozlowski
>>
>> ---
>>
>> Resent third time
>> https://lore.kernel.org/all/20220407202443.23000-1-krzysztof.kozlow...@linaro.org/
>>
>> Cc: Dmitry Baryshkov
>> Cc: Neil Armstrong
>> ---
>> drivers/gpu/drm/armada/armada_drv.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/armada/armada_drv.c
>> b/drivers/gpu/drm/armada/armada_drv.c
>> index e51ecc4f7ef4..f48e2cf8de43 100644
>> --- a/drivers/gpu/drm/armada/armada_drv.c
>> +++ b/drivers/gpu/drm/armada/armada_drv.c
>> @@ -283,4 +283,3 @@ module_exit(armada_drm_exit);
>> MODULE_AUTHOR("Russell King ");
>> MODULE_DESCRIPTION("Armada DRM Driver");
>> MODULE_LICENSE("GPL");
>> -MODULE_ALIAS("platform:armada-drm");
>
> Reviewed-by: Neil Armstrong
>
> I think we'll need maxime or thomas ack to apply this via drm-misc-next
>
I don't think you need an explicit ack for them to commit it:
https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#merge-criteria
BTW, the patch looks good to me as well.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm: remove unused header gma_drm.h
Jani Nikula writes: > gma_drm.h has become an empty, unused header. Remove. > > Cc: Patrik Jakobsson > Signed-off-by: Jani Nikula > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] fbdev: Select I/O-memory framebuffer ops for SBus
Thomas Zimmermann writes:
> Framebuffer I/O on the Sparc Sbus requires read/write helpers for
> I/O memory. Select FB_IOMEM_FOPS accordingly.
>
> Reported-by: Nick Bowler
> Closes:
> https://lore.kernel.org/lkml/5bc21364-41da-a339-676e-5bb0f4fae...@draconx.ca/
> Signed-off-by: Thomas Zimmermann
> Fixes: 8813e86f6d82 ("fbdev: Remove default file-I/O implementations")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Daniel Vetter
> Cc: Helge Deller
> Cc: Sam Ravnborg
> Cc: Arnd Bergmann
> Cc: Geert Uytterhoeven
> Cc: linux-fb...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: # v6.8+
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: drm/tiny: QUESTION: What to use instead of drm_simple_display_pipe ?
Thomas Zimmermann writes:
Hello Mehdi,
> Hi
>
> Am 18.03.24 um 20:18 schrieb Mehdi Djait:
>> Hello everyone :)
>>
>> I am implementing a tiny drm driver and I am currently working on the
>> V2:
>> https://lore.kernel.org/dri-devel/cover.1701267411.git.mehdi.dj...@bootlin.com/
>>
>> I got a review on the v1 telling me not to use the
>> drm_simple_display_pipe. Can someone please explain this further ? Or
>> give me an example drm driver that does it the right way ?
>
> You can copy the code from drm_simple_kms_helper.c into your driver file
> and start inlining everything. For example
>
> Â 1) Your driver calls drm_simple_display_pipe_init(), so you copy that
> code into your source file
> Â 2) drm_simple_display_pipe_init() uses drm_simple_kms_plane_funcs and
> drm_simple_kms_crtc_funcs, so you copy these into your source file;
> together with the drm_simple_kms_*() helpers that they use for their
> callback pointers.
> Â 3) Mayb do this for other drm_simple_kms_*() code.
> Â 4) Then start inlining: inline your copy of
> drm_simple_display_pipe_iit(). Instead of using
> sharp_ls027b7dh01_pipe_funcs, inline its functions into your copy of the
> callers. And so on.
> Â 5) Rename the resulting code, so that it fits you driver.
>
> With careful changes, you 'll end up with the same functionality as
> before, but without the intermediate layer of the simple-KMS code.
>
On top of what Thomas said, you can check 622113b9f11f ("drm/ssd130x:
Replace simple display helpers with the atomic helpers") that did this
change for the drivers/gpu/drm/solomon/ssd130x.c driver.
The driver is also for a monochrome panel controller and it does support
SPI as transport, which means the controller is similar to yours in many
aspects. You could use that driver code as a reference for your driver.
> Best regards
> Thomas
>
>>
>> --
>> Kind Regards
>> Mehdi Djait
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 07/43] fbdev/deferred-io: Provide get_page hook in struct fb_deferred_io
Thomas Zimmermann writes: > Add a callback for drivers to provide framebuffer pages to fbdev's > deferred-I/O helpers. Implementations need to acquire a reference on > the page before returning it. Returning NULL generates a SIGBUS > signal. > > This will be useful for DRM's fbdev emulation with GEM-shemem buffer GEM-shmem > objects. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 06/43] fbdev/deferred-io: Always call get_page() for framebuffer pages
Thomas Zimmermann writes: > Unconditionally call get_page() after looking up a page from the > framebuffer memory. Guarantees that we always hold a reference. > > This change also refactors the code such that it can support a > driver-supplied get_page helper. This will be useful for DRM's > fbdev emulation. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 05/43] fbdev/deferred-io: Test smem_start for I/O memory
Thomas Zimmermann writes: > Test smem_start before looking up pages from its value. Return > NULL if it is unset. This will result in a SIGBUS signal. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Maxime Ripard writes:
> On Mon, Mar 18, 2024 at 08:59:01AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 18.03.24 um 03:35 schrieb Zack Rusin:
>> > On Tue, Mar 12, 2024 at 11:48 AM Thomas Zimmermann
>> > wrote:
>> > > Framebuffer memory is allocated via vmalloc() from non-contiguous
>> > > physical pages. The physical framebuffer start address is therefore
>> > > meaningless. Do not set it.
>> > >
>> > > The value is not used within the kernel and only exported to userspace
>> > > on dedicated ARM configs. No functional change is expected.
>> > >
>> > > Signed-off-by: Thomas Zimmermann
>> > > Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
>> > > Cc: Thomas Zimmermann
>> > > Cc: Javier Martinez Canillas
>> > > Cc: Zack Rusin
>> > > Cc: Maarten Lankhorst
>> > > Cc: Maxime Ripard
>> > > Cc: # v6.4+
>> > > ---
>> > > drivers/gpu/drm/drm_fbdev_generic.c | 1 -
>> > > 1 file changed, 1 deletion(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
>> > > b/drivers/gpu/drm/drm_fbdev_generic.c
>> > > index d647d89764cb9..b4659cd6285ab 100644
>> > > --- a/drivers/gpu/drm/drm_fbdev_generic.c
>> > > +++ b/drivers/gpu/drm/drm_fbdev_generic.c
>> > > @@ -113,7 +113,6 @@ static int drm_fbdev_generic_helper_fb_probe(struct
>> > > drm_fb_helper *fb_helper,
>> > > /* screen */
>> > > info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>> > > info->screen_buffer = screen_buffer;
>> > > - info->fix.smem_start =
>> > > page_to_phys(vmalloc_to_page(info->screen_buffer));
>> > > info->fix.smem_len = screen_size;
>> > >
>> > > /* deferred I/O */
>> > > --
>> > > 2.44.0
>> > >
>> > Good idea. I think given that drm_leak_fbdev_smem is off by default we
>> > could remove the setting of smem_start by all of the in-tree drm
>> > drivers (they all have open source userspace that won't mess around
>> > with fbdev fb) - it will be reset to 0 anyway. Actually, I wonder if
>> > we still need drm_leak_fbdev_smem at all...
>>
>> All I know is that there's an embedded userspace driver that requires that
>> setting. I don't even know which hardware.
>
> The original Mali driver (ie, lima) used to require it, that's why we
> introduced it in the past.
>
> I'm not sure if the newer versions of that driver, or if newer Mali
> generations (ie, panfrost and panthor) closed source driver would
> require it, so it might be worth removing if it's easy enough to revert.
>
Agreed. The DRM_FBDEV_LEAK_PHYS_SMEM symbol already depends on EXPERT and
defaults to 'n', which implies that isn't enabled by most kernels AFAICT.
So dropping it and see if anyone complains sounds like a good idea to me.
> Maxime
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 04/43] fbdev/deferred-io: Test screen_buffer for vmalloc'ed memory
Thomas Zimmermann writes: > Framebuffers in virtual memory are available via screen_buffer. Use > it instead of screen_base and avoid the type casting. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 03/43] fbdev/deferred-io: Clean up pageref on lastclose
Thomas Zimmermann writes: > Clean up the pageref state as part of the lastclose helper. This > only requirs to clear the page's mapping field. The pageref and requires > page can stay in place for the next opened instance of the frame- > buffer file. > > With the change in the clean-up logic, here's no further need there's > to lookup pages during the lastclose cleanup. The code instead > uses the existing pagerefs in its look-up table. It also avoids > using smem_len, which some driver might not set correctly. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 02/43] fbdev/deferred-io: Move pageref setup into separate helper
Thomas Zimmermann writes: > Set up struct fb_deferred_io_pageref in th new helper function the > fb_deferred_io_pageref_lookup(), which runs when the pageref is first > taken. Remove the setup code from the rest of the code. > > At first, the code allocates the memory of all pageref structs. The > setup of the various fields happens when the pageref is required. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Thomas Zimmermann writes:
Hello Thomas,
> Framebuffer memory is allocated via vmalloc() from non-contiguous
It's vmalloc() true, but through vzmalloc() so I would mention that
function instead in the commit message.
> physical pages. The physical framebuffer start address is therefore
> meaningless. Do not set it.
>
> The value is not used within the kernel and only exported to userspace
> on dedicated ARM configs. No functional change is expected.
>
How's that info used? Does user-space assumes that the whole memory range
is contiguous in physical memory or just cares about the phyisical start
address ?
> Signed-off-by: Thomas Zimmermann
> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Zack Rusin
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: # v6.4+
> ---
> drivers/gpu/drm/drm_fbdev_generic.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> b/drivers/gpu/drm/drm_fbdev_generic.c
> index d647d89764cb9..b4659cd6285ab 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -113,7 +113,6 @@ static int drm_fbdev_generic_helper_fb_probe(struct
> drm_fb_helper *fb_helper,
> /* screen */
> info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
> info->screen_buffer = screen_buffer;
> - info->fix.smem_start =
> page_to_phys(vmalloc_to_page(info->screen_buffer));
> info->fix.smem_len = screen_size;
>
Makes sense:
Reviewed-by: Javier Martinez Canillas
What about drivers/gpu/drm/drm_fb_helper.c btw? Since the memory range
allocated may not be physically contiguous if a platform uses an IOMMU ?
Asking because I don't really know how these exported values are used...
I just know that is when the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM is enabled.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/shmem-helper: Remove duplicate include
Javier Martinez Canillas writes: > Jiapeng Chong writes: > >> ./drivers/gpu/drm/drm_gem_shmem_helper.c: linux/module.h is included more >> than once. >> >> Reported-by: Abaci Robot >> Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4567 >> Signed-off-by: Jiapeng Chong >> --- > > Reviewed-by: Javier Martinez Canillas > Pushed to drm-misc (drm-misc-next). Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] Revert "drm/udl: Add ARGB8888 as a format"
Douglas Anderson writes: Hello Doug, > This reverts commit 95bf25bb9ed5dedb7fb39f76489f7d6843ab0475. > > Apparently there was a previous discussion about emulation of formats > and it was decided XRGB was the only format to support for legacy > userspace [1]. Remove ARGB. Userspace needs to be fixed to accept > XRGB. > > [1] https://lore.kernel.org/r/60dc7697-d7a0-4bf4-a22e-32f1bbb79...@suse.de > > Signed-off-by: Douglas Anderson > --- > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
Jani Nikula writes: Hello Jani, > Add kconfig to enable -Werror subsystem wide. This is useful for > development and CI to keep the subsystem warning free, while avoiding > issues outside of the subsystem that kernel wide CONFIG_WERROR=y might > hit. > > v2: Don't depend on COMPILE_TEST > > Reviewed-by: Hamza Mahfooz # v1 > Signed-off-by: Jani Nikula > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm/tidss: Use dev_err_probe() over dev_dbg() when failing to probe the port
Andrew Halaney writes: Hello Andrew, > This gets logged out to /sys/kernel/debug/devices_deferred in the > -EPROBE_DEFER case and as an error otherwise. The message here provides > useful information to the user when troubleshooting why their display is > not working in either case, so let's make it output appropriately. > > Signed-off-by: Andrew Halaney > --- > There's definitely more spots in this driver that could be upgraded from > dev_dbg() to something more appropriate, but this one burned me today so > I thought I'd send a patch for it specifically before I forget. > --- Makes sense to me and I agree that's useful to have that information there. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm: Remove drm_num_crtcs() helper
Thierry Reding writes: Hello Thierry, > From: Thierry Reding > > The drm_num_crtcs() helper determines the number of CRTCs by iterating > over the list of CRTCs that have been registered with the mode config. > However, we already keep track of that number in the mode config's > num_crtcs field, so we can simply retrieve the value from that and > remove the extra helper function. > > Signed-off-by: Thierry Reding > --- > drivers/gpu/drm/drm_crtc.c | 15 +-- > 1 file changed, 1 insertion(+), 14 deletions(-) > Indeed. I don't see why this helper would be needed. Your patch makes sense to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] MAINTAINERS: Update drm.git URL
On Mon, Feb 26, 2024 at 5:13 PM Maxime Ripard wrote: > > On Mon, Feb 26, 2024 at 04:43:04PM +0100, Javier Martinez Canillas wrote: > > Maxime Ripard writes: > > > > Hello Maxime, > > > > > Now that the main DRM tree has moved to Gitlab, adjust the MAINTAINERS > > > git trees to reflect the location change. > > > > > > Signed-off-by: Maxime Ripard > > > --- > > > > Are you planning to post another patch to change the entries that point to > > the git://anongit.freedesktop.org/drm/drm-misc tree ? > > Yes, by the time the drm-misc migration will happen (ie, mid-march) > Got it, thanks for the information. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] MAINTAINERS: Update drm.git URL
Maxime Ripard writes: Hello Maxime, > Now that the main DRM tree has moved to Gitlab, adjust the MAINTAINERS > git trees to reflect the location change. > > Signed-off-by: Maxime Ripard > --- Are you planning to post another patch to change the entries that point to the git://anongit.freedesktop.org/drm/drm-misc tree ? Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v2] drm/tegra: Remove existing framebuffer only if we support display
Thomas Zimmermann writes:
> Hi
>
> Am 23.02.24 um 16:03 schrieb Thierry Reding:
>> From: Thierry Reding
>>
>> Tegra DRM doesn't support display on Tegra234 and later, so make sure
>> not to remove any existing framebuffers in that case.
>>
>> v2: - add comments explaining how this situation can come about
>> - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
>>
>> Signed-off-by: Thierry Reding
>
> Makes sense as far as the aperture helpers are concerned.
>
> Reviewed-by: Thomas Zimmermann
>
I believe this is drm-misc-fixes material. Since the tegra DRM will remove
simple{fb,drm} for Tegra234, even when the driver does not support display
on that platform, leaving the system with no display output at all.
Are you going to push this patch or is going to be done by Thierry?
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v2] drm/tegra: Remove existing framebuffer only if we support display
Thierry Reding writes: Hello Thierry, > From: Thierry Reding > > Tegra DRM doesn't support display on Tegra234 and later, so make sure > not to remove any existing framebuffers in that case. > > v2: - add comments explaining how this situation can come about > - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits > > Signed-off-by: Thierry Reding > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev
Thomas Zimmermann writes:
> Hi
>
> Am 20.02.24 um 10:17 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann writes:
>>
>> Hello Thomas,
>>
>>> Framebuffer drivers for devices with dedicated backlight are supposed
>>> to set struct fb_info.bl_dev to the backlight's respective device. Use
>>> the value to match backlight and framebuffer in the backlight core code.
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>> drivers/video/backlight/backlight.c | 9 +++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/backlight.c
>>> b/drivers/video/backlight/backlight.c
>>> index 86e1cdc8e3697..48844a4f28ad3 100644
>>> --- a/drivers/video/backlight/backlight.c
>>> +++ b/drivers/video/backlight/backlight.c
>>> @@ -98,7 +98,8 @@ static int fb_notifier_callback(struct notifier_block
>>> *self,
>>> {
>>> struct backlight_device *bd;
>>> struct fb_event *evdata = data;
>>> - int node = evdata->info->node;
>>> + struct fb_info *info = evdata->info;
>>> + int node = info->node;
>>> int fb_blank = 0;
>>>
>>> /* If we aren't interested in this event, skip it immediately ... */
>>> @@ -110,8 +111,12 @@ static int fb_notifier_callback(struct notifier_block
>>> *self,
>>>
>>> if (!bd->ops)
>>> goto out;
>>> - if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
>>> + else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
>>> goto out;
>>> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
>>> + else if (info->bl_dev && info->bl_dev != bd)
>> If the driver doesn't provide a struct backlight_ops .check_fb callback, I
>> think that having an info->bl_dev should be mandatory ? Or at least maybe
>> there should be a warning if info->bl_dev isn't set ?
>
> bl_dev can only be used for display drivers that set an explicit
> backlight device; otherwise it's NULL. There seem to be systems where
> backlight and display are distinct. And the docs for check_fb say that
> by default the backlight matches against any display. I tried to keep
> this semantics by silently succeeding if neither check_fb nor bl_dev
> have bene set.
>
>>
>> The would be a driver bug, right ?
>
> I assume that some systems create the backlight instance from platform
> data or DT and the display driver has no means of knowing about it.
>
Ok. I thought that in that case a (platform specific) .check_fb callback
would have to be provided then. But if the semantic is that none could be
missing, then I guess is OK to silently succeeding.
I wonder if at least a debug printout is worth it. But maybe a follow-up.
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 10/10] backlight: Add controls_device callback to struct backlight_ops
Thomas Zimmermann writes: > Replace check_fb with controls_device in struct backlight_ops. The > new callback interface takes a Linux device instead of a framebuffer. > Resolves one of the dependencies of backlight.h on fb.h. > > The few drivers that had custom implementations of check_fb can easily > use the framebuffer's Linux device instead. Update them accordingly. > > Signed-off-by: Thomas Zimmermann > --- Personally, I don't like the "control_device" callback name that much. I think that check_device or probe_device would be a better one and easier to understand what it does. But not strong opinion on that, Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 09/10] fbdev/ssd1307fb: Remove struct backlight_ops.check_fb
Thomas Zimmermann writes: > The driver sets struct fb_info.bl_dev to the correct backlight > device. Thus rely on the backlight core code to match backlight > and framebuffer devices, and remove the extra check_fb function > from struct backlight_ops. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 08/10] fbdev/ssd1307fb: Init backlight before registering framebuffer
Thomas Zimmermann writes: > For drivers that support backlight, LCD and fbdev devices, fbdev has > to be initialized last. See documentation for struct fbinfo.bl_dev. > > The backlight name's index number comes from register_framebuffer(), > which now happens after initializing the backlight device. So like > in all other backlight drivers, we now give the backlight device a > generic name without the fbdev index. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 07/10] fbdev/sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb
Thomas Zimmermann writes: > The driver sets struct fb_info.bl_dev to the correct backlight > device. Thus rely on the backlight core code to match backlight > and framebuffer devices, and remove the extra check_fb function > from struct backlight_ops. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb
Thomas Zimmermann writes: > The internal check_fb callback from struct pwm_bl_data is never > implemented. thus the driver's implementation of check_fb always > returns true, which is the backlight core's default if no > implementation has been set. So remove the code from the driver. > > Signed-off-by: Thomas Zimmermann > Cc: "Uwe Kleine-König" > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 05/10] backlight/aat2870-backlight: Remove struct backlight.check_fb
Thomas Zimmermann writes: > The driver's implementation of check_fb always returns true, which > is the default if no implementation has been set. So remove the code > from the driver. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
Thomas Zimmermann writes: > The driver sets struct fb_info.bl_dev to the correct backlight > device. Thus rely on the backlight core code to match backlight > and framebuffer devices, and remove the extra check_fb function > from struct backlight_ops. > > Signed-off-by: Thomas Zimmermann > Cc: "Bruno Prémont" > --- [...] > +#ifdef CONFIG_HID_PICOLCD_BACKLIGHT > + info->bl_dev = data->backlight; > +#endif > + The robot complains about this, I think that you also need to guard against CONFIG_FB_BACKLIGHT being defined. Alternatively, you could include a preparatory patch to fix the HID_PICOLCD_BACKLIGHT config symbol dependencies. Other than that, Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 03/10] hid/hid-picolcd: Fix initialization order
Thomas Zimmermann writes: > For drivers that support backlight, LCD and fbdev devices, fbdev has > to be initialized last. See documentation for struct fbinfo.bl_dev. > > Signed-off-by: Thomas Zimmermann > Cc: "Bruno Prémont" > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
Thomas Zimmermann writes: > The driver sets struct fb_info.bl_dev to the correct backlight > device. Thus rely on the backlight core code to match backlight > and framebuffer devices, and remove the extra check_fb function > from struct backlight_ops. > > Signed-off-by: Thomas Zimmermann > Cc: Robin van der Gracht > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev
Thomas Zimmermann writes:
Hello Thomas,
> Framebuffer drivers for devices with dedicated backlight are supposed
> to set struct fb_info.bl_dev to the backlight's respective device. Use
> the value to match backlight and framebuffer in the backlight core code.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/video/backlight/backlight.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c
> b/drivers/video/backlight/backlight.c
> index 86e1cdc8e3697..48844a4f28ad3 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -98,7 +98,8 @@ static int fb_notifier_callback(struct notifier_block *self,
> {
> struct backlight_device *bd;
> struct fb_event *evdata = data;
> - int node = evdata->info->node;
> + struct fb_info *info = evdata->info;
> + int node = info->node;
> int fb_blank = 0;
>
> /* If we aren't interested in this event, skip it immediately ... */
> @@ -110,8 +111,12 @@ static int fb_notifier_callback(struct notifier_block
> *self,
>
> if (!bd->ops)
> goto out;
> - if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
> + else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
> goto out;
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
> + else if (info->bl_dev && info->bl_dev != bd)
If the driver doesn't provide a struct backlight_ops .check_fb callback, I
think that having an info->bl_dev should be mandatory ? Or at least maybe
there should be a warning if info->bl_dev isn't set ?
The would be a driver bug, right ?
Regardless, the change makes sense to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/dp: Don't attempt AUX transfers when eDP panels are not powered
Doug Anderson writes: Hello Doug, > Hi, > > On Thu, Feb 15, 2024 at 8:53 AM Neil Armstrong > wrote: >> >> Hi Doug, >> >> On 15/02/2024 16:08, Doug Anderson wrote: >> > Hi, >> > >> > On Thu, Feb 15, 2024 at 2:24 AM Jani Nikula wrote: >> >> >> >> On Wed, 14 Feb 2024, Doug Anderson wrote: >> >>> Hi, >> >>> >> >>> On Tue, Feb 13, 2024 at 10:25 PM Hsin-Yi Wang >> >>> wrote: >> >>>> >> >>>> On Wed, Feb 14, 2024 at 2:23 PM Douglas Anderson >> >>>> wrote: >> >>>>> >> >>>>> If an eDP panel is not powered on then any attempts to talk to it over >> >>>>> the DP AUX channel will timeout. Unfortunately these attempts may be >> >>>>> quite slow. Userspace can initiate these attempts either via a >> >>>>> /dev/drm_dp_auxN device or via the created i2c device. >> >>>>> >> >>>>> Making the DP AUX drivers timeout faster is a difficult proposition. >> >>>>> In theory we could just poll the panel's HPD line in the AUX transfer >> >>>>> function and immediately return an error there. However, this is >> >>>>> easier said than done. For one thing, there's no hard requirement to >> >>>>> hook the HPD line up for eDP panels and it's OK to just delay a fixed >> >>>>> amount. For another thing, the HPD line may not be fast to probe. On >> >>>>> parade-ps8640 we need to wait for the bridge chip's firmware to boot >> >>>>> before we can get the HPD line and this is a slow process. >> >>>>> [...] > > The kernel tree we landed on was the v5.15 tree, which is currently > serving all Qualcomm sc7180-based Chromebooks, all Mediatek 8173 > Chromebooks and all Mediatek 8186 Chromebooks. There are also a pile > of x86 Chromebooks running our v5.15 kernel. This code shouldn't > affect them because (unless I'm mistaken) they don't use the two > affected panel drivers. In any case, I haven't heard any screams from > them either. Given my landing plans of "the week of the 26th", this > still gives another 1.5 weeks for any screams to reach my ears. > > ...or are you looking for non-ChromeOS test reports? I'm not sure how > to encourage those. I suppose sometimes folks at Red Hat end up > stumbling over similar panel problems to those of us in ChromeOS. > Maybe +Javier would be interested in providing a Tested-by? > I do have a SC7180 based HP X2 Chromebook and could test your patch (not today but I could do it early next week), although I haven't followed so if you could please let me know what exactly do you want me to verify. AFAIU the problem is that the fwupd daemon tries to scan DP busses even if the panel is turned off and that's what takes a lot of time (due retries), and your patch makes the driver to bail out immediately ? If that's the case, I guess that just starting fwupd daemon with an without your patch while the panel is turned off could be a good test ? > [1] https://crrev.com/c/5277322 > [2] https://crrev.com/c/5277736 > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm: ci: use clk_ignore_unused for apq8016
Dmitry Baryshkov writes:
Hello Dmitry,
> If the ADV7511 bridge driver is compiled as a module, while DRM_MSM is
> built-in, the clk_disable_unused congests with the runtime PM handling
> of the DSI PHY for the clk_prepare_lock(). This causes apq8016 runner to
> fail without completing any jobs ([1]). Drop the BM_CMDLINE which
> duplicate the command line from the .baremetal-igt-arm64 clause and
> enforce the clk_ignore_unused kernelarg instead to make apq8016 runner
> work.
>
Agree that this is the only practical option for the short term...
> [1] https://gitlab.freedesktop.org/drm/msm/-/jobs/54990475
>
> Fixes: 0119c894ab0d ("drm: Add initial ci/ subdirectory")
> Signed-off-by: Dmitry Baryshkov
> ---
Reviewed-by: Javier Martinez Canillas
> drivers/gpu/drm/ci/test.yml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ci/test.yml b/drivers/gpu/drm/ci/test.yml
> index 355b794ef2b1..b9f864e062df 100644
> --- a/drivers/gpu/drm/ci/test.yml
> +++ b/drivers/gpu/drm/ci/test.yml
> @@ -119,7 +119,7 @@ msm:apq8016:
> DRIVER_NAME: msm
> BM_DTB: https://${PIPELINE_ARTIFACTS_BASE}/arm64/apq8016-sbc-usb-host.dtb
> GPU_VERSION: apq8016
> -BM_CMDLINE: "ip=dhcp console=ttyMSM0,115200n8 $BM_KERNEL_EXTRA_ARGS
> root=/dev/nfs rw nfsrootdebug nfsroot=,tcp,nfsvers=4.2 init=/init
> $BM_KERNELARGS"
Maybe add a comment here explaining why the clk_ignore_unused param is
needed ? (basically what you have in your commit message), that way it
could be dropped once the underlying issue is fixed.
> +BM_KERNEL_EXTRA_ARGS: clk_ignore_unused
> RUNNER_TAG: google-freedreno-db410c
>script:
> - ./install/bare-metal/fastboot.sh
> --
> 2.39.2
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] fbdev/sh7760fb: allow modular build
Randy Dunlap writes:
> There is no reason to prohibit sh7760fb from being built as a
> loadable module as suggested by Geert, so change the config symbol
> from bool to tristate to allow that and change the FB dependency as
> needed.
>
> Fixes: f75f71b2c418 ("fbdev/sh7760fb: Depend on FB=y")
> Suggested-by: Geert Uytterhoeven
> Signed-off-by: Randy Dunlap
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: John Paul Adrian Glaubitz
> Cc: Sam Ravnborg
> Cc: Helge Deller
> Cc: linux-fb...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> ---
> drivers/video/fbdev/Kconfig |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] fbdev: Restrict FB_SH_MOBILE_LCDC to SuperH
Geert Uytterhoeven writes:
Hello Geert,
> Since commit f402f7a02af6956d ("staging: board: Remove Armadillo-800-EVA
> board staging code"), there are no more users of the legacy SuperH
> Mobile LCDC framebuffer driver on Renesas ARM platforms. All former
> users on these platforms have been converted to the SH-Mobile DRM
> driver, using DT.
>
> Suggested-by: Arnd Bergmann
> Signed-off-by: Geert Uytterhoeven
> ---
> Commit f402f7a02af6956d is in staging-next (next-20240129 and later).
> ---
Makes senes to me.
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device
Thomas Zimmermann writes:
> Hi
>
> Am 29.01.24 um 12:04 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann writes:
>>
>>> Add screen_info_get_pci_dev() to find the PCI device of an instance
>>> of screen_info. Does nothing on systems without PCI bus.
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>
>> [...]
>>
>>> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
>>> +{
>>> + struct resource res[SCREEN_INFO_MAX_RESOURCES];
>>> + size_t i, numres;
>>> + int ret;
>>> +
>>> + ret = screen_info_resources(si, res, ARRAY_SIZE(res));
>>> + if (ret < 0)
>>> + return ERR_PTR(ret);
>>> + numres = ret;
>>> +
>>
>> I would just drop the ret variable and assign the screen_info_resources()
>> return value to numres. I think that makes the code easier to follow.
>
> The value of ret could be an errno code. We would effectively return
> NULL for errors. And I just noticed that the function docs imply this.
> But NULL is also a valid value if there is no PCI device. I'd prefer to
> keep the errno-pointer around.
>
Yes. I meant making numres an int instead of size_t (SCREEN_INFO_MAX_RESOURCES
is only 3 after all). That way you could just return ERR_PTR(numres) if is < 0.
No strong preference, just think that the code is easier to read in that case.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 8/8] fbdev/efifb: Remove framebuffer relocation tracking
Thomas Zimmermann writes: > If the firmware framebuffer has been reloacted, the sysfb code > fixes the screen_info state before it creates the framebuffer's > platform device. Efifb will automatically receive a screen_info > with updated values. Hence remove the tracking from efifb. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 7/8] firmware/sysfb: Update screen_info for relocated EFI framebuffers
Javier Martinez Canillas writes:
> Thomas Zimmermann writes:
>
>> On ARM PCI systems, the PCI hierarchy might be reconfigured during
>> boot and the firmware framebuffer might move as a result of that.
>> The values in screen_info will then be invalid.
>>
>> Work around this problem by tracking the framebuffer's initial
>> location before it get relocated; then fix the screen_info state
>> between reloaction and creating the firmware framebuffer's device.
>>
>> This functionality has been lifted from efifb. See the commit message
>> of commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that
>> covers the framebuffer") for more information.
>>
>> Signed-off-by: Thomas Zimmermann
>> ---
>
> [...]
>
>> #if defined(CONFIG_PCI)
>
> Shouldn't this be && !defined(CONFIG_X86) ? Or maybe &&
> defined(CONFIG_ARM64), although I don't know if the same
> also applies to other EFI platforms (e.g: CONFIG_RISCV).
>
Answering my own question, the !defined(CONFIG_X86) was dropped in the commit
dcf8f5ce3165 ("drivers/fbdev/efifb: Allow BAR to be moved instead of claiming
it"). The rationale is explained in that commit message:
While this is less likely to occur on x86, given that the firmware's
PCI resource allocation is more likely to be preserved, this is a
worthwhile sanity check to have in place, and so let's remove the
preprocessor conditional that makes it !X86 only.
So it is OK to just guard with #if defined(CONFIG_PCI).
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 7/8] firmware/sysfb: Update screen_info for relocated EFI framebuffers
Thomas Zimmermann writes:
> On ARM PCI systems, the PCI hierarchy might be reconfigured during
> boot and the firmware framebuffer might move as a result of that.
> The values in screen_info will then be invalid.
>
> Work around this problem by tracking the framebuffer's initial
> location before it get relocated; then fix the screen_info state
> between reloaction and creating the firmware framebuffer's device.
>
> This functionality has been lifted from efifb. See the commit message
> of commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that
> covers the framebuffer") for more information.
>
> Signed-off-by: Thomas Zimmermann
> ---
[...]
> #if defined(CONFIG_PCI)
Shouldn't this be && !defined(CONFIG_X86) ? Or maybe &&
defined(CONFIG_ARM64), although I don't know if the same
also applies to other EFI platforms (e.g: CONFIG_RISCV).
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 6/8] fbdev/efifb: Do not track parent device status
Thomas Zimmermann writes: > There will be no EFI framebuffer device for disabled parent devices > and thus we never probe efifb in that case. Hence remove the tracking > code from efifb. > > Signed-off-by: Thomas Zimmermann > --- Nice cleanup. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 5/8] firmware/sysfb: Create firmware device only for enabled PCI devices
Thomas Zimmermann writes:
> Test if the firmware framebuffer's parent PCI device, if any, has
> been enabled. If not, the firmware framebuffer is most likely not
> working. Hence, do not create a device for the firmware framebuffer
> on disabled PCI devices.
>
> So far, efifb tracked the status of the PCI parent device internally
> and did not bind if it was disabled. This patch implements the
> functionality for all firmware framebuffers.
>
> Signed-off-by: Thomas Zimmermann
> ---
[...]
>
> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
> +{
> +#if defined(CONFIG_PCI)
> + /*
> + * TODO: Try to integrate this code into the PCI subsystem
> + */
> + int ret;
> + u16 command;
> +
> + ret = pci_read_config_word(pdev, PCI_COMMAND, );
> + if (ret != PCIBIOS_SUCCESSFUL)
> + return false;
> + if (!(command & PCI_COMMAND_MEMORY))
> + return false;
> + return true;
> +#else
> + // Getting here without PCI support is probably a bug.
> + return false;
Should we warn before return in this case ?
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 4/8] fbdev/efifb: Remove PM for parent device
Thomas Zimmermann writes: > The EFI device has the correct parent device set. This allows Linux > to handle the power management internally. Hence, remove the manual > PM management for the parent device from efifb. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 3/8] firmware/sysfb: Set firmware-framebuffer parent device
Thomas Zimmermann writes:
> Set the firmware framebuffer's parent device, which usually is the
> graphics hardware's physical device. Integrates the framebuffer in
> the Linux device hierarchy and lets Linux handle dependencies among
> devices. For example, the graphics hardware won't be suspended while
> the firmware device is still active.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/firmware/sysfb.c | 11 ++-
> drivers/firmware/sysfb_simplefb.c | 5 -
> include/linux/sysfb.h | 3 ++-
> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index 19706bd2642a..8a42da3f67a9 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -72,6 +73,8 @@ EXPORT_SYMBOL_GPL(sysfb_disable);
> static __init int sysfb_init(void)
> {
> const struct screen_info *si = _info;
> + struct device *parent = NULL;
> + struct pci_dev *pparent;
Maybe pci_parent? It's easier to read than pparent IMO.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device
Thomas Zimmermann writes:
> Add screen_info_get_pci_dev() to find the PCI device of an instance
> of screen_info. Does nothing on systems without PCI bus.
>
> Signed-off-by: Thomas Zimmermann
> ---
[...]
> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
> +{
> + struct resource res[SCREEN_INFO_MAX_RESOURCES];
> + size_t i, numres;
> + int ret;
> +
> + ret = screen_info_resources(si, res, ARRAY_SIZE(res));
> + if (ret < 0)
> + return ERR_PTR(ret);
> + numres = ret;
> +
I would just drop the ret variable and assign the screen_info_resources()
return value to numres. I think that makes the code easier to follow.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 1/8] video: Add helpers for decoding screen_info
Thomas Zimmermann writes:
Hello Thomas,
> The plain values as stored in struct screen_info need to be decoded
> before being used. Add helpers that decode the type of video output
> and the framebuffer I/O aperture.
>
> Old or non-x86 systems may not set the type of video directly, but
> only indicate the presence by storing 0x01 in orig_video_isVGA. The
> decoding logic in screen_info_video_type() takes this into account.
I always disliked how the orig_video_isVGA variable lost its meaning.
> It then follows similar code in vgacon's vgacon_startup() to detect
> the video type from the given values.
>
> A call to screen_info_resources() returns all known resources of the
> given screen_info. The resources' values have been taken from existing
> code in vgacon and vga16fb. These drivers can later be converted to
> use the new interfaces.
>
> Signed-off-by: Thomas Zimmermann
> ---
Thanks for doing this! It's quite useful to have these helpers, since as
you mention the screen_info data decoding is complex and the variables
used to store the video type and modes are confusing / misleading.
Reviewed-by: Javier Martinez Canillas
I just have a few comments below:
> +static inline bool __screen_info_has_ega_gfx(unsigned int mode)
> +{
> + switch (mode) {
> + case 0x0d: /* 320x200-4 */
> + case 0x0e: /* 640x200-4 */
> + case 0x0f: /* 640x350-1 */
> + case 0x10: /* 640x350-4 */
I wonder if makes sense to define some constant macros for these modes? I
know that check_mode_supported() in drivers/video/fbdev/vga16fb.c also use
magic numbers but I believe that it could ease readability.
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static inline bool __screen_info_has_vga_gfx(unsigned int mode)
> +{
> + switch (mode) {
> + case 0x10: /* 640x480-1 */
> + case 0x12: /* 640x480-4 */
> + case 0x13: /* 320-200-8 */
> + case 0x6a: /* 800x600-4 (VESA) */
> + return true;
And same for these.
It can be a follow-up patch though.
[...]
> +int screen_info_resources(const struct screen_info *si, struct resource *r,
> size_t num)
> +{
> + struct resource *pos = r;
> + unsigned int type = screen_info_video_type(si);
> + u64 base, size;
> +
> + switch (type) {
> + case VIDEO_TYPE_MDA:
> + if (num > 0)
> + resource_init_io_named(pos++, 0x3b0, 12, "mda");
I see that drivers/video/fbdev/i740_reg.h has a #define MDA_BASE
0x3B0. Maybe move to a header in include/video along with the other
constants mentioned above ?
> + if (num > 1)
> + resource_init_io_named(pos++, 0x3bf, 0x01, "mda");
> + if (num > 2)
> + resource_init_mem_named(pos++, 0xb, 0x2000, "mda");
Same for these start addresses. I see that are also used by mdacon_startup()
in drivers/video/console/mdacon.c, so some constants defined somewhere might
be useful for that console driver too.
The comment also applies to all the other start addresses, since I see
that those are used by other drivers (i810, vgacon, etc).
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] Revert "drivers/firmware: Move sysfb_init() from device_initcall to subsys_initcall_sync"
Thomas Zimmermann writes:
> This reverts commit 60aebc9559492cea6a9625f514a8041717e3a2e4.
>
> Commit 60aebc9559492cea ("drivers/firmware: Move sysfb_init() from
> device_initcall to subsys_initcall_sync") messes up initialization order
> of the graphics drivers and leads to blank displays on some systems. So
> revert the commit.
>
> To make the display drivers fully independent from initialization
> order requires to track framebuffer memory by device and independently
> from the loaded drivers. The kernel currently lacks the infrastructure
> to do so.
>
> Reported-by: Jaak Ristioja
> Closes:
> https://lore.kernel.org/dri-devel/ZUnNi3q3yB3zZfTl@P70.localdomain/T/#t
> Reported-by: Huacai Chen
> Closes:
> https://lore.kernel.org/dri-devel/20231108024613.2898921-1-chenhua...@loongson.cn/
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10133
> Signed-off-by: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Thorsten Leemhuis
> Cc: Jani Nikula
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/Makefile: Move tiny drivers before native drivers
Thorsten Leemhuis writes:
> On 23.01.24 09:53, Jani Nikula wrote:
>> On Wed, 08 Nov 2023, Thomas Zimmermann wrote:
[...]
>
>>> As you know, there's a platform device that represents the firmware
>>> framebuffer. The firmware drivers, such as simpledrm, bind to it. In
>>> i915 and the other native drivers we remove that platform device, so
>>> that simpledrm does not run.
>>
>> The problem is still not resolved. Another bug report at [1].
>>
>> The commit message here points at 60aebc955949 ("drivers/firmware: Move
>> sysfb_init() from device_initcall to subsys_initcall_sync") as
>> regressing, and Jaak also bisected it (see Closes:).
>>
>> I agree the patch here is just papering over the issue, but lacking a
>> proper fix, for months, a revert would be in order, no?
>>
Yes, I agree that this patch has to be reverted, since as you said the
issue has not been fixed and is causing regressions for multiple users.
>> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/10133
>
> Interesting.
>
> JFYI for those that don't follow this closely: Huacai Chen proposed a
> fix and asked a earlier reporter to test it, but afaics heard nothing back:
>
> https://lore.kernel.org/all/CAAhV-H5eXM7FNzuRCMthAziG_jg75XwQV3grpw=sdyj-9gx...@mail.gmail.com/
>
It's not a fix but a debug patch for the patch author to get more info ?
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] Documentation/gpu: Reference articles on Linux graphics stack
Thomas Zimmermann writes: > Add two articles on LWN about the Linux graphics stack to DRM's > list of external references. The articles document the graphics > output as a whole, including the kernel side. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v1 4/4] backlight: hx8357: Utilise temporary variable for struct device
Andy Shevchenko writes: > We have a temporary variable to keep pointer to struct device. > Utilise it inside the ->probe() implementation. > > Signed-off-by: Andy Shevchenko > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v1 3/4] backlight: hx8357: Make use of dev_err_probe()
Andy Shevchenko writes: > Simplify the error handling in probe function by switching from > dev_err() to dev_err_probe(). > > Signed-off-by: Andy Shevchenko > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer
Andy Shevchenko writes:
> Move OF table near to the user.
>
> While at it, drop comma at terminator entry.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/video/backlight/hx8357.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c
> b/drivers/video/backlight/hx8357.c
> index c7fd10d55c5d..8709d9141cfb 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -566,19 +566,6 @@ static struct lcd_ops hx8357_ops = {
>
> typedef int (*hx8357_init)(struct lcd_device *);
>
> -static const struct of_device_id hx8357_dt_ids[] = {
> - {
> - .compatible = "himax,hx8357",
> - .data = hx8357_lcd_init,
> - },
> - {
> - .compatible = "himax,hx8369",
> - .data = hx8369_lcd_init,
> - },
> - {},
> -};
> -MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
> -
> static int hx8357_probe(struct spi_device *spi)
> {
> struct device *dev = >dev;
> @@ -640,6 +627,19 @@ static int hx8357_probe(struct spi_device *spi)
> return 0;
> }
>
> +static const struct of_device_id hx8357_dt_ids[] = {
> + {
> + .compatible = "himax,hx8357",
> + .data = hx8357_lcd_init,
> + },
> + {
> + .compatible = "himax,hx8369",
> + .data = hx8369_lcd_init,
> + },
> + {}
While at it, maybe add the { /* sentinel */ } convention to the last entry ?
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
Andy Shevchenko writes:
Hello Andy,
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
>
> Include mod_devicetable.h explicitly to replace the dropped of.h
> which included mod_devicetable.h indirectly.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/video/backlight/hx8357.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/hx8357.c
> b/drivers/video/backlight/hx8357.c
> index bf18337ff0c2..c7fd10d55c5d 100644
> --- a/drivers/video/backlight/hx8357.c
> +++ b/drivers/video/backlight/hx8357.c
> @@ -8,9 +8,9 @@
> #include
> #include
> #include
> +#include
> #include
> -#include
> -#include
> +#include
> #include
>
> #define HX8357_NUM_IM_PINS 3
> @@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = {
> .get_power = hx8357_get_power,
> };
>
> +typedef int (*hx8357_init)(struct lcd_device *);
> +
This kind of typedef usage is frowned upon in the Linux coding style [0]
(per my understanding at least) and indeed in my opinion it makes harder
to grep.
[0] https://www.kernel.org/doc/Documentation/process/coding-style.rst
> static const struct of_device_id hx8357_dt_ids[] = {
> {
> .compatible = "himax,hx8357",
> @@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi)
> struct device *dev = >dev;
> struct lcd_device *lcdev;
> struct hx8357_data *lcd;
> - const struct of_device_id *match;
> + hx8357_init init;
> int i, ret;
>
> lcd = devm_kzalloc(>dev, sizeof(*lcd), GFP_KERNEL);
> @@ -597,8 +599,8 @@ static int hx8357_probe(struct spi_device *spi)
>
> lcd->spi = spi;
>
> - match = of_match_device(hx8357_dt_ids, >dev);
> - if (!match || !match->data)
> + init = device_get_match_data(dev);
> + if (!init)
> return -EINVAL;
>
> lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> @@ -627,7 +629,7 @@ static int hx8357_probe(struct spi_device *spi)
>
> hx8357_lcd_reset(lcdev);
>
> - ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
This is what I mean, before it was clear what was stored in match->data.
But after you changes, what is returned by the device_get_match_data()
function is opaque and you need to look at the typedef hx8357_init to
figure that out.
No strong opinion though and I see other drivers doing the same (but no
other driver in drivers/video/backlight).
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH] drm/imagination: Defer probe if requested firmware is not available
Daniel Vetter writes:
> On Tue, Jan 09, 2024 at 02:48:42PM +0100, Javier Martinez Canillas wrote:
>> Daniel Vetter writes:
>>
>> Hello Sima,
>>
>> Thanks for your feedback.
>>
>> > On Tue, Jan 09, 2024 at 01:05:59PM +0100, Javier Martinez Canillas wrote:
>> >> The device is initialized in the driver's probe callback and as part of
>> >> that initialization, the required firmware is loaded. But this fails if
>> >> the driver is built-in and the firmware isn't present in the initramfs:
>> >>
>> >> $ dmesg | grep powervr
>> >> [2.969757] powervr fd0.gpu: Direct firmware load for
>> >> powervr/rogue_33.15.11.3_v1.fw failed with error -2
>> >> [2.979727] powervr fd0.gpu: [drm] *ERROR* failed to load firmware
>> >> powervr/rogue_33.15.11.3_v1.fw (err=-2)
>> >> [2.989885] powervr: probe of fd0.gpu failed with error -2
>> >>
>> >> $ ls -lh /lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
>> >> -rw-r--r-- 1 root root 51K Dec 12 19:00
>> >> /lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
>> >>
>> >> To prevent the probe to fail for this case, let's defer the probe if the
>> >> firmware isn't available. That way, the driver core can retry it and get
>> >> the probe to eventually succeed once the root filesystem has been mounted.
>> >>
>> >> If the firmware is also not present in the root filesystem, then the probe
>> >> will never succeed and the reason listed in the debugfs devices_deferred:
>> >>
>> >> $ cat /sys/kernel/debug/devices_deferred
>> >> fd0.gpu powervr: failed to load firmware
>> >> powervr/rogue_33.15.11.3_v1.fw (err=-517)
>> >>
>> >> Fixes: f99f5f3ea7ef ("drm/imagination: Add GPU ID parsing and firmware
>> >> loading")
>> >> Suggested-by: Maxime Ripard
>> >> Signed-off-by: Javier Martinez Canillas
>> >
>> > Uh that doesn't work.
>> >
>> > Probe is for "I'm missing a struct device" and _only_ that. You can't
>> > assume that probe deferral will defer enough until the initrd shows up.
>> >
>>
>> Fair.
>>
>> > You need to fix this by fixing the initrd to include the required
>> > firmwares. This is what MODULE_FIRMWARE is for, and if your initrd fails
>> > to observe that it's just broken.
>> >
>>
>> Tha's already the case, when is built as a module the initrd (dracut in
>> this particular case) does figure out that the firmware needs to be added
>> but that doesn't work when the DRM driver is built-in. Because dracut is
>> not able to figure out and doesn't even have a powervr.ko info to look at
>> whatever is set by the MODULE_FIRMWARE macro.
>
> Yeah built-in drivers that require firmware don't really work. I'm not
> sure it changed, but a while ago you had to actually include these in the
> kernel image itself (initrd was again too late), and that gives you
> something you can't even ship because it links blobs with gplv2 kernel.
>
Indeed and even let the legal question aside, doing that makes the kernel
too platform specific, even more than building drivers in.
> Maybe that changed and the initramfs is set up early enough now that it's
> sufficient to have it there ...
>
It does work if I force to include the firmware file in the initrd, e.g:
$ cat /etc/dracut.conf.d/firmware.conf
install_items+=" /lib/firmware/powervr/rogue_33.15.11.3_v1.fw "
> Either way I think this needs module/kernel-image build changes so that
> the list of firmware images needed for the kernel itself is dumped
> somewhere, so that dracut can consume it and tdrt. My take at least.
>
That's a very good idea indeed. Dracut just looking at loaded modules and
their respective module info doesn't really work for built-in drivers...
>> > Yes I know as long as you have enough stuff built as module so that there
>> > will be _any_ kind of device probe after the root fs is mounted, this
>> > works, because that triggers a re-probe of everything. But that's the most
>> > kind of fragile fix there is.
>> >
>>
>> Is fragile that's true but on the other hand it does solve the issue in
>> pratice. The whole device probal mechanism is just a best effort anyways.
>>
>> > If you want to change that then I think that needs an official blessing
>> > from Greg KH/device core folks.
>> >
Ok. Let's see what Greg says, I think $SUBJECT is the path of least
resistance and somethi
Re: [PATCH] drm/imagination: Defer probe if requested firmware is not available
Daniel Vetter writes:
Hello Sima,
Thanks for your feedback.
> On Tue, Jan 09, 2024 at 01:05:59PM +0100, Javier Martinez Canillas wrote:
>> The device is initialized in the driver's probe callback and as part of
>> that initialization, the required firmware is loaded. But this fails if
>> the driver is built-in and the firmware isn't present in the initramfs:
>>
>> $ dmesg | grep powervr
>> [2.969757] powervr fd0.gpu: Direct firmware load for
>> powervr/rogue_33.15.11.3_v1.fw failed with error -2
>> [2.979727] powervr fd0.gpu: [drm] *ERROR* failed to load firmware
>> powervr/rogue_33.15.11.3_v1.fw (err=-2)
>> [2.989885] powervr: probe of fd0.gpu failed with error -2
>>
>> $ ls -lh /lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
>> -rw-r--r-- 1 root root 51K Dec 12 19:00
>> /lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
>>
>> To prevent the probe to fail for this case, let's defer the probe if the
>> firmware isn't available. That way, the driver core can retry it and get
>> the probe to eventually succeed once the root filesystem has been mounted.
>>
>> If the firmware is also not present in the root filesystem, then the probe
>> will never succeed and the reason listed in the debugfs devices_deferred:
>>
>> $ cat /sys/kernel/debug/devices_deferred
>> fd0.gpu powervr: failed to load firmware
>> powervr/rogue_33.15.11.3_v1.fw (err=-517)
>>
>> Fixes: f99f5f3ea7ef ("drm/imagination: Add GPU ID parsing and firmware
>> loading")
>> Suggested-by: Maxime Ripard
>> Signed-off-by: Javier Martinez Canillas
>
> Uh that doesn't work.
>
> Probe is for "I'm missing a struct device" and _only_ that. You can't
> assume that probe deferral will defer enough until the initrd shows up.
>
Fair.
> You need to fix this by fixing the initrd to include the required
> firmwares. This is what MODULE_FIRMWARE is for, and if your initrd fails
> to observe that it's just broken.
>
Tha's already the case, when is built as a module the initrd (dracut in
this particular case) does figure out that the firmware needs to be added
but that doesn't work when the DRM driver is built-in. Because dracut is
not able to figure out and doesn't even have a powervr.ko info to look at
whatever is set by the MODULE_FIRMWARE macro.
> Yes I know as long as you have enough stuff built as module so that there
> will be _any_ kind of device probe after the root fs is mounted, this
> works, because that triggers a re-probe of everything. But that's the most
> kind of fragile fix there is.
>
Is fragile that's true but on the other hand it does solve the issue in
pratice. The whole device probal mechanism is just a best effort anyways.
> If you want to change that then I think that needs an official blessing
> from Greg KH/device core folks.
>
I liked this approach due its simplicity but an alternative (and more
complex) solution could be to delay the firmware request and not do it at
probe time.
For example, the following (only barely tested) patch solves the issue for
me as well but it's a bigger change to this driver and wasn't sure if will
be acceptable:
>From c3fb715047a44691412196d8408f2bd495bcd1ed Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas
Date: Tue, 9 Jan 2024 14:47:05 +0100
Subject: [RFC PATCH] drm/imagination: Move PowerVR GPU init to the drivers's
open
callback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Currently the device is initialized in the driver's probe callback. But as
part of this initialization, the required firmware is loaded and this will
fail when the driver is built-in, unless FW is included in the initramfs:
$ dmesg | grep powervr
[2.969757] powervr fd0.gpu: Direct firmware load for
powervr/rogue_33.15.11.3_v1.fw failed with error -2
[2.979727] powervr fd0.gpu: [drm] *ERROR* failed to load firmware
powervr/rogue_33.15.11.3_v1.fw (err=-2)
[2.989885] powervr: probe of fd0.gpu failed with error -2
$ ls -lh /lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
-rw-r--r-- 1 root root 51K Dec 12 19:00
/lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
To prevent this, let's delay the PowerVR GPU-specific initialization until
the render device is opened by user-space. By then, the root filesystem
will be mounted already and the driver able to find the required firmware.
Besides the mentioned problem, it seems more correct to only load firmware
and request the IRQ if the device is opened rather than do these on probe.
Fixes: f99f5f3ea7ef ("drm/imagination: Add GPU ID parsing and firmware loading")
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/imagination/pvr_device.c | 41 +++-
drivers/gpu/drm/imagi
[PATCH] drm/imagination: Defer probe if requested firmware is not available
The device is initialized in the driver's probe callback and as part of
that initialization, the required firmware is loaded. But this fails if
the driver is built-in and the firmware isn't present in the initramfs:
$ dmesg | grep powervr
[2.969757] powervr fd0.gpu: Direct firmware load for
powervr/rogue_33.15.11.3_v1.fw failed with error -2
[2.979727] powervr fd0.gpu: [drm] *ERROR* failed to load firmware
powervr/rogue_33.15.11.3_v1.fw (err=-2)
[2.989885] powervr: probe of fd0.gpu failed with error -2
$ ls -lh /lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
-rw-r--r-- 1 root root 51K Dec 12 19:00
/lib/firmware/powervr/rogue_33.15.11.3_v1.fw.xz
To prevent the probe to fail for this case, let's defer the probe if the
firmware isn't available. That way, the driver core can retry it and get
the probe to eventually succeed once the root filesystem has been mounted.
If the firmware is also not present in the root filesystem, then the probe
will never succeed and the reason listed in the debugfs devices_deferred:
$ cat /sys/kernel/debug/devices_deferred
fd0.gpu powervr: failed to load firmware powervr/rogue_33.15.11.3_v1.fw
(err=-517)
Fixes: f99f5f3ea7ef ("drm/imagination: Add GPU ID parsing and firmware loading")
Suggested-by: Maxime Ripard
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/imagination/pvr_device.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_device.c
b/drivers/gpu/drm/imagination/pvr_device.c
index 1704c0268589..6eda25366431 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -295,8 +295,16 @@ pvr_request_firmware(struct pvr_device *pvr_dev)
*/
err = request_firmware(, filename, pvr_dev->base.dev);
if (err) {
- drm_err(drm_dev, "failed to load firmware %s (err=%d)\n",
- filename, err);
+ /*
+* Defer probe if the firmware is not available yet (e.g: the
driver
+* is built-in and the firmware not present in the initramfs
image).
+*/
+ if (err == -ENOENT)
+ err = -EPROBE_DEFER;
+
+ dev_err_probe(drm_dev->dev, err, "failed to load firmware %s
(err=%d)\n",
+ filename, err);
+
goto err_free_filename;
}
--
2.43.0
Re: [PATCH RFC v2 08/11] ARM: dts: DRA7xx: Add device tree entry for SGX GPU
Andrew Davis writes: > Add SGX GPU device entry to base DRA7x dtsi file. > > Signed-off-by: Andrew Davis > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH RFC v2 07/11] ARM: dts: AM437x: Add device tree entry for SGX GPU
Andrew Davis writes: > Add SGX GPU device entry to base AM437x dtsi file. > > Signed-off-by: Andrew Davis > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
