diff mbox

[3/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_helper_dirty()

Message ID 20180405195035.24722-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä April 5, 2018, 7:50 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Instead of plane->fb (which we're going to deprecate for atomic drivers)
we need to look at plane->state->fb. The maze of code leading to
vmw_kms_helper_dirty() wasn't particularly clear, but my analysis
concluded that the calls originating from vmw_*_primary_plane_atomic_update()
all pass in the crtc which means we'll never end up in this branch
of the function. All other callers use drm_modeset_lock_all() somewhere
higher up, which means accessing plane->state is safe. We'll toss in
a lockdep assert to catch wrongdoers.

Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Thomas Hellstrom April 5, 2018, 8:15 p.m. UTC | #1 
On 04/05/2018 09:50 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Instead of plane->fb (which we're going to deprecate for atomic drivers)
> we need to look at plane->state->fb. The maze of code leading to
> vmw_kms_helper_dirty() wasn't particularly clear, but my analysis
> concluded that the calls originating from vmw_*_primary_plane_atomic_update()
> all pass in the crtc which means we'll never end up in this branch
> of the function. All other callers use drm_modeset_lock_all() somewhere
> higher up, which means accessing plane->state is safe. We'll toss in
> a lockdep assert to catch wrongdoers.
>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index a2a796b4cc23..5a824125c231 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2326,9 +2326,18 @@ int vmw_kms_helper_dirty(struct vmw_private *dev_priv,
>   	} else {
>   		list_for_each_entry(crtc, &dev_priv->dev->mode_config.crtc_list,
>   				    head) {
> -			if (crtc->primary->fb != &framebuffer->base)
> -				continue;
> -			units[num_units++] = vmw_crtc_to_du(crtc);
> +			struct drm_plane *plane = crtc->primary;
> +
> +			/*
> +			 * vmw_*_primary_plane_atomic_update() pass in the crtc,
> +			 * and so don't end up here. All other callers use
> +			 * drm_modeset_lock_all(), hence we can access the
> +			 * plane state safely.
> +			 */
> +			lockdep_assert_held(&plane->mutex);
> +
I think we can remove the comment (it's a helper, so current users may 
not be future users),
but the lockdep assert should be OK.
> +			if (plane->state->fb != &framebuffer->base)
> +				units[num_units++] = vmw_crtc_to_du(crtc);

This doesn't seem to do what the original code did...

>   		}
>   	}
>   

/Thomas
Ville Syrjälä April 5, 2018, 8:26 p.m. UTC | #2 
On Thu, Apr 05, 2018 at 10:15:57PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 09:50 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Instead of plane->fb (which we're going to deprecate for atomic drivers)
> > we need to look at plane->state->fb. The maze of code leading to
> > vmw_kms_helper_dirty() wasn't particularly clear, but my analysis
> > concluded that the calls originating from vmw_*_primary_plane_atomic_update()
> > all pass in the crtc which means we'll never end up in this branch
> > of the function. All other callers use drm_modeset_lock_all() somewhere
> > higher up, which means accessing plane->state is safe. We'll toss in
> > a lockdep assert to catch wrongdoers.
> >
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Cc: Sinclair Yeh <syeh@vmware.com>
> > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++++++++++++---
> >   1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > index a2a796b4cc23..5a824125c231 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > @@ -2326,9 +2326,18 @@ int vmw_kms_helper_dirty(struct vmw_private *dev_priv,
> >   	} else {
> >   		list_for_each_entry(crtc, &dev_priv->dev->mode_config.crtc_list,
> >   				    head) {
> > -			if (crtc->primary->fb != &framebuffer->base)
> > -				continue;
> > -			units[num_units++] = vmw_crtc_to_du(crtc);
> > +			struct drm_plane *plane = crtc->primary;
> > +
> > +			/*
> > +			 * vmw_*_primary_plane_atomic_update() pass in the crtc,
> > +			 * and so don't end up here. All other callers use
> > +			 * drm_modeset_lock_all(), hence we can access the
> > +			 * plane state safely.
> > +			 */
> > +			lockdep_assert_held(&plane->mutex);
> > +
> I think we can remove the comment (it's a helper, so current users may 
> not be future users),
> but the lockdep assert should be OK.

OK.

> > +			if (plane->state->fb != &framebuffer->base)
> > +				units[num_units++] = vmw_crtc_to_du(crtc);
> 
> This doesn't seem to do what the original code did...

Whoops. Good catch.

> 
> >   		}
> >   	}
> >   
> 
> /Thomas
>
kernel test robot April 6, 2018, 8:51 a.m. UTC | #3 
Hi Ville,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on next-20180405]
[cannot apply to v4.16]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ville-Syrjala/drm-arc-Stop-consulting-plane-fb/20180406-155056
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x010-201813 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from arch/x86/include/asm/bug.h:83:0,
                    from include/linux/bug.h:5,
                    from include/linux/debug_locks.h:7,
                    from include/linux/lockdep.h:28,
                    from include/linux/spinlock_types.h:18,
                    from include/linux/mutex.h:16,
                    from include/linux/kernfs.h:13,
                    from include/linux/sysfs.h:16,
                    from include/linux/kobject.h:20,
                    from include/linux/cdev.h:5,
                    from include/drm/drmP.h:36,
                    from drivers/gpu/drm/vmwgfx/vmwgfx_kms.h:31,
                    from drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:28:
   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c: In function 'vmw_kms_helper_dirty':
>> include/linux/lockdep.h:347:52: error: 'struct drm_modeset_lock' has no member named 'dep_map'
    #define lockdep_is_held(lock)  lock_is_held(&(lock)->dep_map)
                                                       ^
   include/asm-generic/bug.h:112:25: note: in definition of macro 'WARN_ON'
     int __ret_warn_on = !!(condition);    \
                            ^~~~~~~~~
>> include/linux/lockdep.h:373:27: note: in expansion of macro 'lockdep_is_held'
      WARN_ON(debug_locks && !lockdep_is_held(l)); \
                              ^~~~~~~~~~~~~~~
>> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:2337:4: note: in expansion of macro 'lockdep_assert_held'
       lockdep_assert_held(&plane->mutex);
       ^~~~~~~~~~~~~~~~~~~
--
   In file included from arch/x86/include/asm/bug.h:83:0,
                    from include/linux/bug.h:5,
                    from include/linux/debug_locks.h:7,
                    from include/linux/lockdep.h:28,
                    from include/linux/spinlock_types.h:18,
                    from include/linux/mutex.h:16,
                    from include/linux/kernfs.h:13,
                    from include/linux/sysfs.h:16,
                    from include/linux/kobject.h:20,
                    from include/linux/cdev.h:5,
                    from include/drm/drmP.h:36,
                    from drivers/gpu//drm/vmwgfx/vmwgfx_kms.h:31,
                    from drivers/gpu//drm/vmwgfx/vmwgfx_kms.c:28:
   drivers/gpu//drm/vmwgfx/vmwgfx_kms.c: In function 'vmw_kms_helper_dirty':
>> include/linux/lockdep.h:347:52: error: 'struct drm_modeset_lock' has no member named 'dep_map'
    #define lockdep_is_held(lock)  lock_is_held(&(lock)->dep_map)
                                                       ^
   include/asm-generic/bug.h:112:25: note: in definition of macro 'WARN_ON'
     int __ret_warn_on = !!(condition);    \
                            ^~~~~~~~~
>> include/linux/lockdep.h:373:27: note: in expansion of macro 'lockdep_is_held'
      WARN_ON(debug_locks && !lockdep_is_held(l)); \
                              ^~~~~~~~~~~~~~~
   drivers/gpu//drm/vmwgfx/vmwgfx_kms.c:2337:4: note: in expansion of macro 'lockdep_assert_held'
       lockdep_assert_held(&plane->mutex);
       ^~~~~~~~~~~~~~~~~~~

vim +/lockdep_assert_held +2337 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c

  2289	
  2290	/**
  2291	 * vmw_kms_helper_dirty - Helper to build commands and perform actions based
  2292	 * on a set of cliprects and a set of display units.
  2293	 *
  2294	 * @dev_priv: Pointer to a device private structure.
  2295	 * @framebuffer: Pointer to the framebuffer on which to perform the actions.
  2296	 * @clips: A set of struct drm_clip_rect. Either this os @vclips must be NULL.
  2297	 * Cliprects are given in framebuffer coordinates.
  2298	 * @vclips: A set of struct drm_vmw_rect cliprects. Either this or @clips must
  2299	 * be NULL. Cliprects are given in source coordinates.
  2300	 * @dest_x: X coordinate offset for the crtc / destination clip rects.
  2301	 * @dest_y: Y coordinate offset for the crtc / destination clip rects.
  2302	 * @num_clips: Number of cliprects in the @clips or @vclips array.
  2303	 * @increment: Integer with which to increment the clip counter when looping.
  2304	 * Used to skip a predetermined number of clip rects.
  2305	 * @dirty: Closure structure. See the description of struct vmw_kms_dirty.
  2306	 */
  2307	int vmw_kms_helper_dirty(struct vmw_private *dev_priv,
  2308				 struct vmw_framebuffer *framebuffer,
  2309				 const struct drm_clip_rect *clips,
  2310				 const struct drm_vmw_rect *vclips,
  2311				 s32 dest_x, s32 dest_y,
  2312				 int num_clips,
  2313				 int increment,
  2314				 struct vmw_kms_dirty *dirty)
  2315	{
  2316		struct vmw_display_unit *units[VMWGFX_NUM_DISPLAY_UNITS];
  2317		struct drm_crtc *crtc;
  2318		u32 num_units = 0;
  2319		u32 i, k;
  2320	
  2321		dirty->dev_priv = dev_priv;
  2322	
  2323		/* If crtc is passed, no need to iterate over other display units */
  2324		if (dirty->crtc) {
  2325			units[num_units++] = vmw_crtc_to_du(dirty->crtc);
  2326		} else {
  2327			list_for_each_entry(crtc, &dev_priv->dev->mode_config.crtc_list,
  2328					    head) {
  2329				struct drm_plane *plane = crtc->primary;
  2330	
  2331				/*
  2332				 * vmw_*_primary_plane_atomic_update() pass in the crtc,
  2333				 * and so don't end up here. All other callers use
  2334				 * drm_modeset_lock_all(), hence we can access the
  2335				 * plane state safely.
  2336				 */
> 2337				lockdep_assert_held(&plane->mutex);
  2338	
  2339				if (plane->state->fb != &framebuffer->base)
  2340					units[num_units++] = vmw_crtc_to_du(crtc);
  2341			}
  2342		}
  2343	
  2344		for (k = 0; k < num_units; k++) {
  2345			struct vmw_display_unit *unit = units[k];
  2346			s32 crtc_x = unit->crtc.x;
  2347			s32 crtc_y = unit->crtc.y;
  2348			s32 crtc_width = unit->crtc.mode.hdisplay;
  2349			s32 crtc_height = unit->crtc.mode.vdisplay;
  2350			const struct drm_clip_rect *clips_ptr = clips;
  2351			const struct drm_vmw_rect *vclips_ptr = vclips;
  2352	
  2353			dirty->unit = unit;
  2354			if (dirty->fifo_reserve_size > 0) {
  2355				dirty->cmd = vmw_fifo_reserve(dev_priv,
  2356							      dirty->fifo_reserve_size);
  2357				if (!dirty->cmd) {
  2358					DRM_ERROR("Couldn't reserve fifo space "
  2359						  "for dirty blits.\n");
  2360					return -ENOMEM;
  2361				}
  2362				memset(dirty->cmd, 0, dirty->fifo_reserve_size);
  2363			}
  2364			dirty->num_hits = 0;
  2365			for (i = 0; i < num_clips; i++, clips_ptr += increment,
  2366			       vclips_ptr += increment) {
  2367				s32 clip_left;
  2368				s32 clip_top;
  2369	
  2370				/*
  2371				 * Select clip array type. Note that integer type
  2372				 * in @clips is unsigned short, whereas in @vclips
  2373				 * it's 32-bit.
  2374				 */
  2375				if (clips) {
  2376					dirty->fb_x = (s32) clips_ptr->x1;
  2377					dirty->fb_y = (s32) clips_ptr->y1;
  2378					dirty->unit_x2 = (s32) clips_ptr->x2 + dest_x -
  2379						crtc_x;
  2380					dirty->unit_y2 = (s32) clips_ptr->y2 + dest_y -
  2381						crtc_y;
  2382				} else {
  2383					dirty->fb_x = vclips_ptr->x;
  2384					dirty->fb_y = vclips_ptr->y;
  2385					dirty->unit_x2 = dirty->fb_x + vclips_ptr->w +
  2386						dest_x - crtc_x;
  2387					dirty->unit_y2 = dirty->fb_y + vclips_ptr->h +
  2388						dest_y - crtc_y;
  2389				}
  2390	
  2391				dirty->unit_x1 = dirty->fb_x + dest_x - crtc_x;
  2392				dirty->unit_y1 = dirty->fb_y + dest_y - crtc_y;
  2393	
  2394				/* Skip this clip if it's outside the crtc region */
  2395				if (dirty->unit_x1 >= crtc_width ||
  2396				    dirty->unit_y1 >= crtc_height ||
  2397				    dirty->unit_x2 <= 0 || dirty->unit_y2 <= 0)
  2398					continue;
  2399	
  2400				/* Clip right and bottom to crtc limits */
  2401				dirty->unit_x2 = min_t(s32, dirty->unit_x2,
  2402						       crtc_width);
  2403				dirty->unit_y2 = min_t(s32, dirty->unit_y2,
  2404						       crtc_height);
  2405	
  2406				/* Clip left and top to crtc limits */
  2407				clip_left = min_t(s32, dirty->unit_x1, 0);
  2408				clip_top = min_t(s32, dirty->unit_y1, 0);
  2409				dirty->unit_x1 -= clip_left;
  2410				dirty->unit_y1 -= clip_top;
  2411				dirty->fb_x -= clip_left;
  2412				dirty->fb_y -= clip_top;
  2413	
  2414				dirty->clip(dirty);
  2415			}
  2416	
  2417			dirty->fifo_commit(dirty);
  2418		}
  2419	
  2420		return 0;
  2421	}
  2422	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index a2a796b4cc23..5a824125c231 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2326,9 +2326,18 @@  int vmw_kms_helper_dirty(struct vmw_private *dev_priv,
 	} else {
 		list_for_each_entry(crtc, &dev_priv->dev->mode_config.crtc_list,
 				    head) {
-			if (crtc->primary->fb != &framebuffer->base)
-				continue;
-			units[num_units++] = vmw_crtc_to_du(crtc);
+			struct drm_plane *plane = crtc->primary;
+
+			/*
+			 * vmw_*_primary_plane_atomic_update() pass in the crtc,
+			 * and so don't end up here. All other callers use
+			 * drm_modeset_lock_all(), hence we can access the
+			 * plane state safely.
+			 */
+			lockdep_assert_held(&plane->mutex);
+
+			if (plane->state->fb != &framebuffer->base)
+				units[num_units++] = vmw_crtc_to_du(crtc);
 		}
 	}