[21/21] drm/vmwgfx: Nuke preclose hook
diff mbox

Message ID 1452285413-28459-22-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Jan. 8, 2016, 8:36 p.m. UTC
Again since the drm core takes care of event unlinking/disarming this
is now just needless code.

Cc: Thomas Hellström <thellstrom@vmware.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vc4/vc4_drv.h         |  1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c   | 10 ---------
 drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 38 -----------------------------------
 drivers/gpu/drm/vmwgfx/vmwgfx_fence.h |  2 --
 4 files changed, 51 deletions(-)

Comments

Thomas Hellstrom Jan. 8, 2016, 8:53 p.m. UTC | #1
On 01/08/2016 09:36 PM, Daniel Vetter wrote:
> Again since the drm core takes care of event unlinking/disarming this
> is now just needless code.
>
> Cc: Thomas Hellström <thellstrom@vmware.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Hmm,

IIRC this is actually a list of events that core drm is not aware of
yet. They sit on this list waiting for a fence to pass and are then
transferred to core drm....

/Thomas


> ---
>  drivers/gpu/drm/vc4/vc4_drv.h         |  1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c   | 10 ---------
>  drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 38 -----------------------------------
>  drivers/gpu/drm/vmwgfx/vmwgfx_fence.h |  2 --
>  4 files changed, 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 080865ec2bae..4c734d087d7f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -376,7 +376,6 @@ int vc4_bo_stats_debugfs(struct seq_file *m, void *arg);
>  extern struct platform_driver vc4_crtc_driver;
>  int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
> -void vc4_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file);
>  int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
>  
>  /* vc4_debugfs.c */
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index c49812b80dd0..c04c4d1e2f3e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -971,15 +971,6 @@ static int vmw_driver_unload(struct drm_device *dev)
>  	return 0;
>  }
>  
> -static void vmw_preclose(struct drm_device *dev,
> -			 struct drm_file *file_priv)
> -{
> -	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
> -	struct vmw_private *dev_priv = vmw_priv(dev);
> -
> -	vmw_event_fence_fpriv_gone(dev_priv->fman, &vmw_fp->fence_events);
> -}
> -
>  static void vmw_postclose(struct drm_device *dev,
>  			 struct drm_file *file_priv)
>  {
> @@ -1500,7 +1491,6 @@ static struct drm_driver driver = {
>  	.master_set = vmw_master_set,
>  	.master_drop = vmw_master_drop,
>  	.open = vmw_driver_open,
> -	.preclose = vmw_preclose,
>  	.postclose = vmw_postclose,
>  	.set_busid = drm_pci_set_busid,
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> index e0edf149d9d5..55dc3e4754df 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> @@ -808,44 +808,6 @@ int vmw_fence_obj_unref_ioctl(struct drm_device *dev, void *data,
>  }
>  
>  /**
> - * vmw_event_fence_fpriv_gone - Remove references to struct drm_file objects
> - *
> - * @fman: Pointer to a struct vmw_fence_manager
> - * @event_list: Pointer to linked list of struct vmw_event_fence_action objects
> - * with pointers to a struct drm_file object about to be closed.
> - *
> - * This function removes all pending fence events with references to a
> - * specific struct drm_file object about to be closed. The caller is required
> - * to pass a list of all struct vmw_event_fence_action objects with such
> - * events attached. This function is typically called before the
> - * struct drm_file object's event management is taken down.
> - */
> -void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
> -				struct list_head *event_list)
> -{
> -	struct vmw_event_fence_action *eaction;
> -	struct drm_pending_event *event;
> -	unsigned long irq_flags;
> -
> -	while (1) {
> -		spin_lock_irqsave(&fman->lock, irq_flags);
> -		if (list_empty(event_list))
> -			goto out_unlock;
> -		eaction = list_first_entry(event_list,
> -					   struct vmw_event_fence_action,
> -					   fpriv_head);
> -		list_del_init(&eaction->fpriv_head);
> -		event = eaction->event;
> -		eaction->event = NULL;
> -		spin_unlock_irqrestore(&fman->lock, irq_flags);
> -		event->destroy(event);
> -	}
> -out_unlock:
> -	spin_unlock_irqrestore(&fman->lock, irq_flags);
> -}
> -
> -
> -/**
>   * vmw_event_fence_action_seq_passed
>   *
>   * @action: The struct vmw_fence_action embedded in a struct
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
> index 8be6c29f5eb5..83ae301ee141 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
> @@ -116,8 +116,6 @@ extern int vmw_fence_obj_unref_ioctl(struct drm_device *dev, void *data,
>  				     struct drm_file *file_priv);
>  extern int vmw_fence_event_ioctl(struct drm_device *dev, void *data,
>  				 struct drm_file *file_priv);
> -extern void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
> -				       struct list_head *event_list);
>  extern int vmw_event_fence_action_queue(struct drm_file *filee_priv,
>  					struct vmw_fence_obj *fence,
>  					struct drm_pending_event *event,
Daniel Vetter Jan. 9, 2016, 10:43 a.m. UTC | #2
On Fri, Jan 8, 2016 at 9:53 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> On 01/08/2016 09:36 PM, Daniel Vetter wrote:
>> Again since the drm core takes care of event unlinking/disarming this
>> is now just needless code.
>>
>> Cc: Thomas Hellström <thellstrom@vmware.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Hmm,
>
> IIRC this is actually a list of events that core drm is not aware of
> yet. They sit on this list waiting for a fence to pass and are then
> transferred to core drm....

Yes I know. Earlier patches in the series extract new core functions
to setup/tear down such events and send them out, which is what's
needed to make this trick possible. Exynos similarly uses events
similarly, and is also converted. Same for nouveau it seems, but there
the code doesn't use the reserve/send split, so I'm unclear
how/whether at all it correctly handles this race.
-Daniel
Thomas Hellström (VMware) Jan. 10, 2016, 8:52 p.m. UTC | #3
On 01/09/2016 11:43 AM, Daniel Vetter wrote:
> On Fri, Jan 8, 2016 at 9:53 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> On 01/08/2016 09:36 PM, Daniel Vetter wrote:
>>> Again since the drm core takes care of event unlinking/disarming this
>>> is now just needless code.
>>>
>>> Cc: Thomas Hellström <thellstrom@vmware.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> Hmm,
>>
>> IIRC this is actually a list of events that core drm is not aware of
>> yet. They sit on this list waiting for a fence to pass and are then
>> transferred to core drm....
> Yes I know. Earlier patches in the series extract new core functions
> to setup/tear down such events and send them out, which is what's
> needed to make this trick possible. Exynos similarly uses events
> similarly, and is also converted. Same for nouveau it seems, but there
> the code doesn't use the reserve/send split, so I'm unclear
> how/whether at all it correctly handles this race.
> -Daniel

Ah. Hmm I should've looked more closely at the rest of the series.

In any case, this particular patch leaves, from what I can tell, the
eaction fpriv list intact when it is later freed in postclose, which is
bad. Also each eaction is left with a dangling pointer to a freed
pending event which is also very bad since that pointer will be
dereferenced as soon as the fence's seqno has passed. So as far as i can
tell, this function needs to remain except for the event destruction.

Thinking of it, this must be a problem that is more general problem than
for vmwgfx only, I mean, unless the driver traverses all core pending
event list to find relevant pending events to process, something in the
driver must actually point to the pending event (be it a pointer in the
fence object or, as in the vmwgfx case, a pointer in the fence action
object) and that pointer must somehow be invalidated when the pending
event is freed...

Which also brings up a question, where are the pending events actually
destroyed? I can see they are unlinked in drm_fops.c.

/Thomas
Daniel Vetter Jan. 10, 2016, 9:59 p.m. UTC | #4
On Sun, Jan 10, 2016 at 9:52 PM, Thomas Hellstrom <thomas@shipmail.org> wrote:
> On 01/09/2016 11:43 AM, Daniel Vetter wrote:
>> On Fri, Jan 8, 2016 at 9:53 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>>> On 01/08/2016 09:36 PM, Daniel Vetter wrote:
>>>> Again since the drm core takes care of event unlinking/disarming this
>>>> is now just needless code.
>>>>
>>>> Cc: Thomas Hellström <thellstrom@vmware.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> Hmm,
>>>
>>> IIRC this is actually a list of events that core drm is not aware of
>>> yet. They sit on this list waiting for a fence to pass and are then
>>> transferred to core drm....
>> Yes I know. Earlier patches in the series extract new core functions
>> to setup/tear down such events and send them out, which is what's
>> needed to make this trick possible. Exynos similarly uses events
>> similarly, and is also converted. Same for nouveau it seems, but there
>> the code doesn't use the reserve/send split, so I'm unclear
>> how/whether at all it correctly handles this race.
>> -Daniel
>
> Ah. Hmm I should've looked more closely at the rest of the series.
>
> In any case, this particular patch leaves, from what I can tell, the
> eaction fpriv list intact when it is later freed in postclose, which is
> bad. Also each eaction is left with a dangling pointer to a freed
> pending event which is also very bad since that pointer will be
> dereferenced as soon as the fence's seqno has passed. So as far as i can
> tell, this function needs to remain except for the event destruction.

Oops, I missed that part, it needs to go too. All other drivers look
for events to clean up from some other objects (for the crtc page flip
stuff) or are just outright confused, I didn't check vmwgfx code
carefully enough.

> Thinking of it, this must be a problem that is more general problem than
> for vmwgfx only, I mean, unless the driver traverses all core pending
> event list to find relevant pending events to process, something in the
> driver must actually point to the pending event (be it a pointer in the
> fence object or, as in the vmwgfx case, a pointer in the fence action
> object) and that pointer must somehow be invalidated when the pending
> event is freed...

This is exactly what this patch series attempts to do, by unlinking
events from the fpriv if that disappears. Driver code can still call
drm_even_send&friends (which are all rolled out in this series), they
will simply only free up the event and not try to send it out.

> Which also brings up a question, where are the pending events actually
> destroyed? I can see they are unlinked in drm_fops.c.

When the driver eventually calls drm_send_event. That way fpriv
disappearing is completely transparent to the driver.

I'll resend the vmwgfx patch.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 080865ec2bae..4c734d087d7f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -376,7 +376,6 @@  int vc4_bo_stats_debugfs(struct seq_file *m, void *arg);
 extern struct platform_driver vc4_crtc_driver;
 int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
 void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
-void vc4_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file);
 int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
 
 /* vc4_debugfs.c */
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index c49812b80dd0..c04c4d1e2f3e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -971,15 +971,6 @@  static int vmw_driver_unload(struct drm_device *dev)
 	return 0;
 }
 
-static void vmw_preclose(struct drm_device *dev,
-			 struct drm_file *file_priv)
-{
-	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
-	struct vmw_private *dev_priv = vmw_priv(dev);
-
-	vmw_event_fence_fpriv_gone(dev_priv->fman, &vmw_fp->fence_events);
-}
-
 static void vmw_postclose(struct drm_device *dev,
 			 struct drm_file *file_priv)
 {
@@ -1500,7 +1491,6 @@  static struct drm_driver driver = {
 	.master_set = vmw_master_set,
 	.master_drop = vmw_master_drop,
 	.open = vmw_driver_open,
-	.preclose = vmw_preclose,
 	.postclose = vmw_postclose,
 	.set_busid = drm_pci_set_busid,
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
index e0edf149d9d5..55dc3e4754df 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
@@ -808,44 +808,6 @@  int vmw_fence_obj_unref_ioctl(struct drm_device *dev, void *data,
 }
 
 /**
- * vmw_event_fence_fpriv_gone - Remove references to struct drm_file objects
- *
- * @fman: Pointer to a struct vmw_fence_manager
- * @event_list: Pointer to linked list of struct vmw_event_fence_action objects
- * with pointers to a struct drm_file object about to be closed.
- *
- * This function removes all pending fence events with references to a
- * specific struct drm_file object about to be closed. The caller is required
- * to pass a list of all struct vmw_event_fence_action objects with such
- * events attached. This function is typically called before the
- * struct drm_file object's event management is taken down.
- */
-void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
-				struct list_head *event_list)
-{
-	struct vmw_event_fence_action *eaction;
-	struct drm_pending_event *event;
-	unsigned long irq_flags;
-
-	while (1) {
-		spin_lock_irqsave(&fman->lock, irq_flags);
-		if (list_empty(event_list))
-			goto out_unlock;
-		eaction = list_first_entry(event_list,
-					   struct vmw_event_fence_action,
-					   fpriv_head);
-		list_del_init(&eaction->fpriv_head);
-		event = eaction->event;
-		eaction->event = NULL;
-		spin_unlock_irqrestore(&fman->lock, irq_flags);
-		event->destroy(event);
-	}
-out_unlock:
-	spin_unlock_irqrestore(&fman->lock, irq_flags);
-}
-
-
-/**
  * vmw_event_fence_action_seq_passed
  *
  * @action: The struct vmw_fence_action embedded in a struct
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
index 8be6c29f5eb5..83ae301ee141 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.h
@@ -116,8 +116,6 @@  extern int vmw_fence_obj_unref_ioctl(struct drm_device *dev, void *data,
 				     struct drm_file *file_priv);
 extern int vmw_fence_event_ioctl(struct drm_device *dev, void *data,
 				 struct drm_file *file_priv);
-extern void vmw_event_fence_fpriv_gone(struct vmw_fence_manager *fman,
-				       struct list_head *event_list);
 extern int vmw_event_fence_action_queue(struct drm_file *filee_priv,
 					struct vmw_fence_obj *fence,
 					struct drm_pending_event *event,