| Message ID | 20230120120822.2536032-4-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes and cleanup for DRM fbdev emulation and deferred I/O | expand |
Hi Javier,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on linus/master v6.2-rc4 next-20230120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/fbdev-Remove-unused-struct-fb_deferred_io-first_io-field/20230120-201143
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20230120120822.2536032-4-javierm%40redhat.com
patch subject: [PATCH 3/3] drm/fb-helper: Use a per-driver FB deferred I/O handler
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230121/202301211139.vzFLFznY-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/21185713f8ccb3dc34c91fcecff9464c4a8790fa
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Javier-Martinez-Canillas/fbdev-Remove-unused-struct-fb_deferred_io-first_io-field/20230120-201143
git checkout 21185713f8ccb3dc34c91fcecff9464c4a8790fa
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/gpu/drm/drm_modeset_helper.c:24:
>> include/drm/drm_fb_helper.h:215:31: error: field 'fbdefio' has incomplete type
215 | struct fb_deferred_io fbdefio;
| ^~~~~~~
vim +/fbdefio +215 include/drm/drm_fb_helper.h
103
104 /**
105 * struct drm_fb_helper - main structure to emulate fbdev on top of KMS
106 * @fb: Scanout framebuffer object
107 * @dev: DRM device
108 * @funcs: driver callbacks for fb helper
109 * @info: emulated fbdev device info struct
110 * @pseudo_palette: fake palette of 16 colors
111 * @damage_clip: clip rectangle used with deferred_io to accumulate damage to
112 * the screen buffer
113 * @damage_lock: spinlock protecting @damage_clip
114 * @damage_work: worker used to flush the framebuffer
115 * @resume_work: worker used during resume if the console lock is already taken
116 *
117 * This is the main structure used by the fbdev helpers. Drivers supporting
118 * fbdev emulation should embedded this into their overall driver structure.
119 * Drivers must also fill out a &struct drm_fb_helper_funcs with a few
120 * operations.
121 */
122 struct drm_fb_helper {
123 /**
124 * @client:
125 *
126 * DRM client used by the generic fbdev emulation.
127 */
128 struct drm_client_dev client;
129
130 /**
131 * @buffer:
132 *
133 * Framebuffer used by the generic fbdev emulation.
134 */
135 struct drm_client_buffer *buffer;
136
137 struct drm_framebuffer *fb;
138 struct drm_device *dev;
139 const struct drm_fb_helper_funcs *funcs;
140 struct fb_info *info;
141 u32 pseudo_palette[17];
142 struct drm_clip_rect damage_clip;
143 spinlock_t damage_lock;
144 struct work_struct damage_work;
145 struct work_struct resume_work;
146
147 /**
148 * @lock:
149 *
150 * Top-level FBDEV helper lock. This protects all internal data
151 * structures and lists, such as @connector_info and @crtc_info.
152 *
153 * FIXME: fbdev emulation locking is a mess and long term we want to
154 * protect all helper internal state with this lock as well as reduce
155 * core KMS locking as much as possible.
156 */
157 struct mutex lock;
158
159 /**
160 * @kernel_fb_list:
161 *
162 * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.
163 */
164 struct list_head kernel_fb_list;
165
166 /**
167 * @delayed_hotplug:
168 *
169 * A hotplug was received while fbdev wasn't in control of the DRM
170 * device, i.e. another KMS master was active. The output configuration
171 * needs to be reprobe when fbdev is in control again.
172 */
173 bool delayed_hotplug;
174
175 /**
176 * @deferred_setup:
177 *
178 * If no outputs are connected (disconnected or unknown) the FB helper
179 * code will defer setup until at least one of the outputs shows up.
180 * This field keeps track of the status so that setup can be retried
181 * at every hotplug event until it succeeds eventually.
182 *
183 * Protected by @lock.
184 */
185 bool deferred_setup;
186
187 /**
188 * @preferred_bpp:
189 *
190 * Temporary storage for the driver's preferred BPP setting passed to
191 * FB helper initialization. This needs to be tracked so that deferred
192 * FB helper setup can pass this on.
193 *
194 * See also: @deferred_setup
195 */
196 int preferred_bpp;
197
198 /**
199 * @hint_leak_smem_start:
200 *
201 * Hint to the fbdev emulation to store the framebuffer's physical
202 * address in struct &fb_info.fix.smem_start. If the hint is unset,
203 * the smem_start field should always be cleared to zero.
204 */
205 bool hint_leak_smem_start;
206
207 /**
208 * @fbdefio:
209 *
210 * Temporary storage for the driver's FB deferred I/O handler. If the
211 * driver uses the DRM fbdev emulation layer, this is set by the core
212 * to a generic deferred I/O handler if a driver is preferring to use
213 * a shadow buffer.
214 */
> 215 struct fb_deferred_io fbdefio;
216 };
217
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c index b2df8c03594c..bd1f8f28297c 100644 --- a/drivers/gpu/drm/drm_fbdev_generic.c +++ b/drivers/gpu/drm/drm_fbdev_generic.c @@ -171,11 +171,6 @@ static const struct fb_ops drm_fbdev_fb_ops = { .fb_imageblit = drm_fbdev_fb_imageblit, }; -static struct fb_deferred_io drm_fbdev_defio = { - .delay = HZ / 20, - .deferred_io = drm_fb_helper_deferred_io, -}; - /* * This function uses the client API to create a framebuffer backed by a dumb buffer. */ @@ -222,7 +217,11 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper, return -ENOMEM; fbi->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST; - fbi->fbdefio = &drm_fbdev_defio; + /* Set a default deferred I/O handler */ + fb_helper->fbdefio.delay = HZ / 20; + fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io; + + fbi->fbdefio = &fb_helper->fbdefio; ret = fb_deferred_io_init(fbi); if (ret) return ret; diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index f443e1f11654..39b42c1a1954 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -203,6 +203,16 @@ struct drm_fb_helper { * the smem_start field should always be cleared to zero. */ bool hint_leak_smem_start; + + /** + * @fbdefio: + * + * Temporary storage for the driver's FB deferred I/O handler. If the + * driver uses the DRM fbdev emulation layer, this is set by the core + * to a generic deferred I/O handler if a driver is preferring to use + * a shadow buffer. + */ + struct fb_deferred_io fbdefio; }; static inline struct drm_fb_helper *
The DRM fbdev emulation layer sets the struct fb_info .fbdefio field to a struct fb_deferred_io pointer, that is shared across all drivers that use the generic drm_fbdev_generic_setup() helper function. It is a problem because the fbdev core deferred I/O logic assumes that the struct fb_deferred_io data is not shared between devices, and it's stored there state such as the list of pages touched and a mutex that is use to synchronize between the fb_deferred_io_track_page() function that track the dirty pages and fb_deferred_io_work() workqueue handler doing the actual deferred I/O. The latter can lead to the following error, since it may happen that two drivers are probed and then one is removed, which causes the mutex bo be destroyed and not existing anymore by the time the other driver tries to grab it for the fbdev deferred I/O logic: [ 369.756553] ------------[ cut here ]------------ [ 369.756604] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [ 369.756631] WARNING: CPU: 2 PID: 1023 at kernel/locking/mutex.c:582 __mutex_lock+0x348/0x424 [ 369.756744] Modules linked in: nf_conntrack_netbios_ns nf_conntrack_broadcast nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ip v6 nf_defrag_ipv4 ip_set nf_tables nfnetlink qrtr btsdio bluetooth sunrpc brcmfmac snd_soc_hdmi_codec cpufreq_dt cfg80211 vfat fat vc4 rfkill brcmutil raspberrypi_cpufreq i2c_bcm2835 iproc_rng200 bcm2711_thermal snd_soc_core snd_pcm_dmaen gine leds_gpio nvmem_rmem joydev hid_cherry uas usb_storage gpio_raspberrypi_exp v3d snd_pcm raspberrypi_hwmon gpu_sched bcm2835_wdt broadcom bcm_phy_lib snd_timer genet snd mdio_bcm_unimac clk_bcm2711_dvp soundcore drm_display_helper pci e_brcmstb cec ip6_tables ip_tables fuse [ 369.757400] CPU: 2 PID: 1023 Comm: fbtest Not tainted 5.19.0-rc6+ #94 [ 369.757455] Hardware name: raspberrypi,4-model-b Raspberry Pi 4 Model B Rev 1.4/Raspberry Pi 4 Model B Rev 1.4, BIOS 2022.10 10/01/2022 [ 369.757538] pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 369.757596] pc : __mutex_lock+0x348/0x424 [ 369.757635] lr : __mutex_lock+0x348/0x424 [ 369.757672] sp : ffff80000953bb00 [ 369.757703] x29: ffff80000953bb00 x28: ffff17fdc087c000 x27: 0000000000000002 [ 369.757771] x26: ffff17fdc349f9b0 x25: fffffc5ff72e0100 x24: 0000000000000000 [ 369.757838] x23: 0000000000000000 x22: 0000000000000002 x21: ffffa618df636f10 [ 369.757903] x20: ffff80000953bb68 x19: ffffa618e0f18138 x18: 0000000000000001 [ 369.757968] x17: 0000000020000000 x16: 0000000000000002 x15: 0000000000000000 [ 369.758032] x14: 0000000000000000 x13: 284e4f5f4e524157 x12: 5f534b434f4c5f47 [ 369.758097] x11: 00000000ffffdfff x10: ffffa618e0c79f88 x9 : ffffa618de472484 [ 369.758162] x8 : 000000000002ffe8 x7 : c0000000ffffdfff x6 : 00000000000affa8 [ 369.758227] x5 : 0000000000001fff x4 : 0000000000000000 x3 : 0000000000000027 [ 369.758292] x2 : 0000000000000001 x1 : ffff17fdc087c000 x0 : 0000000000000028 [ 369.758357] Call trace: [ 369.758383] __mutex_lock+0x348/0x424 [ 369.758420] mutex_lock_nested+0x4c/0x5c [ 369.758459] fb_deferred_io_mkwrite+0x78/0x1d8 [ 369.758507] do_page_mkwrite+0x5c/0x19c [ 369.758550] wp_page_shared+0x70/0x1a0 [ 369.758590] do_wp_page+0x3d0/0x510 [ 369.758628] handle_pte_fault+0x1c0/0x1e0 [ 369.758670] __handle_mm_fault+0x250/0x380 [ 369.758712] handle_mm_fault+0x17c/0x3a4 [ 369.758753] do_page_fault+0x158/0x530 [ 369.758792] do_mem_abort+0x50/0xa0 [ 369.758831] el0_da+0x78/0x19c [ 369.758864] el0t_64_sync_handler+0xbc/0x150 [ 369.758904] el0t_64_sync+0x190/0x194 [ 369.758942] irq event stamp: 11395 [ 369.758973] hardirqs last enabled at (11395): [<ffffa618de472554>] __up_console_sem+0x74/0x80 [ 369.759042] hardirqs last disabled at (11394): [<ffffa618de47254c>] __up_console_sem+0x6c/0x80 [ 369.760554] softirqs last enabled at (11392): [<ffffa618de330a74>] __do_softirq+0x4c4/0x6b8 [ 369.762060] softirqs last disabled at (11383): [<ffffa618de3c9124>] __irq_exit_rcu+0x104/0x214 [ 369.763564] ---[ end trace 0000000000000000 ]--- Fixes: d536540f304c ("drm/fb-helper: Add generic fbdev emulation .fb_probe function") Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_fbdev_generic.c | 11 +++++------ include/drm/drm_fb_helper.h | 10 ++++++++++ 2 files changed, 15 insertions(+), 6 deletions(-)