[Intel-gfx] [PATCH 18/37] drm/exynos: Drop drm_vblank_cleanup

[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 at samsung.com>
> > Cc: Joonyoung Shim <jy0922.shim at samsung.com>
> > Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
> > Cc: Kyungmin Park <kyungmin.park at samsung.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at 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:
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch