| Message ID | 20170524145212.27837-24-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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). > 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> regards Philipp
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
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:
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. 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(-)