Re: [PATCH 23/37] drm/imx: Drop drm_vblank_cleanup

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 29, 2017 at 01:07:40PM +0200, Philipp Zabel wrote:
> Hi Daniel,
> 
> On Wed, 2017-05-24 at 16:51 +0200, Daniel Vetter wrote:
> > It's only done in the driver load error path, where vblanks don't need
> > to be quiescent anyway. And that's all drm_vblank_cleanup does, since
> > the core will release the vblank allocations on its own already. So
> > drop it.
> 
> Thank you for cleaning this up and improving the docs.
> From the function name and kerneldoc comment, it was really not clear
> that this function is already called in the drm_device release path.
> 
> I think the comment is slightly misleading though, as drm_vblank_cleanup
> does call kfree(dev->vblank).

Yeah I got a bit sloppy with the commit message. Since this is error code
resetting ->num_crtcs doesn't do anything useful, and the kfree happens
anyway (because the core calls drm_vblank_cleanup for you). That only
leaves the timer shutdown, and the timer can't run yet here at this point.

Hence this does nothing useful (except move the kfree around).
> 
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >
> > ---
> >  drivers/gpu/drm/imx/imx-drm-core.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > index 50add2f9e250..95e2181963d9 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -278,7 +278,7 @@ static int imx_drm_bind(struct device *dev)
> >  	/* Now try and bind all our sub-components */
> >  	ret = component_bind_all(dev, drm);
> >  	if (ret)
> > -		goto err_vblank;
> > +		goto err_kms;
> >  
> >  	drm_mode_config_reset(drm);
> >  
> > @@ -316,8 +316,6 @@ static int imx_drm_bind(struct device *dev)
> >  err_unbind:
> >  #endif
> >  	component_unbind_all(drm->dev, drm);
> > -err_vblank:
> > -	drm_vblank_cleanup(drm);
> >  err_kms:
> >  	drm_mode_config_cleanup(drm);
> >  err_unref:
> 
> As I understand, the drm_dev_unref(drm) that follows this causes
> drm_dev_release -> drm_dev_fini -> drm_vblank_cleanup to be called, so
> 
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks, applied, as I did all the other patches which gathered an ack or
r-b already.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel