| Message ID | 1450948214-14688-1-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Inki, On 24 December 2015 at 09:10, Inki Dae <inki.dae@samsung.com> wrote: > +void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc) > +{ > + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); > + unsigned long flags; > + > + spin_lock_irqsave(&crtc->dev->event_lock, flags); > + exynos_crtc->event = NULL; > + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > +} This will leak the event and event space; you should call event->base.destroy() here. With that fixed: Reviewed-by: Daniel Stone <daniels@collabora.com> Cheers, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Daniel, 2015? 12? 24? 22:32? Daniel Stone ?(?) ? ?: > Hi Inki, > > On 24 December 2015 at 09:10, Inki Dae <inki.dae@samsung.com> wrote: >> +void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc) >> +{ >> + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&crtc->dev->event_lock, flags); >> + exynos_crtc->event = NULL; >> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >> +} > > This will leak the event and event space; you should call > event->base.destroy() here. With that fixed: Right. we don't use exynos specific page flip function anymore which managed the event as a list so that the event objects can be freed by postclose callback. Anyway, would it be better for event->base.destory() to be called between spin lock/unlock? Thanks, Inki Dae > Reviewed-by: Daniel Stone <daniels@collabora.com> > > Cheers, > Daniel > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Inki, On 4 January 2016 at 12:57, Inki Dae <inki.dae@samsung.com> wrote: > 2015? 12? 24? 22:32? Daniel Stone ?(?) ? ?: >> On 24 December 2015 at 09:10, Inki Dae <inki.dae@samsung.com> wrote: >>> +void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc) >>> +{ >>> + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&crtc->dev->event_lock, flags); >>> + exynos_crtc->event = NULL; >>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >>> +} >> >> This will leak the event and event space; you should call >> event->base.destroy() here. With that fixed: > > Right. we don't use exynos specific page flip function anymore which managed the event as a list so that the event objects can be freed by postclose callback. > Anyway, would it be better for event->base.destory() to be called between spin lock/unlock? You must increment event->base.file_priv->event_space (see drm_atomic.c:destroy_vblank_event), as well as calling event->base.destroy (see drm_fops.c:drm_read) underneath event_lock, yes. Cheers, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Daniel, 2016? 01? 05? 05:24? Daniel Stone ?(?) ? ?: > Hi Inki, > > On 4 January 2016 at 12:57, Inki Dae <inki.dae@samsung.com> wrote: >> 2015? 12? 24? 22:32? Daniel Stone ?(?) ? ?: >>> On 24 December 2015 at 09:10, Inki Dae <inki.dae@samsung.com> wrote: >>>> +void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc) >>>> +{ >>>> + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); >>>> + unsigned long flags; >>>> + >>>> + spin_lock_irqsave(&crtc->dev->event_lock, flags); >>>> + exynos_crtc->event = NULL; >>>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >>>> +} >>> >>> This will leak the event and event space; you should call >>> event->base.destroy() here. With that fixed: >> >> Right. we don't use exynos specific page flip function anymore which managed the event as a list so that the event objects can be freed by postclose callback. >> Anyway, would it be better for event->base.destory() to be called between spin lock/unlock? > > You must increment event->base.file_priv->event_space (see > drm_atomic.c:destroy_vblank_event), as well as calling Reasonable to me. Seems other DRM drivers don't increment event_space. > event->base.destroy (see drm_fops.c:drm_read) underneath event_lock, > yes. In addition, only event objects belonging to the request process should be destroyed. Thanks, Inki Dae > > Cheers, > Daniel > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 05, 2016 at 07:55:52PM +0900, Inki Dae wrote: > Hi Daniel, > > 2016? 01? 05? 05:24? Daniel Stone ?(?) ? ?: > > Hi Inki, > > > > On 4 January 2016 at 12:57, Inki Dae <inki.dae@samsung.com> wrote: > >> 2015? 12? 24? 22:32? Daniel Stone ?(?) ? ?: > >>> On 24 December 2015 at 09:10, Inki Dae <inki.dae@samsung.com> wrote: > >>>> +void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc) > >>>> +{ > >>>> + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); > >>>> + unsigned long flags; > >>>> + > >>>> + spin_lock_irqsave(&crtc->dev->event_lock, flags); > >>>> + exynos_crtc->event = NULL; > >>>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > >>>> +} > >>> > >>> This will leak the event and event space; you should call > >>> event->base.destroy() here. With that fixed: > >> > >> Right. we don't use exynos specific page flip function anymore which managed the event as a list so that the event objects can be freed by postclose callback. > >> Anyway, would it be better for event->base.destory() to be called between spin lock/unlock? > > > > You must increment event->base.file_priv->event_space (see > > drm_atomic.c:destroy_vblank_event), as well as calling > > Reasonable to me. Seems other DRM drivers don't increment event_space. > > > event->base.destroy (see drm_fops.c:drm_read) underneath event_lock, > > yes. > > In addition, only event objects belonging to the request process should be destroyed. Just random comment out of the far left field, but robclark had a bunch of patches to clean up all that event alloc/cleanup code a bit and extract it into core functions. Might be good to ping him on irc to figure out where that series is and whether you could take it over. Cheers, Daniel
+ Rob Clark, Hi Daniel and Rob, 2016? 01? 05? 20:08? Daniel Vetter ?(?) ? ?: > On Tue, Jan 05, 2016 at 07:55:52PM +0900, Inki Dae wrote: >> Hi Daniel, >> >> 2016? 01? 05? 05:24? Daniel Stone ?(?) ? ?: >>> Hi Inki, >>> >>> On 4 January 2016 at 12:57, Inki Dae <inki.dae@samsung.com> wrote: >>>> 2015? 12? 24? 22:32? Daniel Stone ?(?) ? ?: >>>>> On 24 December 2015 at 09:10, Inki Dae <inki.dae@samsung.com> wrote: >>>>>> +void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc) >>>>>> +{ >>>>>> + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); >>>>>> + unsigned long flags; >>>>>> + >>>>>> + spin_lock_irqsave(&crtc->dev->event_lock, flags); >>>>>> + exynos_crtc->event = NULL; >>>>>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >>>>>> +} >>>>> >>>>> This will leak the event and event space; you should call >>>>> event->base.destroy() here. With that fixed: >>>> >>>> Right. we don't use exynos specific page flip function anymore which managed the event as a list so that the event objects can be freed by postclose callback. >>>> Anyway, would it be better for event->base.destory() to be called between spin lock/unlock? >>> >>> You must increment event->base.file_priv->event_space (see >>> drm_atomic.c:destroy_vblank_event), as well as calling >> >> Reasonable to me. Seems other DRM drivers don't increment event_space. >> >>> event->base.destroy (see drm_fops.c:drm_read) underneath event_lock, >>> yes. >> >> In addition, only event objects belonging to the request process should be destroyed. > > Just random comment out of the far left field, but robclark had a bunch of > patches to clean up all that event alloc/cleanup code a bit and extract it > into core functions. Might be good to ping him on irc to figure out where > that series is and whether you could take it over. Good news. I'll try to ping him on irc. To Rob, Can you let me know where your bunch of patches are? I'd like to look into the patches. I'd planned to have pull request so that this patch can go to 4.4. As you had already relevant patch set maybe, we would need to check whether my patch can be replaced with your patch set or there is any corner case. Thanks, Inki Dae > > Cheers, Daniel > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 81cfff5..57619b8 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -235,3 +235,13 @@ 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); + unsigned long flags; + + spin_lock_irqsave(&crtc->dev->event_lock, flags); + exynos_crtc->event = NULL; + 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)
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. Signed-off-by: Inki Dae <inki.dae@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 10 ++++++++++ drivers/gpu/drm/exynos/exynos_drm_crtc.h | 3 +++ drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 +++++ 3 files changed, 18 insertions(+)