| Message ID | 20170524145212.27837-19-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, 2017년 05월 24일 23:51에 Daniel Vetter 이(가) 쓴 글: > Only in the load failure path, where the hardware is quiet anyway. > > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > index 50294a7bd29d..1c814b9342af 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -376,7 +376,7 @@ static int exynos_drm_bind(struct device *dev) > /* Probe non kms sub drivers and virtual display driver. */ > ret = exynos_drm_device_subdrv_probe(drm); > if (ret) > - goto err_cleanup_vblank; > + goto err_unbind_all; With this change shouldn't you post the patch to remove drm_vblank_init and setup vblank stuff in drm_crtc_init together? I couldn't find the relevant patch on your patch series[1]. As of now, I think resource leak would happen with this patch only. Thanks, Inki Dae [1] http://www.spinics.net/lists/dri-devel/msg142387.html > > drm_mode_config_reset(drm); > > @@ -407,8 +407,6 @@ static int exynos_drm_bind(struct device *dev) > exynos_drm_fbdev_fini(drm); > drm_kms_helper_poll_fini(drm); > exynos_drm_device_subdrv_remove(drm); > -err_cleanup_vblank: > - drm_vblank_cleanup(drm); > err_unbind_all: > component_unbind_all(drm->dev, drm); > err_mode_config_cleanup: >
On Tue, May 30, 2017 at 09:03:34AM +0900, Inki Dae wrote: > Hi Daniel, > > 2017년 05월 24일 23:51에 Daniel Vetter 이(가) 쓴 글: > > Only in the load failure path, where the hardware is quiet anyway. > > > > Cc: Inki Dae <inki.dae@samsung.com> > > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > > Cc: Kyungmin Park <kyungmin.park@samsung.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > index 50294a7bd29d..1c814b9342af 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > @@ -376,7 +376,7 @@ static int exynos_drm_bind(struct device *dev) > > /* Probe non kms sub drivers and virtual display driver. */ > > ret = exynos_drm_device_subdrv_probe(drm); > > if (ret) > > - goto err_cleanup_vblank; > > + goto err_unbind_all; > > With this change shouldn't you post the patch to remove drm_vblank_init and setup vblank stuff in drm_crtc_init together? > I couldn't find the relevant patch on your patch series[1]. No, drm_vblank_cleanup is already called in the core. The only reason to call it in the driver is to fall back from kms to ums when irq setup somehow failed, then you need to disable vblank support again. The only driver which ever needed this was radeon, and radeon long ago lost ums support. drm_vblank_cleanup is already called for you, and most drivers don't even do this cleanup call. But somehow a lot of people copied from radeon without understanding what it does. Looking at the last patch in this series might help a bit in understanding the history here. Can you pls reevaluate the patch? Thanks, Daniel > As of now, I think resource leak would happen with this patch only. > > Thanks, > Inki Dae > > [1] http://www.spinics.net/lists/dri-devel/msg142387.html > > > > > drm_mode_config_reset(drm); > > > > @@ -407,8 +407,6 @@ static int exynos_drm_bind(struct device *dev) > > exynos_drm_fbdev_fini(drm); > > drm_kms_helper_poll_fini(drm); > > exynos_drm_device_subdrv_remove(drm); > > -err_cleanup_vblank: > > - drm_vblank_cleanup(drm); > > err_unbind_all: > > component_unbind_all(drm->dev, drm); > > err_mode_config_cleanup: > >
2017년 05월 31일 17:45에 Daniel Vetter 이(가) 쓴 글: > On Tue, May 30, 2017 at 09:03:34AM +0900, Inki Dae wrote: >> Hi Daniel, >> >> 2017년 05월 24일 23:51에 Daniel Vetter 이(가) 쓴 글: >>> Only in the load failure path, where the hardware is quiet anyway. >>> >>> Cc: Inki Dae <inki.dae@samsung.com> >>> Cc: Joonyoung Shim <jy0922.shim@samsung.com> >>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com> >>> Cc: Kyungmin Park <kyungmin.park@samsung.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c >>> index 50294a7bd29d..1c814b9342af 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c >>> @@ -376,7 +376,7 @@ static int exynos_drm_bind(struct device *dev) >>> /* Probe non kms sub drivers and virtual display driver. */ >>> ret = exynos_drm_device_subdrv_probe(drm); >>> if (ret) >>> - goto err_cleanup_vblank; >>> + goto err_unbind_all; >> >> With this change shouldn't you post the patch to remove drm_vblank_init and setup vblank stuff in drm_crtc_init together? >> I couldn't find the relevant patch on your patch series[1]. > > No, drm_vblank_cleanup is already called in the core. The only reason to > call it in the driver is to fall back from kms to ums when irq setup > somehow failed, then you need to disable vblank support again. > > The only driver which ever needed this was radeon, and radeon long ago > lost ums support. drm_vblank_cleanup is already called for you, and most > drivers don't even do this cleanup call. But somehow a lot of people > copied from radeon without understanding what it does. > > Looking at the last patch in this series might help a bit in understanding > the history here. Can you pls reevaluate the patch? Thanks for explaination. Confirmed. Reviewed-by: Inki Dae <inki.dae@samsung.com> Thanks, Inki Dae > > Thanks, Daniel > >> As of now, I think resource leak would happen with this patch only. >> >> Thanks, >> Inki Dae >> >> [1] http://www.spinics.net/lists/dri-devel/msg142387.html >> >>> >>> drm_mode_config_reset(drm); >>> >>> @@ -407,8 +407,6 @@ static int exynos_drm_bind(struct device *dev) >>> exynos_drm_fbdev_fini(drm); >>> drm_kms_helper_poll_fini(drm); >>> exynos_drm_device_subdrv_remove(drm); >>> -err_cleanup_vblank: >>> - drm_vblank_cleanup(drm); >>> err_unbind_all: >>> component_unbind_all(drm->dev, drm); >>> err_mode_config_cleanup: >>> >
On Thu, Jun 01, 2017 at 03:15:13PM +0900, Inki Dae wrote: > > > 2017년 05월 31일 17:45에 Daniel Vetter 이(가) 쓴 글: > > On Tue, May 30, 2017 at 09:03:34AM +0900, Inki Dae wrote: > >> Hi Daniel, > >> > >> 2017년 05월 24일 23:51에 Daniel Vetter 이(가) 쓴 글: > >>> Only in the load failure path, where the hardware is quiet anyway. > >>> > >>> Cc: Inki Dae <inki.dae@samsung.com> > >>> Cc: Joonyoung Shim <jy0922.shim@samsung.com> > >>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > >>> Cc: Kyungmin Park <kyungmin.park@samsung.com> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>> --- > >>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +--- > >>> 1 file changed, 1 insertion(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > >>> index 50294a7bd29d..1c814b9342af 100644 > >>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > >>> @@ -376,7 +376,7 @@ static int exynos_drm_bind(struct device *dev) > >>> /* Probe non kms sub drivers and virtual display driver. */ > >>> ret = exynos_drm_device_subdrv_probe(drm); > >>> if (ret) > >>> - goto err_cleanup_vblank; > >>> + goto err_unbind_all; > >> > >> With this change shouldn't you post the patch to remove drm_vblank_init and setup vblank stuff in drm_crtc_init together? > >> I couldn't find the relevant patch on your patch series[1]. > > > > No, drm_vblank_cleanup is already called in the core. The only reason to > > call it in the driver is to fall back from kms to ums when irq setup > > somehow failed, then you need to disable vblank support again. > > > > The only driver which ever needed this was radeon, and radeon long ago > > lost ums support. drm_vblank_cleanup is already called for you, and most > > drivers don't even do this cleanup call. But somehow a lot of people > > copied from radeon without understanding what it does. > > > > Looking at the last patch in this series might help a bit in understanding > > the history here. Can you pls reevaluate the patch? > > > Thanks for explaination. Confirmed. > > Reviewed-by: Inki Dae <inki.dae@samsung.com> Thanks for the review, patch applied to drm-misc-next. -Daniel
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 50294a7bd29d..1c814b9342af 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -376,7 +376,7 @@ static int exynos_drm_bind(struct device *dev) /* Probe non kms sub drivers and virtual display driver. */ ret = exynos_drm_device_subdrv_probe(drm); if (ret) - goto err_cleanup_vblank; + goto err_unbind_all; drm_mode_config_reset(drm); @@ -407,8 +407,6 @@ static int exynos_drm_bind(struct device *dev) exynos_drm_fbdev_fini(drm); drm_kms_helper_poll_fini(drm); exynos_drm_device_subdrv_remove(drm); -err_cleanup_vblank: - drm_vblank_cleanup(drm); err_unbind_all: component_unbind_all(drm->dev, drm); err_mode_config_cleanup:
Only in the load failure path, where the hardware is quiet anyway. Cc: Inki Dae <inki.dae@samsung.com> Cc: Joonyoung Shim <jy0922.shim@samsung.com> Cc: Seung-Woo Kim <sw0312.kim@samsung.com> Cc: Kyungmin Park <kyungmin.park@samsung.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)