| Message ID | 20220610092924.754942-20-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Fix hotplug for vc4 | expand |
On Fri, 10 Jun 2022 at 10:30, Maxime Ripard <maxime@cerno.tech> wrote: > > The current code will call drm_crtc_cleanup() when the device is > unbound. However, by then, there might still be some references held to > that CRTC, including by the userspace that might still have the DRM > device open. > > Let's switch to a DRM-managed initialization to clean up after ourselves > only once the DRM device has been last closed. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 18 +++++++----------- > drivers/gpu/drm/vc4/vc4_drv.h | 1 - > drivers/gpu/drm/vc4/vc4_txp.c | 1 - > 3 files changed, 7 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index c74fa3d07561..24de4706b61a 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -205,11 +205,6 @@ static bool vc4_crtc_get_scanout_position(struct drm_crtc *crtc, > return ret; > } > > -void vc4_crtc_destroy(struct drm_crtc *crtc) > -{ > - drm_crtc_cleanup(crtc); > -} > - > static u32 vc4_get_fifo_full_level(struct vc4_crtc *vc4_crtc, u32 format) > { > const struct vc4_crtc_data *crtc_data = vc4_crtc_to_vc4_crtc_data(vc4_crtc); > @@ -953,7 +948,6 @@ void vc4_crtc_reset(struct drm_crtc *crtc) > > static const struct drm_crtc_funcs vc4_crtc_funcs = { > .set_config = drm_atomic_helper_set_config, > - .destroy = vc4_crtc_destroy, > .page_flip = vc4_page_flip, > .set_property = NULL, > .cursor_set = NULL, /* handled by drm_mode_cursor_universal */ > @@ -1131,6 +1125,7 @@ int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, > struct drm_crtc *crtc = &vc4_crtc->base; > struct drm_plane *primary_plane; > unsigned int i; > + int ret; > > /* For now, we create just the primary and the legacy cursor > * planes. We should be able to stack more planes on easily, > @@ -1144,10 +1139,13 @@ int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, > return PTR_ERR(primary_plane); > } > > - spin_lock_init(&vc4_crtc->irq_lock); > - drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL, > - crtc_funcs, NULL); > + ret = drmm_crtc_init_with_planes(drm, crtc, primary_plane, NULL, > + crtc_funcs, NULL); > + if (ret) > + return ret; > + > drm_crtc_helper_add(crtc, crtc_helper_funcs); > + spin_lock_init(&vc4_crtc->irq_lock); Moving the spin_lock_init appears to be cosmetic and unrelated, but otherwise: Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > if (!vc4->hvs->hvs5) { > drm_mode_crtc_set_gamma_size(crtc, ARRAY_SIZE(vc4_crtc->lut_r)); > @@ -1226,8 +1224,6 @@ static void vc4_crtc_unbind(struct device *dev, struct device *master, > struct platform_device *pdev = to_platform_device(dev); > struct vc4_crtc *vc4_crtc = dev_get_drvdata(dev); > > - vc4_crtc_destroy(&vc4_crtc->base); > - > CRTC_WRITE(PV_INTEN, 0); > > platform_set_drvdata(pdev, NULL); > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index 9a53ace85d95..fff3772be2d4 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -845,7 +845,6 @@ int vc4_crtc_disable_at_boot(struct drm_crtc *crtc); > int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, > const struct drm_crtc_funcs *crtc_funcs, > const struct drm_crtc_helper_funcs *crtc_helper_funcs); > -void vc4_crtc_destroy(struct drm_crtc *crtc); > int vc4_page_flip(struct drm_crtc *crtc, > struct drm_framebuffer *fb, > struct drm_pending_vblank_event *event, > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > index e983ff7c5e13..f306e05ac5b2 100644 > --- a/drivers/gpu/drm/vc4/vc4_txp.c > +++ b/drivers/gpu/drm/vc4/vc4_txp.c > @@ -383,7 +383,6 @@ static void vc4_txp_disable_vblank(struct drm_crtc *crtc) {} > > static const struct drm_crtc_funcs vc4_txp_crtc_funcs = { > .set_config = drm_atomic_helper_set_config, > - .destroy = vc4_crtc_destroy, > .page_flip = vc4_page_flip, > .reset = vc4_crtc_reset, > .atomic_duplicate_state = vc4_crtc_duplicate_state, > -- > 2.36.1 >
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index c74fa3d07561..24de4706b61a 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -205,11 +205,6 @@ static bool vc4_crtc_get_scanout_position(struct drm_crtc *crtc, return ret; } -void vc4_crtc_destroy(struct drm_crtc *crtc) -{ - drm_crtc_cleanup(crtc); -} - static u32 vc4_get_fifo_full_level(struct vc4_crtc *vc4_crtc, u32 format) { const struct vc4_crtc_data *crtc_data = vc4_crtc_to_vc4_crtc_data(vc4_crtc); @@ -953,7 +948,6 @@ void vc4_crtc_reset(struct drm_crtc *crtc) static const struct drm_crtc_funcs vc4_crtc_funcs = { .set_config = drm_atomic_helper_set_config, - .destroy = vc4_crtc_destroy, .page_flip = vc4_page_flip, .set_property = NULL, .cursor_set = NULL, /* handled by drm_mode_cursor_universal */ @@ -1131,6 +1125,7 @@ int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, struct drm_crtc *crtc = &vc4_crtc->base; struct drm_plane *primary_plane; unsigned int i; + int ret; /* For now, we create just the primary and the legacy cursor * planes. We should be able to stack more planes on easily, @@ -1144,10 +1139,13 @@ int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, return PTR_ERR(primary_plane); } - spin_lock_init(&vc4_crtc->irq_lock); - drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL, - crtc_funcs, NULL); + ret = drmm_crtc_init_with_planes(drm, crtc, primary_plane, NULL, + crtc_funcs, NULL); + if (ret) + return ret; + drm_crtc_helper_add(crtc, crtc_helper_funcs); + spin_lock_init(&vc4_crtc->irq_lock); if (!vc4->hvs->hvs5) { drm_mode_crtc_set_gamma_size(crtc, ARRAY_SIZE(vc4_crtc->lut_r)); @@ -1226,8 +1224,6 @@ static void vc4_crtc_unbind(struct device *dev, struct device *master, struct platform_device *pdev = to_platform_device(dev); struct vc4_crtc *vc4_crtc = dev_get_drvdata(dev); - vc4_crtc_destroy(&vc4_crtc->base); - CRTC_WRITE(PV_INTEN, 0); platform_set_drvdata(pdev, NULL); diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 9a53ace85d95..fff3772be2d4 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -845,7 +845,6 @@ int vc4_crtc_disable_at_boot(struct drm_crtc *crtc); int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, const struct drm_crtc_funcs *crtc_funcs, const struct drm_crtc_helper_funcs *crtc_helper_funcs); -void vc4_crtc_destroy(struct drm_crtc *crtc); int vc4_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index e983ff7c5e13..f306e05ac5b2 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -383,7 +383,6 @@ static void vc4_txp_disable_vblank(struct drm_crtc *crtc) {} static const struct drm_crtc_funcs vc4_txp_crtc_funcs = { .set_config = drm_atomic_helper_set_config, - .destroy = vc4_crtc_destroy, .page_flip = vc4_page_flip, .reset = vc4_crtc_reset, .atomic_duplicate_state = vc4_crtc_duplicate_state,
The current code will call drm_crtc_cleanup() when the device is unbound. However, by then, there might still be some references held to that CRTC, including by the userspace that might still have the DRM device open. Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_crtc.c | 18 +++++++----------- drivers/gpu/drm/vc4/vc4_drv.h | 1 - drivers/gpu/drm/vc4/vc4_txp.c | 1 - 3 files changed, 7 insertions(+), 13 deletions(-)