strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. Let's use the new 2-argument strscpy() which guarantees NUL-termination on the destination buffer while also simplifying the syntax. Note that strscpy() will not NUL-pad the destination buffer like strncpy() does. However, the NUL-padding behavior of strncpy() is not required since fbdev is already NUL-allocated from au1200fb_drv_probe() -> frameuffer_alloc(), rendering any additional NUL-padding redundant. | p = kzalloc(fb_info_size + size, GFP_KERNEL); Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/video/fbdev/au1200fb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c index 6f20efc663d7..e718fea63662 100644 --- a/drivers/video/fbdev/au1200fb.c +++ b/drivers/video/fbdev/au1200fb.c @@ -1557,7 +1557,7 @@ static int au1200fb_init_fbinfo(struct au1200fb_device *fbdev) return ret; } - strncpy(fbi->fix.id, "AU1200", sizeof(fbi->fix.id)); + strscpy(fbi->fix.id, "AU1200"); fbi->fix.smem_start = fbdev->fb_phys; fbi->fix.smem_len = fbdev->fb_len; fbi->fix.type = FB_TYPE_PACKED_PIXELS; --- base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400 change-id: 20240318-strncpy-drivers-video-fbdev-au1200fb-c-7bc337998096 Best regards, -- Justin Stitt <justinstitt@google.com>
Hi,
On 2024/3/12 23:44, 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 <tzimmermann@suse.de>
> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Zack Rusin <zackr@vmware.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: <stable@vger.kernel.org> # v6.4+
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Zack Rusin <zack.rusin@broadcom.com>
Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Tested-by: Sui Jingfeng <sui.jingfeng@linux.dev>
--
Best regards,
Sui
On 3/18/24 09:11, Roman Smirnov wrote:
> On Fri, 15 Mar 2024 09:44:08 +0100 Helge Deller wrote:
>> On 3/5/24 14:51, Roman Smirnov wrote:
>>> The expression htotal * vtotal can have a zero value on
>>> overflow.
>>
>> I'm not sure if those always results in zero in kernel on overflow.
>> Might be architecture-depended too, but let's assume it
>> can become zero, ....
>>
>>> It is necessary to prevent division by zero like in
>>> fb_var_to_videomode().
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with Svace.
>>>
>>> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> V1 -> V2: Replaced the code of the first version with a check.
>>>
>>> drivers/video/fbdev/core/fbmon.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
>>> index 79e5bfbdd34c..b137590386da 100644
>>> --- a/drivers/video/fbdev/core/fbmon.c
>>> +++ b/drivers/video/fbdev/core/fbmon.c
>>> @@ -1344,7 +1344,7 @@ int fb_videomode_from_videomode(const struct videomode *vm,
>>> vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
>>> vm->vsync_len;
>>> /* prevent division by zero */
>>> - if (htotal && vtotal) {
>>> + if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) {
>>
>> why don't you then simply check for
>> if .. ((htotal * vtotal) == 0) ...
>> instead?
>>
>> Helge
>
> Thomas Zimmermann from the previous discussion said:
>
> On Tue, 5 Mar 2024 11:18:05 +0100 Thomas Zimmerman wrote:
>> Maybe use
>>
>> if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal))
>>
>> for the test. That rules out overflowing multiplication and sets
>> refresh to 0 in such cases.
>
> This prevents overflow, which is also a problematic case.
I don't like adding another division here and I doubt we have
a problem with possible overflow.
So, I suggest to keep it simple, something like:
...
total = htotal * vtotal;
if (total)
fbmode->refresh = vm->pixelclock / total;
else...
Helge
Thomas Zimmermann <tzimmermann@suse.de> 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 <tzimmermann@suse.de> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Thomas Zimmermann <tzimmermann@suse.de> 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 <tzimmermann@suse.de>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Thomas Zimmermann <tzimmermann@suse.de> 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 <tzimmermann@suse.de>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On Wed, Mar 13, 2024 at 04:45:05PM +0100, Thomas Zimmermann wrote:
> Remove the field fb_blank from struct backlight_properties and remove
> all code that still sets or reads it. Backlight blank status is now
> tracked exclusively in struct backlight_properties.state.
>
> The core backlight code keeps the fb_blank and state fields in sync,
> but doesn't do anything else with fb_blank. Several drivers initialize
> fb_blank to FB_BLANK_UNBLANK to enable the backlight. This is already
> the default for the state field. So we can delete the fb_blank code
> from core and drivers and rely on the state field.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Flavio Suligoi <f.suligoi@asem.it>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > any other value in fb_blank. But the field fb_blank in struct > backlight_properties is deprecated and should not be used any > longer. > > Replace the test for fb_blank in omap's backlight code with a > simple boolean parameter and push the test into the update_status > helper. Instead of reading fb_blank directly, decode the backlight > device's status with backlight_is_blank(). > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/video/backlight/omap1_bl.c | 46 ++++++++++++++---------------- > 1 file changed, 21 insertions(+), 25 deletions(-) > > diff --git a/drivers/video/backlight/omap1_bl.c b/drivers/video/backlight/omap1_bl.c > index 84d148f385951..3fd8bbb7f5877 100644 > --- a/drivers/video/backlight/omap1_bl.c > +++ b/drivers/video/backlight/omap1_bl.c > @@ -9,7 +9,6 @@ > #include <linux/kernel.h> > #include <linux/init.h> > #include <linux/platform_device.h> > -#include <linux/fb.h> > #include <linux/backlight.h> > #include <linux/slab.h> > #include <linux/platform_data/omap1_bl.h> > @@ -20,7 +19,7 @@ > #define OMAPBL_MAX_INTENSITY 0xff > > struct omap_backlight { > - int powermode; > + bool power; The new name is hard to read in several of the conditionals below (which were previously "documented" by the comparisons to constants. This boolean effectively controls what we send to omapbl_send_enable(). On that basis I'd rather it was called something like "enabled". > int current_intensity; > > struct device *dev; > @@ -37,21 +36,14 @@ static inline void omapbl_send_enable(int enable) > omap_writeb(enable, OMAP_PWL_CLK_ENABLE); > } > > -static void omapbl_blank(struct omap_backlight *bl, int mode) > +static void omapbl_power(struct omap_backlight *bl, bool on) As above omapbl_enable would be better. > { > - switch (mode) { > - case FB_BLANK_NORMAL: > - case FB_BLANK_VSYNC_SUSPEND: > - case FB_BLANK_HSYNC_SUSPEND: > - case FB_BLANK_POWERDOWN: > - omapbl_send_intensity(0); > - omapbl_send_enable(0); > - break; > - > - case FB_BLANK_UNBLANK: > + if (on) { > omapbl_send_intensity(bl->current_intensity); > omapbl_send_enable(1); > - break; > + } else { > + omapbl_send_intensity(0); > + omapbl_send_enable(0); > } > } > > @@ -61,7 +53,7 @@ static int omapbl_suspend(struct device *dev) > struct backlight_device *bl_dev = dev_get_drvdata(dev); > struct omap_backlight *bl = bl_get_data(bl_dev); > > - omapbl_blank(bl, FB_BLANK_POWERDOWN); > + omapbl_power(bl, false); > return 0; > } > > @@ -70,33 +62,37 @@ static int omapbl_resume(struct device *dev) > struct backlight_device *bl_dev = dev_get_drvdata(dev); > struct omap_backlight *bl = bl_get_data(bl_dev); > > - omapbl_blank(bl, bl->powermode); > + omapbl_power(bl, bl->power); > return 0; > } > #endif > > -static int omapbl_set_power(struct backlight_device *dev, int state) > +static void omapbl_set_power(struct backlight_device *dev, bool on) May also like a new name... > { > struct omap_backlight *bl = bl_get_data(dev); > > - omapbl_blank(bl, state); > - bl->powermode = state; > - > - return 0; > + omapbl_power(bl, on); > + bl->power = on; > } > > static int omapbl_update_status(struct backlight_device *dev) > { > struct omap_backlight *bl = bl_get_data(dev); > + bool power; > > if (bl->current_intensity != dev->props.brightness) { > - if (bl->powermode == FB_BLANK_UNBLANK) > + if (bl->power) > omapbl_send_intensity(dev->props.brightness); > bl->current_intensity = dev->props.brightness; > } > > - if (dev->props.fb_blank != bl->powermode) > - omapbl_set_power(dev, dev->props.fb_blank); > + if (backlight_is_blank(dev)) > + power = false; > + else > + power = true; I'd be happy with: enable = !backlight_is_blank() Daniel.
Hi,
On 28/02/2024 08:35, Tony Lindgren wrote:
> Here are two fixes for omapdrm for missing drm_framebuffer_funcs.dirty
> that needs to be paired with omap_framebuffer_dirty(), and to add
> FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS so things behave as earlier with
> drm_fb_helper_sys_write(). Without these fixes, the console won't update
> for the command mode displays. And likely mmap() using writes can miss
> updates as noted by Thomas.
>
> Regards,
>
> Tony
>
> Changes since v2:
> - Fix cache issue noted by Tomi using custom omap_fbdev_fb_mmap() as
> suggested by Thomas
>
> - Add FB_DMAMEM_HELPERS_DEFERRED Kconfig option and use it for omapdrm
> as noted by Thomas
>
> Changes since v1:
>
> - Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with
> FB_DEFAULT_DEFERRED_OPS as suggested by Thomas
>
> Tony Lindgren (2):
> drm/omapdrm: Fix console by implementing fb_dirty
> drm/omapdrm: Fix console with deferred ops
>
> drivers/gpu/drm/omapdrm/Kconfig | 2 +-
> drivers/gpu/drm/omapdrm/omap_fbdev.c | 40 +++++++++++++++++++++++-----
> drivers/video/fbdev/core/Kconfig | 6 +++++
> include/linux/fb.h | 4 +++
> 4 files changed, 45 insertions(+), 7 deletions(-)
>
Looks fine to me, I'll apply to drm-misc-next.
Tomi
On Wed, Mar 13, 2024 at 04:45:01PM +0100, Thomas Zimmermann wrote:
> The callback set_power in struct omap_backlight_config is not
> implemented anywhere. Remove it from the structure and driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
Hi Dan,
> The "num_levels" variable is used to store error codes from
> device_property_count_u32() so it needs to be signed. This doesn't cause an
> issue at runtime because devm_kcalloc() won't allocate negative sizes.
> However, it's still worth fixing.
>
> Fixes: b54c828bdba9 ("backlight: mp3309c: Make use of device properties")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
I've just tested on my board with the mp3309c chip, all is ok.
Thanks!
Tested-by: Flavio Suligoi <f.suligoi@asem.it>
On Sat, Mar 16, 2024 at 12:45:27PM +0300, Dan Carpenter wrote:
> The "num_levels" variable is used to store error codes from
> device_property_count_u32() so it needs to be signed. This doesn't
> cause an issue at runtime because devm_kcalloc() won't allocate negative
> sizes. However, it's still worth fixing.
>
> Fixes: b54c828bdba9 ("backlight: mp3309c: Make use of device properties")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
On Sat, Mar 16, 2024 at 12:45:27PM +0300, Dan Carpenter wrote:
> The "num_levels" variable is used to store error codes from
> device_property_count_u32() so it needs to be signed. This doesn't
> cause an issue at runtime because devm_kcalloc() won't allocate negative
> sizes. However, it's still worth fixing.
Agree.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
Maxime Ripard <mripard@kernel.org> 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 <tzimmermann@suse.de> 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 <tzimmermann@suse.de> >> > > Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering") >> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> >> > > Cc: Javier Martinez Canillas <javierm@redhat.com> >> > > Cc: Zack Rusin <zackr@vmware.com> >> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> > > Cc: Maxime Ripard <mripard@kernel.org> >> > > Cc: <stable@vger.kernel.org> # 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
On 18/03/2024 08:56, Thomas Zimmermann wrote: > Hi > > Am 13.03.24 um 15:03 schrieb Jocelyn Falempe: >> Hi, >> >> Thanks, it looks good to me. >> >> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com> > > Thanks. Do you still have access to that broken realtime system? I > wonder if this patch makes a difference, as there's now one large > memcpy() less. Hi, Sure, I'll do some latency tests if I can get access to that server again. Best regards, > > Best regards > Thomas >
Hi Thomas,
Thank you for submitting your patch, it looks fine to me.
Reviewed-by: Robin van der Gracht <robin@protonic.nl>
On Wed, 13 Mar 2024 16:45:00 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Replace the use of struct backlight_properties.fb_blank with a
> call to backlight_get_brightness(). The helper implement the same
> logic as the driver's function.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Robin van der Gracht <robin@protonic.nl>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> ---
> drivers/auxdisplay/ht16k33.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/auxdisplay/ht16k33.c
> b/drivers/auxdisplay/ht16k33.c index a90430b7d07ba..83db829b97a5e
> 100644 --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -314,14 +314,9 @@ static int ht16k33_initialize(struct
> ht16k33_priv *priv)
> static int ht16k33_bl_update_status(struct backlight_device *bl)
> {
> - int brightness = bl->props.brightness;
> + int brightness = backlight_get_brightness(bl);
> struct ht16k33_priv *priv = bl_get_data(bl);
>
> - if (bl->props.power != FB_BLANK_UNBLANK ||
> - bl->props.fb_blank != FB_BLANK_UNBLANK ||
> - bl->props.state & BL_CORE_FBBLANK)
> - brightness = 0;
> -
> return ht16k33_brightness_set(priv, brightness);
> }
>
[-- Attachment #1: Type: text/plain, Size: 2608 bytes --] 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 <tzimmermann@suse.de> 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 <tzimmermann@suse.de> > > > Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering") > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Cc: Javier Martinez Canillas <javierm@redhat.com> > > > Cc: Zack Rusin <zackr@vmware.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: <stable@vger.kernel.org> # 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. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
Hi Am 17.03.24 um 13:43 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> 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. Ok. > >> 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 ? I assume that it is supposed to refer to contiguous memory. There's the corresponding field smem_len, which refers to the full memory. > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering") >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Javier Martinez Canillas <javierm@redhat.com> >> Cc: Zack Rusin <zackr@vmware.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: <stable@vger.kernel.org> # 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 <javierm@redhat.com> > > 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. The value of smem_start is used in the fbdev code in only two places : deferred I/O [1] and devfile I/O [2]. For the former, patch 5 of this series adds an additional test. The latter case is only relevant for framebuffers in I/O memory space. But that does not affect fbdev-generic, which has the shadow buffer in main memory. Some driver-internal fbdev code sets smem_length, such as in gma500, but these drivers are special anyway. For what userspace does with this value IDK. But it can't be much, as we've had smem_start cleared for years. Best regards Thomas [1] https://elixir.bootlin.com/linux/v6.8/source/drivers/video/fbdev/core/fb_defio.c#L34 [2] https://elixir.bootlin.com/linux/v6.8/source/drivers/video/fbdev/core/fb_io_fops.c#L143 > > -- > Best regards, > > Javier Martinez Canillas > Core Platforms > Red Hat > > -- -- 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)
On Fri, 15 Mar 2024 09:44:08 +0100 Helge Deller wrote: > On 3/5/24 14:51, Roman Smirnov wrote: > > The expression htotal * vtotal can have a zero value on > > overflow. > > I'm not sure if thos always results in zero in kernel on overflow. > Might be architecture-depended too, but let's assume it > can become zero, .... > > > It is necessary to prevent division by zero like in > > fb_var_to_videomode(). > > > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > > > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > > V1 -> V2: Replaced the code of the first version with a check. > > > > drivers/video/fbdev/core/fbmon.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c > > index 79e5bfbdd34c..b137590386da 100644 > > --- a/drivers/video/fbdev/core/fbmon.c > > +++ b/drivers/video/fbdev/core/fbmon.c > > @@ -1344,7 +1344,7 @@ int fb_videomode_from_videomode(const struct videomode *vm, > > vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch + > > vm->vsync_len; > > /* prevent division by zero */ > > - if (htotal && vtotal) { > > + if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) { > > why don't you then simply check for > if .. ((htotal * vtotal) == 0) ... > instead? > > Helge Thomas Zimmermann from the previous discussion said: On Tue, 5 Mar 2024 11:18:05 +0100 Thomas Zimmerman wrote: > Maybe use > > if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) > > for the test. That rules out overflowing multiplication and sets > refresh to 0 in such cases. This prevents overflow, which is also a problematic case.
Hi Am 18.03.24 um 03:35 schrieb Zack Rusin: > On Tue, Mar 12, 2024 at 11:48 AM Thomas Zimmermann <tzimmermann@suse.de> 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 <tzimmermann@suse.de> >> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering") >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Javier Martinez Canillas <javierm@redhat.com> >> Cc: Zack Rusin <zackr@vmware.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: <stable@vger.kernel.org> # 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. Best regards Thomas > > Reviewed-by: Zack Rusin <zack.rusin@broadcom.com> > > z -- -- 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)
Hi
Am 13.03.24 um 15:03 schrieb Jocelyn Falempe:
> Hi,
>
> Thanks, it looks good to me.
>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Thanks. Do you still have access to that broken realtime system? I
wonder if this patch makes a difference, as there's now one large
memcpy() less.
Best regards
Thomas
--
--
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)
On Tue, Mar 12, 2024 at 11:48 AM Thomas Zimmermann <tzimmermann@suse.de> 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 <tzimmermann@suse.de>
> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Zack Rusin <zackr@vmware.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: <stable@vger.kernel.org> # 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...
Reviewed-by: Zack Rusin <zack.rusin@broadcom.com>
z
On 3/12/24 16:45, Thomas Zimmermann wrote:
> Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports
> damage handling, which is required by repaper. Avoids the overhead of
> fbdev-generic's additional shadow buffering. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> ---
> drivers/gpu/drm/tiny/repaper.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
On 3/12/24 16:45, Thomas Zimmermann wrote:
> Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports
> damage handling, which is required by panel-mipi-dbi. Avoids the
> overhead of fbdev-generic's additional shadow buffering. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> ---
> drivers/gpu/drm/tiny/panel-mipi-dbi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
On 3/12/24 16:45, Thomas Zimmermann wrote:
> Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports
> damage handling, which is required by mi0283qt. Avoids the overhead of
> fbdev-generic's additional shadow buffering. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> ---
> drivers/gpu/drm/tiny/mi0283qt.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Acked-by: Noralf Trønnes <noralf@tronnes.org>