[PATCH] drm/exynos: do not free pended event at postclose callback

[PATCH] drm/exynos: do not free pended event at postclose callback

[Inki Dae]

These events will be freed by drm_events_release of DRM core so
it doesn't need to free these events in SoC specific driver.
This patch removes relevant codes from Exynos DRM driver.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 57c0e7d..59041d7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -340,20 +340,9 @@ static void exynos_drm_preclose(struct drm_device *dev,
 
 static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
 {
-	struct drm_pending_event *e, *et;
-	unsigned long flags;
-
 	if (!file->driver_priv)
 		return;
 
-	spin_lock_irqsave(&dev->event_lock, flags);
-	/* Release all events handled by page flip handler but not freed. */
-	list_for_each_entry_safe(e, et, &file->event_list, link) {
-		list_del(&e->link);
-		e->destroy(e);
-	}
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
 	kfree(file->driver_priv);
 	file->driver_priv = NULL;
 }
-- 
1.9.1

[PATCH v3] drm/exynos: fix kernel panic issue at drm releasing

[Inki Dae]

This patch fixes a kernel panic issue which happened
when drm driver is closed while modetest.

This issue could be reproduced easily by launching modetest
with page flip repeatedly.

The reason is that invalid drm_file object could be accessed by
send_vblank_event function when finishing page flip if the drm_file
object was removed by drm_release and there was a pended page
flip event which was already committed to hardware.

So this patch makes the pended page flip event to be cancelled by
preclose callback which is called at front of drm_release function.

Changelog v2:
- free vblank event objects belonging to the request process,
  increment event space and decrease pending_update when cancelling
  the event

Changelog v3:
- initialize only device specific things. Each page flip event object
  is created by DRM core so DRM core should release the object including
  incrementing event space.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 15 +++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_crtc.h |  3 +++
 drivers/gpu/drm/exynos/exynos_drm_drv.c  |  5 +++++
 3 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index cd94981..8b503a0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -226,3 +226,18 @@ void exynos_drm_crtc_te_handler(struct drm_crtc *crtc)
 	if (exynos_crtc->ops->te_handler)
 		exynos_crtc->ops->te_handler(exynos_crtc);
 }
+
+void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc)
+{
+	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+	struct drm_pending_vblank_event *e;
+	unsigned long flags;
+
+	spin_lock_irqsave(&crtc->dev->event_lock, flags);
+	e = exynos_crtc->event;
+	if (e) {
+		exynos_crtc->event = NULL;
+		atomic_dec(&exynos_crtc->pending_update);
+	}
+	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
index 6a581a8..b4def6e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
@@ -40,4 +40,7 @@ int exynos_drm_crtc_get_pipe_from_type(struct drm_device *drm_dev,
  */
 void exynos_drm_crtc_te_handler(struct drm_crtc *crtc);
 
+/* This function cancels a page flip request. */
+void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc);
+
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 9756797a..57c0e7d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -330,7 +330,12 @@ err_file_priv_free:
 static void exynos_drm_preclose(struct drm_device *dev,
 					struct drm_file *file)
 {
+	struct drm_crtc *crtc;
+
 	exynos_drm_subdrv_close(dev, file);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		exynos_drm_crtc_cancel_page_flip(crtc);
 }
 
 static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
-- 
1.9.1

Re: [PATCH v3] drm/exynos: fix kernel panic issue at drm releasing

[Daniel Vetter]

On Fri, Jan 8, 2016 at 9:46 AM, Inki Dae <inki.dae@samsung.com> wrote:
> This patch fixes a kernel panic issue which happened
> when drm driver is closed while modetest.
>
> This issue could be reproduced easily by launching modetest
> with page flip repeatedly.
>
> The reason is that invalid drm_file object could be accessed by
> send_vblank_event function when finishing page flip if the drm_file
> object was removed by drm_release and there was a pended page
> flip event which was already committed to hardware.
>
> So this patch makes the pended page flip event to be cancelled by
> preclose callback which is called at front of drm_release function.
>
> Changelog v2:
> - free vblank event objects belonging to the request process,
>   increment event space and decrease pending_update when cancelling
>   the event
>
> Changelog v3:
> - initialize only device specific things. Each page flip event object
>   is created by DRM core so DRM core should release the object including
>   incrementing event space.
>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Reviewed-by: Daniel Stone <daniels@collabora.com>

Note that after my drm_event cleanup series has landed (aiming for
early 4.6) you can/should revert this again. But it's too big as a
bugfix this late in 4.5. Ack/tested-by from exynos on that series
would be great.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3] drm/exynos: fix kernel panic issue at drm releasing

[Daniel Stone]

Hi Inki,

On 8 January 2016 at 08:46, Inki Dae <inki.dae@samsung.com> wrote:
> Changelog v3:
> - initialize only device specific things. Each page flip event object
>   is created by DRM core so DRM core should release the object including
>   incrementing event space.

I'm a bit confused here; we no longer call event->base.destroy(),
because you say that the DRM core should release it. But how does the
DRM core know to release the event? From the core point of view, the
event disappears into the driver, and it is no longer tracked.

As Daniel says though, later versions handle all this in the core in a
much more clean way, so we can remove these from the drivers then.

Cheers,
Daniel

Re: [PATCH v3] drm/exynos: fix kernel panic issue at drm releasing

[Inki Dae]

Hi Daniel,

2016년 01월 12일 04:00에 Daniel Stone 이(가) 쓴 글:
> Hi Inki,
> 
> On 8 January 2016 at 08:46, Inki Dae <inki.dae@samsung.com> wrote:
>> Changelog v3:
>> - initialize only device specific things. Each page flip event object
>>   is created by DRM core so DRM core should release the object including
>>   incrementing event space.
> 
> I'm a bit confused here; we no longer call event->base.destroy(),
> because you say that the DRM core should release it. But how does the
> DRM core know to release the event? From the core point of view, the
> event disappears into the driver, and it is no longer tracked.

DRM core would need something to track the events. I think basically, someone who created one object should also destroy the object.

> 
> As Daniel says though, later versions handle all this in the core in a
> much more clean way, so we can remove these from the drivers then.

So I think it's not reasonable for specific driver to destroy the object created by core although there is a memory leak. However, the memory leak would be more critical than temporary codes.
Ok, I will merge this patch with more comments which will say the object will be destroyed by core part later.

Thanks,
Inki Dae

> 
> Cheers,
> Daniel
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/exynos: fix kernel panic issue at drm releasing

[Daniel Stone]

Hi Inki,

On 12 January 2016 at 06:25, Inki Dae <inki.dae@samsung.com> wrote:
> 2016년 01월 12일 04:00에 Daniel Stone 이(가) 쓴 글:
>> On 8 January 2016 at 08:46, Inki Dae <inki.dae@samsung.com> wrote:
>>> Changelog v3:
>>> - initialize only device specific things. Each page flip event object
>>>   is created by DRM core so DRM core should release the object including
>>>   incrementing event space.
>>
>> I'm a bit confused here; we no longer call event->base.destroy(),
>> because you say that the DRM core should release it. But how does the
>> DRM core know to release the event? From the core point of view, the
>> event disappears into the driver, and it is no longer tracked.
>
> DRM core would need something to track the events. I think basically, someone who created one object should also destroy the object.

You're right, but this doesn't exist until Daniel Vetter's rather
larger patchset which is still pending merge.

>> As Daniel says though, later versions handle all this in the core in a
>> much more clean way, so we can remove these from the drivers then.
>
> So I think it's not reasonable for specific driver to destroy the object created by core although there is a memory leak. However, the memory leak would be more critical than temporary codes.
> Ok, I will merge this patch with more comments which will say the object will be destroyed by core part later.

Also, by stealing the event out of crtc_state->event and moving it to
exynos_crtc->event, you can argue that we have quite explictly removed
the responsibility from the core. ;)

Thanks for handling this!

Cheers,
Daniel