diff mbox series

[3/3] drm/fb-helper: Use a per-driver FB deferred I/O handler

Message ID 20230120120822.2536032-4-javierm@redhat.com
State New
Headers
Series Fixes and cleanup for DRM fbdev emulation and deferred I/O |

Commit Message

Javier Martinez Canillas Jan. 20, 2023, 12:08 p.m. UTC 
  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(-)

Comments

kernel test robot Jan. 21, 2023, 4 a.m. UTC | #1 
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 mbox series

Patch

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 *