[PATCH] [next] i915/gvt: remove hardcoded value on crc32_start calculation
struct gvt_firmware_header has a crc32 member in which all members that come after the that field are used to calculate it. The previous implementation added the value '4' (crc32's u32 size) to calculate the crc32_start offset which came across as a bit cryptic until you take a deeper look at the struct. This patch changes crc32_start offset to the 'version' member which is the first member of the struct gvt_firmware_header after crc32. It's worth mentioning that doing a build before/after this patch results in no binary output differences. Signed-off-by: Paulo Miguel Almeida --- drivers/gpu/drm/i915/gvt/firmware.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gvt/firmware.c b/drivers/gpu/drm/i915/gvt/firmware.c index 54fe442238c6..a683c22d5b64 100644 --- a/drivers/gpu/drm/i915/gvt/firmware.c +++ b/drivers/gpu/drm/i915/gvt/firmware.c @@ -104,7 +104,7 @@ static int expose_firmware_sysfs(struct intel_gvt *gvt) memcpy(p, gvt->firmware.mmio, info->mmio_size); - crc32_start = offsetof(struct gvt_firmware_header, crc32) + 4; + crc32_start = offsetof(struct gvt_firmware_header, version); h->crc32 = crc32_le(0, firmware + crc32_start, size - crc32_start); firmware_attr.size = size; -- 2.25.4
Re: [PATCH v1] drm: rockchip: remove rockchip_drm_framebuffer_init() function
On Wed, 19 Oct 2022 23:35:03 +0200, Johan Jonker wrote: > The function rockchip_drm_framebuffer_init() was in use > in the rockchip_drm_fbdev.c file, but that is now replaced > by a generic fbdev setup. Reduce the image size by > removing the rockchip_drm_framebuffer_init() and sub function > rockchip_fb_alloc() and cleanup the rockchip_drm_fb.h header file. > > > [...] Applied, thanks! [1/1] drm: rockchip: remove rockchip_drm_framebuffer_init() function commit: 4016379301a33e8bd0c2ef5c02f5d7f6a4afece4 Best regards, -- Heiko Stuebner
Re: [PATCH] drm/rockchip: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()
On Wed, 15 Jun 2022 06:26:44 +, Yuan Can wrote: > Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() to avoid > device usage counter leak. > > Applied, thanks! [1/1] drm/rockchip: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync() commit: e3558747ebe15306e6d0b75bd6d211436be4a7d5 I've dropped the lvds part and used the separate patch from your Huawei-collegue Zhang Qilong Best regards, -- Heiko Stuebner
Re: (subset) [PATCH -next 0/2] fix PM usage counter unbalance
On Thu, 22 Sep 2022 21:21:05 +0800, Zhang Qilong wrote: > pm_runtime_get_sync will increment pm usage counter even it failed. > Forgetting to putting operation will result in reference leak here. > We fix it by replacing it with the newest pm_runtime_resume_and_get > to keep usage counter balanced. > > Zhang Qilong (2): > drm/rockchip: vop: fix PM usage counter unbalance in vop ops > drm/rockchip: fix PM usage counter unbalance in poweron > > [...] Applied, thanks! [2/2] drm/rockchip: fix PM usage counter unbalance in poweron commit: 4dba27f1a14592ac4cf71c3bc1cc1fd05dea8015 I've ignored patch1 here, as your Huawei-collegue Yuan Can did sent a different one that catches some more occurences. Best regards, -- Heiko Stuebner
Re: [PATCH] drm/rockchip: dsi: Remove the unused function dsi_update_bits()
On Mon, 17 Oct 2022 16:43:30 +0800, Jiapeng Chong wrote: > The function dsi_update_bits() is defined in the dw-mipi-dsi-rockchip.c > file, but not called elsewhere, so delete this unused function. > > drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c:367:20: warning: unused > function 'dsi_update_bits'. > > https://bugzilla.openanolis.cn/show_bug.cgi?id=2414 > > [...] Applied, thanks! [1/1] drm/rockchip: dsi: Remove the unused function dsi_update_bits() commit: 3daf391fee830f2343cc6b1ba131b1b5115dea1f Best regards, -- Heiko Stuebner
Re: [PATCH] drm/rockchip: vop2: Register Esmart0-win0 as primary plane
On Mon, 26 Sep 2022 10:16:43 +0200, Sascha Hauer wrote: > Esmart0-win0 could serve as primary plane, so mark it as such. On > RK3568 this window will never be used as primary plane, because the > three windows at the beginning of the rk3568_vop_win_data[] array > will be used. On RK3566 however, two of the windows at the beginning > of the rk3568_vop_win_data[] array cannot not be used due to hardware > limitations, so without this patch we end up with CRTCs without primary > planes when multiple VPs are active. > > [...] Applied, thanks! [1/1] drm/rockchip: vop2: Register Esmart0-win0 as primary plane commit: de4a4c8f64021b02aaa8ab21a82fe1f11a17b975 Best regards, -- Heiko Stuebner
Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add prefix for ClockworkPi
Hi Max, On 9/17/22 22:44, Max Fierke wrote: > Add a prefix for Clockwork Tech LLC, known as ClockworkPi. They > produce a number of hobbyist devices, including the ClockworkPi > DevTerm and GameShell. > > Signed-off-by: Max Fierke > Acked-by: Krzysztof Kozlowski > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml > b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index 2f0151e9f6be..64f4b899c40c 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -258,6 +258,8 @@ patternProperties: > description: Cirrus Logic, Inc. >"^cisco,.*": > description: Cisco Systems, Inc. > + "^clockworkpi,.*": > +description: Clockwork Tech LLC The vendor uses "clockwork" as the prefix in their downstream devicetrees[1][2][3], so I would suggest using the same here. I think there is a distinction between "Clockwork" the company and "ClockworkPi" the product. This is what I did for the board devicetree I sent[4]. Regards, Samuel [1]: https://github.com/clockworkpi/DevTerm/blob/main/Code/patch/armbian_build_a04/userpatches/kernel/sunxi-current/kernel_001_dts.patch#L31 [2]: https://github.com/clockworkpi/DevTerm/blob/main/Code/patch/armbian_build_a04/userpatches/kernel/sunxi-current/kernel_001_dts.patch#L127 [3]: https://github.com/clockworkpi/DevTerm/blob/main/Code/patch/armbian_build_a06/patch/kernel-001-a06-dts.patch#L37 [4]: https://lore.kernel.org/lkml/20220815050815.22340-12-sam...@sholland.org/ >"^cloudengines,.*": > description: Cloud Engines, Inc. >"^cnm,.*":
Re: [PATCH] staging: fbtft: Use ARRAY_SIZE() to get argument count
On Sat, 29 Oct 2022, Deepak R Varma wrote:
> On Sat, Oct 29, 2022 at 09:32:50AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Oct 28, 2022 at 07:00:05PM +0530, Deepak R Varma wrote:
> > > The ARRAY_SIZE(foo) macro should be preferred over sizeof operator
> > > based computation such as sizeof(foo)/sizeof(foo[0]) for finding
> > > number of elements in an array. Issue identified using coccicheck.
> > >
> > > Signed-off-by: Deepak R Varma
> > > ---
> > > drivers/staging/fbtft/fbtft.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> > > index 2c2b5f1c1df3..5506a473be91 100644
> > > --- a/drivers/staging/fbtft/fbtft.h
> > > +++ b/drivers/staging/fbtft/fbtft.h
> > > @@ -231,7 +231,7 @@ struct fbtft_par {
> > > bool polarity;
> > > };
> > >
> > > -#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__}) / sizeof(int))
> > > +#define NUMARGS(...) ARRAY_SIZE(((int[]){ __VA_ARGS__ }))
> >
> > Please please please test-build your patches before sending them out.
> > To not do so just wastes reviewer resources :(
>
> Hello Greg,
> I did build the .ko files by making the driver/staging/fbtft/ path. I verified
> .o and .ko files were built.
>
> I did a make clean just now and was again able to rebuild without any errors.
> Please see the attached log file.
>
> Is there something wrong with the way I am firing the build?
The change is in the definition of a macro. The compiler won't help you
in this case unless the macro is actually used in code that is compiled.
Find the uses and check for any nearby ifdefs. For file foo.c you can
also do make foo.i to see the result of reducing ifdef and expanding
macros. Then you can see if the code you changed is actually included in
the build.
julia
Re: [PATCH] staging: fbtft: Use ARRAY_SIZE() to get argument count
On Sat, Oct 29, 2022 at 09:32:50AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 28, 2022 at 07:00:05PM +0530, Deepak R Varma wrote:
> > The ARRAY_SIZE(foo) macro should be preferred over sizeof operator
> > based computation such as sizeof(foo)/sizeof(foo[0]) for finding
> > number of elements in an array. Issue identified using coccicheck.
> >
> > Signed-off-by: Deepak R Varma
> > ---
> > drivers/staging/fbtft/fbtft.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> > index 2c2b5f1c1df3..5506a473be91 100644
> > --- a/drivers/staging/fbtft/fbtft.h
> > +++ b/drivers/staging/fbtft/fbtft.h
> > @@ -231,7 +231,7 @@ struct fbtft_par {
> > bool polarity;
> > };
> >
> > -#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__}) / sizeof(int))
> > +#define NUMARGS(...) ARRAY_SIZE(((int[]){ __VA_ARGS__ }))
>
> Please please please test-build your patches before sending them out.
> To not do so just wastes reviewer resources :(
Hello Greg,
I did build the .ko files by making the driver/staging/fbtft/ path. I verified
.o and .ko files were built.
I did a make clean just now and was again able to rebuild without any errors.
Please see the attached log file.
Is there something wrong with the way I am firing the build?
Thank you,
./drv
>
> thanks,
>
> greg k-h
drv@ubunlion:~/git/kernels/staging$ git show HEAD
commit d7f07b9179428a4b84bcb7a90574edcea8c37fc4 (HEAD -> cocci-patches)
Author: Deepak R Varma
Date: Fri Oct 28 18:40:54 2022 +0530
staging: fbtft: Use ARRAY_SIZE() to get argument count
The ARRAY_SIZE(foo) macro should be preferred over sizeof operator
based computation such as sizeof(foo)/sizeof(foo[0]) for finding
number of elements in an array. Issue identified using coccicheck.
Signed-off-by: Deepak R Varma
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 2c2b5f1c1df3..5506a473be91 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -231,7 +231,7 @@ struct fbtft_par {
bool polarity;
};
-#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__}) / sizeof(int))
+#define NUMARGS(...) ARRAY_SIZE(((int[]){ __VA_ARGS__ }))
#define write_reg(par, ...)\
((par)->fbtftops.write_register(par, NUMARGS(__VA_ARGS__), __VA_ARGS__))
drv@ubunlion:~/git/kernels/staging$ make M=drivers/staging/fbtft/
drv@ubunlion:~/git/kernels/staging$ make M=drivers/staging/fbtft/ clean
CLEAN drivers/staging/fbtft/Module.symvers
drv@ubunlion:~/git/kernels/staging$ make M=drivers/staging/fbtft/
CC [M] drivers/staging/fbtft/fbtft-core.o
CC [M] drivers/staging/fbtft/fbtft-sysfs.o
CC [M] drivers/staging/fbtft/fbtft-bus.o
CC [M] drivers/staging/fbtft/fbtft-io.o
LD [M] drivers/staging/fbtft/fbtft.o
CC [M] drivers/staging/fbtft/fb_agm1264k-fl.o
CC [M] drivers/staging/fbtft/fb_bd663474.o
CC [M] drivers/staging/fbtft/fb_hx8340bn.o
CC [M] drivers/staging/fbtft/fb_hx8347d.o
CC [M] drivers/staging/fbtft/fb_hx8353d.o
CC [M] drivers/staging/fbtft/fb_hx8357d.o
CC [M] drivers/staging/fbtft/fb_ili9163.o
CC [M] drivers/staging/fbtft/fb_ili9320.o
CC [M] drivers/staging/fbtft/fb_ili9325.o
CC [M] drivers/staging/fbtft/fb_ili9340.o
CC [M] drivers/staging/fbtft/fb_ili9341.o
CC [M] drivers/staging/fbtft/fb_ili9481.o
CC [M] drivers/staging/fbtft/fb_ili9486.o
CC [M] drivers/staging/fbtft/fb_pcd8544.o
CC [M] drivers/staging/fbtft/fb_ra8875.o
CC [M] drivers/staging/fbtft/fb_s6d02a1.o
CC [M] drivers/staging/fbtft/fb_s6d1121.o
CC [M] drivers/staging/fbtft/fb_seps525.o
CC [M] drivers/staging/fbtft/fb_sh1106.o
CC [M] drivers/staging/fbtft/fb_ssd1289.o
CC [M] drivers/staging/fbtft/fb_ssd1305.o
CC [M] drivers/staging/fbtft/fb_ssd1306.o
CC [M] drivers/staging/fbtft/fb_ssd1325.o
CC [M] drivers/staging/fbtft/fb_ssd1331.o
CC [M] drivers/staging/fbtft/fb_ssd1351.o
CC [M] drivers/staging/fbtft/fb_st7735r.o
CC [M] drivers/staging/fbtft/fb_st7789v.o
CC [M] drivers/staging/fbtft/fb_tinylcd.o
CC [M] drivers/staging/fbtft/fb_tls8204.o
CC [M] drivers/staging/fbtft/fb_uc1611.o
CC [M] drivers/staging/fbtft/fb_uc1701.o
CC [M] drivers/staging/fbtft/fb_upd161704.o
MODPOST drivers/staging/fbtft/Module.symvers
CC [M] drivers/staging/fbtft/fb_agm1264k-fl.mod.o
LD [M] drivers/staging/fbtft/fb_agm1264k-fl.ko
BTF [M] drivers/staging/fbtft/fb_agm1264k-fl.ko
Skipping BTF generation for drivers/staging/fbtft/fb_agm1264k-fl.ko due to
unavailability of vmlinux
CC [M] drivers/staging/fbtft/fb_bd663474.mod.o
LD [M] drivers/staging/fbtft/fb_bd663474.ko
BTF [M] drivers/staging/fbtft/fb_bd663474.ko
Skipping BTF generation for drivers/staging/fbtft/fb_bd663474.ko due to
unavailability of vmlinux
CC [M] drivers/staging/fbtft/fb_hx8340bn.mod.o
LD [M] drivers/staging/fbtft/fb_hx8340bn.ko
BTF [M]
Re: [PATCH 2/9] drm/i915: Use kmap_local_page() in gem/i915_gem_pyhs.c
On lunedì 17 ottobre 2022 11:37:18 CEST Zhao Liu wrote: > From: Zhao Liu > > The use of kmap_atomic() is being deprecated in favor of > kmap_local_page()[1]. > > The main difference between atomic and local mappings is that local > mappings doesn't disable page faults or preemption. > > In drm/i915/gem/i915_gem_phys.c, the functions > i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys() > don't need to disable pagefaults and preemption for mapping because of > these 2 reasons: > > 1. The flush operation is safe for CPU hotplug when preemption is not > disabled. In drm/i915/gem/i915_gem_object.c, the functions > i915_gem_object_get_pages_phys() and i915_gem_object_put_pages_phys() > calls drm_clflush_virt_range() to use CLFLUSHOPT or WBINVD to flush. > Since CLFLUSHOPT is global on x86 and WBINVD is called on each cpu in > drm_clflush_virt_range(), the flush operation is global and any issue > with cpu's being added or removed can be handled safely. > > 2. Any context switch caused by preemption or sleep (pagefault may > cause sleep) doesn't affect the validity of local mapping. > > Therefore, i915_gem_object_get_pages_phys() and > i915_gem_object_put_pages_phys() are two functions where the use of > kmap_local_page() in place of kmap_atomic() is correctly suited. > > Convert the calls of kmap_atomic() / kunmap_atomic() to > kmap_local_page() / kunmap_local(). > I have here the same questions as in 1/9. > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com > > Suggested-by: Dave Hansen > Suggested-by: Ira Weiny > Suggested-by: Fabio M. De Francesco > Signed-off-by: Zhao Liu > --- > Suggested by credits: > Dave: Referred to his explanation about cache flush. > Ira: Referred to his task document, review comments and explanation about >cache flush. > Fabio: Referred to his boiler plate commit message. > --- > drivers/gpu/drm/i915/gem/i915_gem_phys.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > b/drivers/gpu/drm/i915/gem/i915_gem_phys.c index 0d0e46dae559..d602ba19ecb2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > @@ -66,10 +66,10 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object > *obj) if (IS_ERR(page)) > goto err_st; > > - src = kmap_atomic(page); > + src = kmap_local_page(page); > memcpy(dst, src, PAGE_SIZE); > drm_clflush_virt_range(dst, PAGE_SIZE); > - kunmap_atomic(src); > + kunmap_local(src); Please use memcpy_from_page() instead of open coding mapping + memcpy() + unmapping. > > put_page(page); > dst += PAGE_SIZE; > @@ -114,10 +114,10 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj, > if (IS_ERR(page)) > continue; > > - dst = kmap_atomic(page); > + dst = kmap_local_page(page); > drm_clflush_virt_range(src, PAGE_SIZE); > memcpy(dst, src, PAGE_SIZE); > - kunmap_atomic(dst); > + kunmap_local(dst); For the same reasons said above, memcpy_to_page() should be used here and avoid open coding of three functions. Using those helpers forces you to move drm_clflush_virt_range() out of the mapping / un-mapping region. I may be wrong, however I'm pretty sure that the relative positions of each of those call sites is something that cannot be randomly chosen. Thanks, Fabio > > set_page_dirty(page); > if (obj->mm.madv == I915_MADV_WILLNEED)
Re: [PATCH 1/2] drm/rockchip: dsi: Clean up 'usage_mode' when failing to attach
On Wed, 19 Oct 2022 17:03:48 -0700, Brian Norris wrote: > If we fail to attach the first time (especially: EPROBE_DEFER), we fail > to clean up 'usage_mode', and thus will fail to attach on any subsequent > attempts, with "dsi controller already in use". > > Re-set to DW_DSI_USAGE_IDLE on attach failure. > > This is especially common to hit when enabling asynchronous probe on a > duel-DSI system (such as RK3399 Gru/Scarlet), such that we're more > likely to fail dw_mipi_dsi_rockchip_find_second() the first time. > > [...] Applied, thanks! [1/2] drm/rockchip: dsi: Clean up 'usage_mode' when failing to attach commit: 0be67e0556e469c57100ffe3c90df90abc796f3b [2/2] drm/rockchip: dsi: Force synchronous probe commit: 81e592f86f7afdb76d655e7fbd7803d7b8f985d8 Best regards, -- Heiko Stuebner
Re: [PATCH] drm/rockchip: fix fbdev on non-IOMMU devices
On Thu, 20 Oct 2022 19:12:47 +0100, John Keeping wrote: > When switching to the generic fbdev infrastructure, it was missed that > framebuffers were created with the alloc_kmap parameter to > rockchip_gem_create_object() set to true. The generic infrastructure > calls this via the .dumb_create() driver operation and thus creates a > buffer without an associated kmap. > > alloc_kmap only makes a difference on devices without an IOMMU, but when > it is missing rockchip_gem_prime_vmap() fails and the framebuffer cannot > be used. > > [...] Applied, thanks! [1/1] drm/rockchip: fix fbdev on non-IOMMU devices commit: ab78c74cfc5a3caa2bbb7627cb8f3bca40bb5fb0 Best regards, -- Heiko Stuebner
Re: [PATCH] drm/rockchip: dw_hdmi: filter regulator -EPROBE_DEFER error messages
On Mon, 26 Sep 2022 22:37:52 +0200, Aurelien Jarno wrote: > When the avdd-0v9 or avdd-1v8 supply are not yet available, EPROBE_DEFER > is returned by rockchip_hdmi_parse_dt(). This causes the following error > message to be printed multiple times: > > dwhdmi-rockchip fe0a.hdmi: [drm:dw_hdmi_rockchip_bind [rockchipdrm]] > *ERROR* Unable to parse OF data > > Fix that by not printing the message when rockchip_hdmi_parse_dt() > returns -EPROBE_DEFER. > > [...] Applied, thanks! [1/1] drm/rockchip: dw_hdmi: filter regulator -EPROBE_DEFER error messages commit: bfab00b94bd8569cdb84a6511d6615e6a8104e9c Best regards, -- Heiko Stuebner
Re: [PATCH] drm/rockchip: dsi: Fix VOP selection on SoCs that support it
On Sun, 23 Oct 2022 18:07:47 +0200, Ondrej Jirman wrote: > lcdsel_grf_reg is defined as u32, so "< 0" comaprison is always false, > which breaks VOP selection on eg. RK3399. Compare against 0. > > Applied, thanks! [1/1] drm/rockchip: dsi: Fix VOP selection on SoCs that support it commit: 553c5a429aee26c9cfaf37ae158a8915540270fe Best regards, -- Heiko Stuebner
Re: [PATCH 0/2] drm/rockchip: vop2: fix IOMMU warnings after reenable
On Fri, 28 Oct 2022 11:52:04 +0200, Michael Tretter wrote: > I was observing a lot of warnings that the IOMMU has blocked accessed by the > VOP2 when I disabled and reenabled the VOP2 by shutting down a DRM user space > application (namely Weston) and restarting it. > > The reason for the warnings was that the address of the last framebuffer was > still residing in the read register of Smart0-win0. After enabling the VOP2, > the Smart0-win0 was still enabled and started reading from the address. > > [...] Applied, thanks! [1/2] drm/rockchip: vop2: fix null pointer in plane_atomic_disable commit: 471bf2406c043491b1a8288e5f04bc278f7d7ca1 [2/2] drm/rockchip: vop2: disable planes when disabling the crtc commit: 447fb14bf07905b880c9ed1ea92c53d6dd0649d7 Best regards, -- Heiko Stuebner
Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
On 29/10/2022 01:59, Jessica Zhang wrote:
Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
drm_plane. In addition, add support for setting and getting the values
of these properties.
COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
represents the format of the color fill. Userspace can set enable solid
fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
framebuffer to NULL.
Signed-off-by: Jessica Zhang
Planes report supported formats using the drm_mode_getplane(). You'd
also need to tell userspace, which formats are supported for color fill.
I don't think one supports e.g. YV12.
A bit of generic comment for the discussion (this is an RFC anyway).
Using color_fill/color_fill_format properties sounds simple, but this
might be not generic enough. Limiting color_fill to 32 bits would
prevent anybody from using floating point formats (e.g.
DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
e.g. using 64-bit for the color_fill value, but then this doesn't sound
extensible too much.
So, a question for other hardware maintainers. Do we have hardware that
supports such 'color filled' planes? Do we want to support format
modifiers for filling color/data? Because what I have in mind is closer
to the blob structure, which can then be used for filling the plane:
struct color_fill_blob {
u32 pixel_format;
u64 modifiers4];
u32 pixel_data_size; // fixme: is this necessary?
u8 pixel_data[];
};
And then... This sounds a lot like a custom framebuffer.
So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
flag to the framebuffers, which would e.g. mean that the FB gets stamped
all over the plane. This would also save us from changing if (!fb)
checks all over the drm core.
Another approach might be using a format modifier instead of the FB flag.
What do you think?
--
With best wishes
Dmitry
Re: [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes
On 29/10/2022 01:59, Jessica Zhang wrote:
Initialize and use the color_fill properties for planes in DPU driver. In
addition, relax framebuffer requirements within atomic commit path and
add checks for NULL framebuffers.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++-
2 files changed, 48 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..157698b4f234 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -441,7 +441,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
sspp_idx - SSPP_VIG0,
state->fb ? state->fb->base.id : -1);
- format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
+ if (pstate->base.fb)
+ format =
to_dpu_format(msm_framebuffer_format(pstate->base.fb));
+ else if (state->color_fill && !state->color_fill_format)
+ format = dpu_get_dpu_format(DRM_FORMAT_ABGR);
As I wrote in the review of the earlier patch, this disallows using
black as the plane fill colour. Not to mention that using ABGR should be
explicit rather than implicit.
+ else
+ format = dpu_get_dpu_format(state->color_fill_format);
if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
bg_alpha_enable = true;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 658005f609f4..f3be37e97b64 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -103,7 +103,6 @@ struct dpu_plane {
enum dpu_sspp pipe;
struct dpu_hw_pipe *pipe_hw;
- uint32_t color_fill;
bool is_error;
bool is_rt_pipe;
const struct dpu_mdss_cfg *catalog;
@@ -697,7 +696,10 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
* select fill format to match user property expectation,
* h/w only supports RGB variants
*/
- fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
+ if (plane->state->color_fill && !plane->state->color_fill_format)
+ fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
+ else
+ fmt = dpu_get_dpu_format(plane->state->color_fill_format);
/* update sspp */
if (fmt && pdpu->pipe_hw->ops.setup_solidfill) {
@@ -720,6 +722,10 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
fmt, DPU_SSPP_SOLID_FILL,
pstate->multirect_index);
+ /* skip remaining processing on color fill */
+ if (!plane->state->fb)
+ return 0;
+
if (pdpu->pipe_hw->ops.setup_rects)
pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
_cfg,
@@ -999,12 +1005,21 @@ static int dpu_plane_atomic_check(struct drm_plane
*plane,
dst = drm_plane_state_dest(new_plane_state);
- fb_rect.x2 = new_plane_state->fb->width;
- fb_rect.y2 = new_plane_state->fb->height;
+ if (new_plane_state->fb) {
+ fb_rect.x2 = new_plane_state->fb->width;
+ fb_rect.y2 = new_plane_state->fb->height;
+ }
max_linewidth = pdpu->catalog->caps->max_linewidth;
- fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+ if (new_plane_state->fb) {
+ fmt =
to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+ } else if (new_plane_state->color_fill) {
+ if (new_plane_state->color_fill_format)
+ fmt =
dpu_get_dpu_format(new_plane_state->color_fill_format);
+ else
+ fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
+ }
min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
@@ -1016,7 +1031,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
return -EINVAL;
/* check src bounds */
- } else if (!dpu_plane_validate_src(, _rect, min_src_size)) {
+ } else if (new_plane_state->fb && !dpu_plane_validate_src(,
_rect, min_src_size)) {
DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
DRM_RECT_ARG());
return -E2BIG;
@@ -1084,9 +1099,9 @@ void dpu_plane_flush(struct drm_plane *plane)
if (pdpu->is_error)
/* force white frame with 100% alpha pipe output on error */
_dpu_plane_color_fill(pdpu, 0xFF, 0xFF);
- else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
+ else if (!(plane->state->fb) && plane->state->color_fill)
/* force 100% alpha
Re: [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color
On 29/10/2022 01:59, Jessica Zhang wrote:
Loosen the requirements for atomic and legacy commit so that, in cases
where solid fill planes is enabled (and FB_ID is NULL), the commit can
still go through.
In addition, add framebuffer NULL checks in other areas to account for
FB being NULL when solid fill is enabled.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c| 68 -
drivers/gpu/drm/drm_atomic_helper.c | 34 +--
drivers/gpu/drm/drm_plane.c | 8 ++--
include/drm/drm_atomic_helper.h | 5 ++-
4 files changed, 64 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f197f59f6d99..b576ed221431 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -601,8 +601,10 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
uint32_t num_clips;
int ret;
- /* either *both* CRTC and FB must be set, or neither */
- if (crtc && !fb) {
+ /* When color_fill is disabled,
+* either *both* CRTC and FB must be set, or neither
+*/
+ if (crtc && !fb && !new_plane_state->color_fill) {
Using !color_fill sounds bad. It would disallow using black as the plane
fill color. I think, you need to check color_fill_format instead.
drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
plane->base.id, plane->name);
return -EINVAL;
@@ -626,14 +628,16 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
}
Don't we need to check that the color_fill_format is supported too?
/* Check whether this plane supports the fb pixel format. */
- ret = drm_plane_check_pixel_format(plane, fb->format->format,
- fb->modifier);
- if (ret) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid pixel format %p4cc, modifier
0x%llx\n",
- plane->base.id, plane->name,
- >format->format, fb->modifier);
- return ret;
+ if (fb) {
+ ret = drm_plane_check_pixel_format(plane, fb->format->format,
+ fb->modifier);
+
+ if (ret)
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid pixel format %p4cc,
modifier 0x%llx\n",
+ plane->base.id, plane->name,
+ >format->format, fb->modifier);
+ return ret;
}
/* Give drivers some help against integer overflows */
@@ -649,28 +653,30 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
return -ERANGE;
}
- fb_width = fb->width << 16;
- fb_height = fb->height << 16;
+ if (fb) {
+ fb_width = fb->width << 16;
+ fb_height = fb->height << 16;
- /* Make sure source coordinates are inside the fb. */
- if (new_plane_state->src_w > fb_width ||
- new_plane_state->src_x > fb_width - new_plane_state->src_w ||
- new_plane_state->src_h > fb_height ||
- new_plane_state->src_y > fb_height - new_plane_state->src_h) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid source coordinates "
- "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
- plane->base.id, plane->name,
- new_plane_state->src_w >> 16,
- ((new_plane_state->src_w & 0x) * 15625) >>
10,
- new_plane_state->src_h >> 16,
- ((new_plane_state->src_h & 0x) * 15625) >>
10,
- new_plane_state->src_x >> 16,
- ((new_plane_state->src_x & 0x) * 15625) >>
10,
- new_plane_state->src_y >> 16,
- ((new_plane_state->src_y & 0x) * 15625) >>
10,
- fb->width, fb->height);
- return -ENOSPC;
+ /* Make sure source coordinates are inside the fb. */
+ if (new_plane_state->src_w > fb_width ||
+ new_plane_state->src_x > fb_width - new_plane_state->src_w
||
+ new_plane_state->src_h > fb_height ||
+ new_plane_state->src_y > fb_height -
new_plane_state->src_h) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid source coordinates
"
+ "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb
%ux%u)\n",
+
Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
On 29/10/2022 01:59, Jessica Zhang wrote:
Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
drm_plane. In addition, add support for setting and getting the values
of these properties.
COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
represents the format of the color fill. Userspace can set enable solid
fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
framebuffer to NULL.
I suppose that COLOR_FILL should override framebuffer rather than
requiring that FB is set to NULL. In other words, if color_filL_format
is non-zero, it would make sense to ignore the FB. Then one can use the
color_fill_format property to quickly switch between filled plane and
FB-backed one.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
drivers/gpu/drm/drm_blend.c | 38 +++
include/drm/drm_blend.h | 2 ++
include/drm/drm_plane.h | 28 +++
4 files changed, 76 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..e1664463fca4 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -544,6 +544,10 @@ static int drm_atomic_plane_set_property(struct drm_plane
*plane,
state->src_w = val;
} else if (property == config->prop_src_h) {
state->src_h = val;
+ } else if (property == plane->color_fill_format_property) {
+ state->color_fill_format = val;
+ } else if (property == plane->color_fill_property) {
+ state->color_fill = val;
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {
@@ -616,6 +620,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_w;
} else if (property == config->prop_src_h) {
*val = state->src_h;
+ } else if (property == plane->color_fill_format_property) {
+ *val = state->color_fill_format;
+ } else if (property == plane->color_fill_property) {
+ *val = state->color_fill;
} else if (property == plane->alpha_property) {
*val = state->alpha;
} else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index b4c8cab7158c..b8c2b263fa51 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -616,3 +616,41 @@ int drm_plane_create_blend_mode_property(struct drm_plane
*plane,
return 0;
}
EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+int drm_plane_create_color_fill_property(struct drm_plane *plane)
+{
+ struct drm_property *prop;
+
+ prop = drm_property_create_range(plane->dev, 0, "color_fill",
+0, 0x);
+ if (!prop)
+ return -ENOMEM;
+
+ drm_object_attach_property(>base, prop, 0);
+ plane->color_fill_property = prop;
+
+ if (plane->state)
+ plane->state->color_fill = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_color_fill_property);
+
+int drm_plane_create_color_fill_format_property(struct drm_plane *plane)
+{
+ struct drm_property *prop;
+
+ prop = drm_property_create_range(plane->dev, 0, "color_fill_format",
+0, 0x);
+ if (!prop)
+ return -ENOMEM;
+
+ drm_object_attach_property(>base, prop, 0);
+ plane->color_fill_format_property = prop;
+
+ if (plane->state)
+ plane->state->color_fill_format = 0;
Don't you also need to reset these properties in
__drm_atomic_helper_plane_state_reset() ?
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_color_fill_format_property);
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 88bdfec3bd88..3e96f5e83cce 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -58,4 +58,6 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
struct drm_atomic_state *state);
int drm_plane_create_blend_mode_property(struct drm_plane *plane,
unsigned int supported_modes);
+int drm_plane_create_color_fill_property(struct drm_plane *plane);
+int drm_plane_create_color_fill_format_property(struct drm_plane *plane);
#endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 89ea54652e87..dcbfdb0e1f71 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -116,6 +116,20 @@ struct drm_plane_state {
/** @src_h: height of visible portion of plane (in 16.16) */
uint32_t src_h, src_w;
+ /**
+* @color_fill_format:
+* Format of the color fill
Re: [PATCH 1/9] drm/i915: Use kmap_local_page() in gem/i915_gem_object.c
On lunedì 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> From: Zhao Liu
>
> The use of kmap_atomic() is being deprecated in favor of
> kmap_local_page()[1].
>
> The main difference between atomic and local mappings is that local
> mappings doesn't disable page faults or preemption.
You are right about about page faults which are never disabled by
kmap_local_page(). However kmap_atomic might not disable preemption. It
depends on CONFIG_PREEMPT_RT.
Please refer to how kmap_atomic_prot() works (this function is called by
kmap_atomic() when kernels have HIGHMEM enabled).
>
> There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> need to disable pagefaults and preemption for mapping:
>
> 1. The flush operation is safe for CPU hotplug when preemption is not
> disabled.
I'm confused here. Why are you talking about CPU hotplug?
In any case, developers should never rely on implicit calls of
preempt_disable() for the reasons said above. Therefore, flush operations
should be allowed regardless that kmap_atomic() potential side effect.
> In drm/i915/gem/i915_gem_object.c, the function
> i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range()
If I recall correctly, drm_clflush_virt_range() can always be called with page
faults and preemption enabled. If so, this is enough to say that the
conversion is safe.
Is this code explicitly related to flushing the cache lines before removing /
adding CPUs? If I recall correctly, there are several other reasons behind the
need to issue cache lines flushes. Am I wrong about this?
Can you please say more about what I'm missing here?
> to
> use CLFLUSHOPT or WBINVD to flush. Since CLFLUSHOPT is global on x86
> and WBINVD is called on each cpu in drm_clflush_virt_range(), the flush
> operation is global and any issue with cpu's being added or removed
> can be handled safely.
Again your main concern is about CPU hotplug.
Even if I'm missing something, do we really need all these details about the
inner workings of drm_clflush_virt_range()?
I'm not an expert, so may be that I'm wrong about all I wrote above.
Therefore, can you please elaborate a little more for readers with very little
knowledge of these kinds of things (like me and perhaps others)?
> 2. Any context switch caused by preemption or sleep (pagefault may
> cause sleep) doesn't affect the validity of local mapping.
I'd replace "preemption or sleep" with "preemption and page faults" since
yourself then added that page faults lead to tasks being put to sleep.
> Therefore, i915_gem_object_read_from_page_kmap() is a function where
> the use of kmap_local_page() in place of kmap_atomic() is correctly
> suited.
>
> Convert the calls of kmap_atomic() / kunmap_atomic() to
> kmap_local_page() / kunmap_local().
>
> And remove the redundant variable that stores the address of the mapped
> page since kunmap_local() can accept any pointer within the page.
>
> [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com
>
> Suggested-by: Dave Hansen
> Suggested-by: Ira Weiny
> Suggested-by: Fabio M. De Francesco
> Signed-off-by: Zhao Liu
> ---
> Suggested by credits:
> Dave: Referred to his explanation about cache flush.
> Ira: Referred to his task document, review comments and explanation about
>cache flush.
> Fabio: Referred to his boiler plate commit message.
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> b/drivers/gpu/drm/i915/gem/i915_gem_object.c index
369006c5317f..a0072abed75e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -413,17 +413,15 @@ void __i915_gem_object_invalidate_frontbuffer(struct
> drm_i915_gem_object *obj, static void
> i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64
offset, void
> *dst, int size) {
> - void *src_map;
> void *src_ptr;
>
> - src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >>
PAGE_SHIFT));
> -
> - src_ptr = src_map + offset_in_page(offset);
> + src_ptr = kmap_local_page(i915_gem_object_get_page(obj, offset >>
PAGE_SHIFT))
> + + offset_in_page(offset);
> if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> drm_clflush_virt_range(src_ptr, size);
> memcpy(dst, src_ptr, size);
>
> - kunmap_atomic(src_map);
> + kunmap_local(src_ptr);
> }
>
> static void
The changes look good, but I'd like to better understand the commit message.
Thanks,
Fabio
[PATCH] drm/vc4: hdmi: Fix pointer dereference before check
Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced
the vc4_hdmi_reset_link() function. This function dereferences the
"connector" pointer before checking whether it is NULL or not.
Rework variable assignment to avoid this issue.
Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
Signed-off-by: José Expósito
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 4a73fafca51b..07d058b6afb7 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -319,9 +319,9 @@ static int reset_pipe(struct drm_crtc *crtc,
static int vc4_hdmi_reset_link(struct drm_connector *connector,
struct drm_modeset_acquire_ctx *ctx)
{
- struct drm_device *drm = connector->dev;
- struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
- struct drm_encoder *encoder = _hdmi->encoder.base;
+ struct drm_device *drm;
+ struct vc4_hdmi *vc4_hdmi;
+ struct drm_encoder *encoder;
struct drm_connector_state *conn_state;
struct drm_crtc_state *crtc_state;
struct drm_crtc *crtc;
@@ -332,6 +332,10 @@ static int vc4_hdmi_reset_link(struct drm_connector
*connector,
if (!connector)
return 0;
+ drm = connector->dev;
+ vc4_hdmi = connector_to_vc4_hdmi(connector);
+ encoder = _hdmi->encoder.base;
+
ret = drm_modeset_lock(>mode_config.connection_mutex, ctx);
if (ret)
return ret;
--
2.25.1
Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()
On 10/29/22 10:32 AM, Kees Cook wrote: On Sat, Oct 29, 2022 at 08:55:43AM +0300, Gwan-gyeong Mun wrote: Hi Kees, Hi! :) I've updated to v5 with the last comment of Nathan. Could you please kindly review what more is needed as we move forward with this patch? It looks fine to me -- I assume it'll go via the drm tree? Would you rather I carry the non-drm changes in my tree instead? Hi! Yes, I think it would be better to run this patch on your tree. this patch moves the macro of i915 to overflows.h and modifies one part of drm's driver code, but I think this part can be easily applied when merging into the drm tree. Many thanks, G.G.
Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()
On Sat, Oct 29, 2022 at 08:55:43AM +0300, Gwan-gyeong Mun wrote: > Hi Kees, Hi! :) > I've updated to v5 with the last comment of Nathan. > Could you please kindly review what more is needed as we move forward with > this patch? It looks fine to me -- I assume it'll go via the drm tree? Would you rather I carry the non-drm changes in my tree instead? > -- Kees Cook
Re: [PATCH] staging: fbtft: Use ARRAY_SIZE() to get argument count
On Fri, Oct 28, 2022 at 07:00:05PM +0530, Deepak R Varma wrote:
> The ARRAY_SIZE(foo) macro should be preferred over sizeof operator
> based computation such as sizeof(foo)/sizeof(foo[0]) for finding
> number of elements in an array. Issue identified using coccicheck.
>
> Signed-off-by: Deepak R Varma
> ---
> drivers/staging/fbtft/fbtft.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> index 2c2b5f1c1df3..5506a473be91 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -231,7 +231,7 @@ struct fbtft_par {
> bool polarity;
> };
>
> -#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__}) / sizeof(int))
> +#define NUMARGS(...) ARRAY_SIZE(((int[]){ __VA_ARGS__ }))
Please please please test-build your patches before sending them out.
To not do so just wastes reviewer resources :(
thanks,
greg k-h
Re: [PATCH 0/9] drm/i915: Replace kmap_atomic() with kmap_local_page()
On lunedì 17 ottobre 2022 11:37:16 CEST Zhao Liu wrote: > From: Zhao Liu > > The use of kmap_atomic() is being deprecated in favor of > kmap_local_page()[1]. Some words to explain why kmap_atomic was deprecated won't hurt. Many maintainers and reviewers, and also casual readers might not yet be aware of the reasons behind that deprecation. > In the following patches, we can convert the calls of kmap_atomic() / > kunmap_atomic() to kmap_local_page() / kunmap_local(), which can > instead do the mapping / unmapping regardless of the context. Readers are probably much more interested in what you did in the following patches and why you did it, instead of being informed about what "we can" do. I would suggest something like "The following patches convert the calls to kmap_atomic() to kmap_local_page() [the rest looks OK]". This could also be the place to say something about why we prefer kmap_local_page() to kmap_atomic(). Are you sure that the reasons that motivates your conversions are merely summarized to kmap_local_page() being able to do mappings regardless of context? I think you are missing the real reasons why. What about avoiding the often unwanted side effect of unnecessary page faults disables? > > With kmap_local_page(), the mapping is per thread, CPU local and not > globally visible. No news here. kmap_atomic() is "per thread, CPU local and not glocally visible". I cannot see any difference here between kmap_atomic() and kmap_local_page(). > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com > --- > Zhao Liu (9): > drm/i915: Use kmap_local_page() in gem/i915_gem_object.c > drm/i915: Use kmap_local_page() in gem/i915_gem_pyhs.c > drm/i915: Use kmap_local_page() in gem/i915_gem_shmem.c > drm/i915: Use kmap_local_page() in gem/selftests/huge_pages.c > drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_coherency.c > drm/i915: Use kmap_local_page() in gem/selftests/i915_gem_context.c > drm/i915: Use memcpy_from_page() in gt/uc/intel_uc_fw.c > drm/i915: Use kmap_local_page() in i915_cmd_parser.c > drm/i915: Use kmap_local_page() in gem/i915_gem_execbuffer.c > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++- > drivers/gpu/drm/i915/gem/i915_gem_phys.c | 8 > drivers/gpu/drm/i915/gem/i915_gem_shmem.c| 6 -- > drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 6 +++--- > .../gpu/drm/i915/gem/selftests/i915_gem_coherency.c | 12 > .../gpu/drm/i915/gem/selftests/i915_gem_context.c| 8 > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 5 + > drivers/gpu/drm/i915/i915_cmd_parser.c | 4 ++-- > 9 files changed, 30 insertions(+), 37 deletions(-) Thanks for helping with kmap_atomic() conversions to kmap_local_page(). Fabio
Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion
On 28/10/2022 17.07, Thomas Zimmermann wrote: > In yesterday's discussion on IRC, it was said that several devices > advertise ARGB framebuffers when the hardware actually uses XRGB? Is > there hardware that supports transparent primary planes? ARGB hardware probably exists in the form of embedded systems with preconfigured blending. For example, one could imagine an OSD-type setup where there is a hardware video scaler controlled entirely outside of DRM/KMS (probably by a horrible vendor driver), and the overlay framebuffer is exposed via simpledrm as a dumb memory region, and expects ARGB to work. So ideally, we wouldn't expose XRGB on ARGB systems. But there is this problem: arch/arm64/boot/dts/qcom/msm8998-oneplus-common.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm630-sony-xperia-nile.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm660-xiaomi-lavender.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm850-samsung-w737.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm6350-sony-xperia-lena-pdx213.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm8150-sony-xperia-kumano.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm8250-sony-xperia-edo.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/socionext/uniphier-ld20-akebi96.dts: format = "a8r8g8b8"; I'm pretty sure those phones don't have transparent screens, nor magically put video planes below the firmware framebuffer. If there are 12 device trees for phones in mainline which lie about having alpha support, who knows how many more exist outside? If we stop advertising pretend-XRGB on them, I suspect we're going to break a lot of software... Of course, there is one "correct" solution here: have an actual xrgb->argb conversion helper that just clears the high byte. Then those platforms lying about having alpha and using xrgb from userspace will take a performace hit, but they should arguably just fix their device tree in that case. Maybe this is the way to go in this case? Note that there would be no inverse conversion (no advertising argb on xrgb backends), so that one would be dropped vs. what we have today. This effectively keeps the "xrgb helpers and nothing else" rule while actually supporting it for argb backend framebuffers correctly. Any platforms actually wanting to use argb framebuffers with meaningful alpha should be configuring their userspace to preferentially render directly to argb to avoid the perf hit anyway. - Hector
