* [PATCH] drm/sti: make crct disable atomic @ 2018-10-12 9:46 Benjamin Gaignard 2018-10-12 9:46 ` [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework Benjamin Gaignard 2018-10-18 12:06 ` [PATCH] drm/sti: make crct disable atomic Benjamin Gaignard 0 siblings, 2 replies; 6+ messages in thread From: Benjamin Gaignard @ 2018-10-12 9:46 UTC (permalink / raw) To: airlied, daniel; +Cc: dri-devel, linux-kernel, Benjamin Gaignard Wait until the next vblank to be sure that crtc has been disabled. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/sti/sti_crtc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/sti/sti_crtc.c b/drivers/gpu/drm/sti/sti_crtc.c index 5824e6aca8f4..61c2379fba87 100644 --- a/drivers/gpu/drm/sti/sti_crtc.c +++ b/drivers/gpu/drm/sti/sti_crtc.c @@ -40,6 +40,8 @@ static void sti_crtc_atomic_disable(struct drm_crtc *crtc, DRM_DEBUG_DRIVER("\n"); mixer->status = STI_MIXER_DISABLING; + + drm_crtc_wait_one_vblank(crtc); } static int -- 2.15.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework 2018-10-12 9:46 [PATCH] drm/sti: make crct disable atomic Benjamin Gaignard @ 2018-10-12 9:46 ` Benjamin Gaignard 2018-10-15 8:00 ` Daniel Vetter 2018-10-18 12:06 ` [PATCH] drm/sti: make crct disable atomic Benjamin Gaignard 1 sibling, 1 reply; 6+ messages in thread From: Benjamin Gaignard @ 2018-10-12 9:46 UTC (permalink / raw) To: airlied, daniel; +Cc: dri-devel, linux-kernel, Benjamin Gaignard Since drm_atomic_helper_shutdown() rework it is possible to do additional clean up in sti driver: custom plane destroy functions become useless and clean up encoder is no more needed. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> --- drivers/gpu/drm/sti/sti_cursor.c | 9 +-------- drivers/gpu/drm/sti/sti_gdp.c | 9 +-------- drivers/gpu/drm/sti/sti_hqvdp.c | 9 +-------- drivers/gpu/drm/sti/sti_tvout.c | 24 ------------------------ 4 files changed, 3 insertions(+), 48 deletions(-) diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c index bc908453ffb3..e1ba253055c7 100644 --- a/drivers/gpu/drm/sti/sti_cursor.c +++ b/drivers/gpu/drm/sti/sti_cursor.c @@ -328,13 +328,6 @@ static const struct drm_plane_helper_funcs sti_cursor_helpers_funcs = { .atomic_disable = sti_cursor_atomic_disable, }; -static void sti_cursor_destroy(struct drm_plane *drm_plane) -{ - DRM_DEBUG_DRIVER("\n"); - - drm_plane_cleanup(drm_plane); -} - static int sti_cursor_late_register(struct drm_plane *drm_plane) { struct sti_plane *plane = to_sti_plane(drm_plane); @@ -346,7 +339,7 @@ static int sti_cursor_late_register(struct drm_plane *drm_plane) static const struct drm_plane_funcs sti_cursor_plane_helpers_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = sti_cursor_destroy, + .destroy = drm_plane_cleanup, .reset = sti_plane_reset, .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c index 3c19614d3f75..87b50451afd7 100644 --- a/drivers/gpu/drm/sti/sti_gdp.c +++ b/drivers/gpu/drm/sti/sti_gdp.c @@ -879,13 +879,6 @@ static const struct drm_plane_helper_funcs sti_gdp_helpers_funcs = { .atomic_disable = sti_gdp_atomic_disable, }; -static void sti_gdp_destroy(struct drm_plane *drm_plane) -{ - DRM_DEBUG_DRIVER("\n"); - - drm_plane_cleanup(drm_plane); -} - static int sti_gdp_late_register(struct drm_plane *drm_plane) { struct sti_plane *plane = to_sti_plane(drm_plane); @@ -897,7 +890,7 @@ static int sti_gdp_late_register(struct drm_plane *drm_plane) static const struct drm_plane_funcs sti_gdp_plane_helpers_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = sti_gdp_destroy, + .destroy = drm_plane_cleanup, .reset = sti_plane_reset, .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c index 23565f52dd71..065a5b08a702 100644 --- a/drivers/gpu/drm/sti/sti_hqvdp.c +++ b/drivers/gpu/drm/sti/sti_hqvdp.c @@ -1256,13 +1256,6 @@ static const struct drm_plane_helper_funcs sti_hqvdp_helpers_funcs = { .atomic_disable = sti_hqvdp_atomic_disable, }; -static void sti_hqvdp_destroy(struct drm_plane *drm_plane) -{ - DRM_DEBUG_DRIVER("\n"); - - drm_plane_cleanup(drm_plane); -} - static int sti_hqvdp_late_register(struct drm_plane *drm_plane) { struct sti_plane *plane = to_sti_plane(drm_plane); @@ -1274,7 +1267,7 @@ static int sti_hqvdp_late_register(struct drm_plane *drm_plane) static const struct drm_plane_funcs sti_hqvdp_plane_helpers_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = sti_hqvdp_destroy, + .destroy = drm_plane_cleanup, .reset = sti_plane_reset, .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c index ea4a3b87fa55..4dc3b2ec40eb 100644 --- a/drivers/gpu/drm/sti/sti_tvout.c +++ b/drivers/gpu/drm/sti/sti_tvout.c @@ -788,21 +788,6 @@ static void sti_tvout_create_encoders(struct drm_device *dev, tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout); } -static void sti_tvout_destroy_encoders(struct sti_tvout *tvout) -{ - if (tvout->hdmi) - drm_encoder_cleanup(tvout->hdmi); - tvout->hdmi = NULL; - - if (tvout->hda) - drm_encoder_cleanup(tvout->hda); - tvout->hda = NULL; - - if (tvout->dvo) - drm_encoder_cleanup(tvout->dvo); - tvout->dvo = NULL; -} - static int sti_tvout_bind(struct device *dev, struct device *master, void *data) { struct sti_tvout *tvout = dev_get_drvdata(dev); @@ -815,17 +800,8 @@ static int sti_tvout_bind(struct device *dev, struct device *master, void *data) return 0; } -static void sti_tvout_unbind(struct device *dev, struct device *master, - void *data) -{ - struct sti_tvout *tvout = dev_get_drvdata(dev); - - sti_tvout_destroy_encoders(tvout); -} - static const struct component_ops sti_tvout_ops = { .bind = sti_tvout_bind, - .unbind = sti_tvout_unbind, }; static int sti_tvout_probe(struct platform_device *pdev) -- 2.15.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework 2018-10-12 9:46 ` [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework Benjamin Gaignard @ 2018-10-15 8:00 ` Daniel Vetter 2018-10-16 18:30 ` Benjamin Gaignard 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2018-10-15 8:00 UTC (permalink / raw) To: Benjamin Gaignard; +Cc: airlied, daniel, dri-devel, linux-kernel On Fri, Oct 12, 2018 at 11:46:39AM +0200, Benjamin Gaignard wrote: > Since drm_atomic_helper_shutdown() rework it is possible to do additional > clean up in sti driver: custom plane destroy functions become useless and > clean up encoder is no more needed. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> > --- > drivers/gpu/drm/sti/sti_cursor.c | 9 +-------- > drivers/gpu/drm/sti/sti_gdp.c | 9 +-------- > drivers/gpu/drm/sti/sti_hqvdp.c | 9 +-------- > drivers/gpu/drm/sti/sti_tvout.c | 24 ------------------------ > 4 files changed, 3 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c > index bc908453ffb3..e1ba253055c7 100644 > --- a/drivers/gpu/drm/sti/sti_cursor.c > +++ b/drivers/gpu/drm/sti/sti_cursor.c > @@ -328,13 +328,6 @@ static const struct drm_plane_helper_funcs sti_cursor_helpers_funcs = { > .atomic_disable = sti_cursor_atomic_disable, > }; > > -static void sti_cursor_destroy(struct drm_plane *drm_plane) > -{ > - DRM_DEBUG_DRIVER("\n"); > - > - drm_plane_cleanup(drm_plane); > -} > - > static int sti_cursor_late_register(struct drm_plane *drm_plane) > { > struct sti_plane *plane = to_sti_plane(drm_plane); > @@ -346,7 +339,7 @@ static int sti_cursor_late_register(struct drm_plane *drm_plane) > static const struct drm_plane_funcs sti_cursor_plane_helpers_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > - .destroy = sti_cursor_destroy, > + .destroy = drm_plane_cleanup, > .reset = sti_plane_reset, > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c > index 3c19614d3f75..87b50451afd7 100644 > --- a/drivers/gpu/drm/sti/sti_gdp.c > +++ b/drivers/gpu/drm/sti/sti_gdp.c > @@ -879,13 +879,6 @@ static const struct drm_plane_helper_funcs sti_gdp_helpers_funcs = { > .atomic_disable = sti_gdp_atomic_disable, > }; > > -static void sti_gdp_destroy(struct drm_plane *drm_plane) > -{ > - DRM_DEBUG_DRIVER("\n"); > - > - drm_plane_cleanup(drm_plane); > -} > - > static int sti_gdp_late_register(struct drm_plane *drm_plane) > { > struct sti_plane *plane = to_sti_plane(drm_plane); > @@ -897,7 +890,7 @@ static int sti_gdp_late_register(struct drm_plane *drm_plane) > static const struct drm_plane_funcs sti_gdp_plane_helpers_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > - .destroy = sti_gdp_destroy, > + .destroy = drm_plane_cleanup, > .reset = sti_plane_reset, > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c > index 23565f52dd71..065a5b08a702 100644 > --- a/drivers/gpu/drm/sti/sti_hqvdp.c > +++ b/drivers/gpu/drm/sti/sti_hqvdp.c > @@ -1256,13 +1256,6 @@ static const struct drm_plane_helper_funcs sti_hqvdp_helpers_funcs = { > .atomic_disable = sti_hqvdp_atomic_disable, > }; > > -static void sti_hqvdp_destroy(struct drm_plane *drm_plane) > -{ > - DRM_DEBUG_DRIVER("\n"); > - > - drm_plane_cleanup(drm_plane); > -} > - > static int sti_hqvdp_late_register(struct drm_plane *drm_plane) > { > struct sti_plane *plane = to_sti_plane(drm_plane); > @@ -1274,7 +1267,7 @@ static int sti_hqvdp_late_register(struct drm_plane *drm_plane) > static const struct drm_plane_funcs sti_hqvdp_plane_helpers_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > - .destroy = sti_hqvdp_destroy, > + .destroy = drm_plane_cleanup, > .reset = sti_plane_reset, > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c > index ea4a3b87fa55..4dc3b2ec40eb 100644 > --- a/drivers/gpu/drm/sti/sti_tvout.c > +++ b/drivers/gpu/drm/sti/sti_tvout.c > @@ -788,21 +788,6 @@ static void sti_tvout_create_encoders(struct drm_device *dev, > tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout); > } > > -static void sti_tvout_destroy_encoders(struct sti_tvout *tvout) > -{ > - if (tvout->hdmi) > - drm_encoder_cleanup(tvout->hdmi); > - tvout->hdmi = NULL; > - > - if (tvout->hda) > - drm_encoder_cleanup(tvout->hda); > - tvout->hda = NULL; > - > - if (tvout->dvo) > - drm_encoder_cleanup(tvout->dvo); > - tvout->dvo = NULL; > -} > - > static int sti_tvout_bind(struct device *dev, struct device *master, void *data) > { > struct sti_tvout *tvout = dev_get_drvdata(dev); > @@ -815,17 +800,8 @@ static int sti_tvout_bind(struct device *dev, struct device *master, void *data) > return 0; > } > > -static void sti_tvout_unbind(struct device *dev, struct device *master, > - void *data) > -{ > - struct sti_tvout *tvout = dev_get_drvdata(dev); > - > - sti_tvout_destroy_encoders(tvout); > -} > - > static const struct component_ops sti_tvout_ops = { > .bind = sti_tvout_bind, > - .unbind = sti_tvout_unbind, Hm, this here looks strange now. I'd put a comment somewhere that master_ops->unbind cleans up everything. Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > }; > > static int sti_tvout_probe(struct platform_device *pdev) > -- > 2.15.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework 2018-10-15 8:00 ` Daniel Vetter @ 2018-10-16 18:30 ` Benjamin Gaignard 2018-10-16 19:15 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Benjamin Gaignard @ 2018-10-16 18:30 UTC (permalink / raw) To: David Airlie, ML dri-devel, Linux Kernel Mailing List; +Cc: Daniel Vetter Le lun. 15 oct. 2018 à 10:00, Daniel Vetter <daniel@ffwll.ch> a écrit : > > On Fri, Oct 12, 2018 at 11:46:39AM +0200, Benjamin Gaignard wrote: > > Since drm_atomic_helper_shutdown() rework it is possible to do additional > > clean up in sti driver: custom plane destroy functions become useless and > > clean up encoder is no more needed. > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > --- > > drivers/gpu/drm/sti/sti_cursor.c | 9 +-------- > > drivers/gpu/drm/sti/sti_gdp.c | 9 +-------- > > drivers/gpu/drm/sti/sti_hqvdp.c | 9 +-------- > > drivers/gpu/drm/sti/sti_tvout.c | 24 ------------------------ > > 4 files changed, 3 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c > > index bc908453ffb3..e1ba253055c7 100644 > > --- a/drivers/gpu/drm/sti/sti_cursor.c > > +++ b/drivers/gpu/drm/sti/sti_cursor.c > > @@ -328,13 +328,6 @@ static const struct drm_plane_helper_funcs sti_cursor_helpers_funcs = { > > .atomic_disable = sti_cursor_atomic_disable, > > }; > > > > -static void sti_cursor_destroy(struct drm_plane *drm_plane) > > -{ > > - DRM_DEBUG_DRIVER("\n"); > > - > > - drm_plane_cleanup(drm_plane); > > -} > > - > > static int sti_cursor_late_register(struct drm_plane *drm_plane) > > { > > struct sti_plane *plane = to_sti_plane(drm_plane); > > @@ -346,7 +339,7 @@ static int sti_cursor_late_register(struct drm_plane *drm_plane) > > static const struct drm_plane_funcs sti_cursor_plane_helpers_funcs = { > > .update_plane = drm_atomic_helper_update_plane, > > .disable_plane = drm_atomic_helper_disable_plane, > > - .destroy = sti_cursor_destroy, > > + .destroy = drm_plane_cleanup, > > .reset = sti_plane_reset, > > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > > diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c > > index 3c19614d3f75..87b50451afd7 100644 > > --- a/drivers/gpu/drm/sti/sti_gdp.c > > +++ b/drivers/gpu/drm/sti/sti_gdp.c > > @@ -879,13 +879,6 @@ static const struct drm_plane_helper_funcs sti_gdp_helpers_funcs = { > > .atomic_disable = sti_gdp_atomic_disable, > > }; > > > > -static void sti_gdp_destroy(struct drm_plane *drm_plane) > > -{ > > - DRM_DEBUG_DRIVER("\n"); > > - > > - drm_plane_cleanup(drm_plane); > > -} > > - > > static int sti_gdp_late_register(struct drm_plane *drm_plane) > > { > > struct sti_plane *plane = to_sti_plane(drm_plane); > > @@ -897,7 +890,7 @@ static int sti_gdp_late_register(struct drm_plane *drm_plane) > > static const struct drm_plane_funcs sti_gdp_plane_helpers_funcs = { > > .update_plane = drm_atomic_helper_update_plane, > > .disable_plane = drm_atomic_helper_disable_plane, > > - .destroy = sti_gdp_destroy, > > + .destroy = drm_plane_cleanup, > > .reset = sti_plane_reset, > > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > > diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c > > index 23565f52dd71..065a5b08a702 100644 > > --- a/drivers/gpu/drm/sti/sti_hqvdp.c > > +++ b/drivers/gpu/drm/sti/sti_hqvdp.c > > @@ -1256,13 +1256,6 @@ static const struct drm_plane_helper_funcs sti_hqvdp_helpers_funcs = { > > .atomic_disable = sti_hqvdp_atomic_disable, > > }; > > > > -static void sti_hqvdp_destroy(struct drm_plane *drm_plane) > > -{ > > - DRM_DEBUG_DRIVER("\n"); > > - > > - drm_plane_cleanup(drm_plane); > > -} > > - > > static int sti_hqvdp_late_register(struct drm_plane *drm_plane) > > { > > struct sti_plane *plane = to_sti_plane(drm_plane); > > @@ -1274,7 +1267,7 @@ static int sti_hqvdp_late_register(struct drm_plane *drm_plane) > > static const struct drm_plane_funcs sti_hqvdp_plane_helpers_funcs = { > > .update_plane = drm_atomic_helper_update_plane, > > .disable_plane = drm_atomic_helper_disable_plane, > > - .destroy = sti_hqvdp_destroy, > > + .destroy = drm_plane_cleanup, > > .reset = sti_plane_reset, > > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > > diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c > > index ea4a3b87fa55..4dc3b2ec40eb 100644 > > --- a/drivers/gpu/drm/sti/sti_tvout.c > > +++ b/drivers/gpu/drm/sti/sti_tvout.c > > @@ -788,21 +788,6 @@ static void sti_tvout_create_encoders(struct drm_device *dev, > > tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout); > > } > > > > -static void sti_tvout_destroy_encoders(struct sti_tvout *tvout) > > -{ > > - if (tvout->hdmi) > > - drm_encoder_cleanup(tvout->hdmi); > > - tvout->hdmi = NULL; > > - > > - if (tvout->hda) > > - drm_encoder_cleanup(tvout->hda); > > - tvout->hda = NULL; > > - > > - if (tvout->dvo) > > - drm_encoder_cleanup(tvout->dvo); > > - tvout->dvo = NULL; > > -} > > - > > static int sti_tvout_bind(struct device *dev, struct device *master, void *data) > > { > > struct sti_tvout *tvout = dev_get_drvdata(dev); > > @@ -815,17 +800,8 @@ static int sti_tvout_bind(struct device *dev, struct device *master, void *data) > > return 0; > > } > > > > -static void sti_tvout_unbind(struct device *dev, struct device *master, > > - void *data) > > -{ > > - struct sti_tvout *tvout = dev_get_drvdata(dev); > > - > > - sti_tvout_destroy_encoders(tvout); > > -} > > - > > static const struct component_ops sti_tvout_ops = { > > .bind = sti_tvout_bind, > > - .unbind = sti_tvout_unbind, > > Hm, this here looks strange now. I'd put a comment somewhere that > master_ops->unbind cleans up everything. Either way: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Hi Daniel, unbind undo what has been done in bind even the functions aren't symetric: encoder creation are done is this level of the driver while they destruction is done in the top level of the driver by calling drm shutdown function. From module point of view bind and unbind are balanced correctly. I have test it on board :-) I will not merge this patch until I get a clear review from you. Regards, Benjamin > > > }; > > > > static int sti_tvout_probe(struct platform_device *pdev) > > -- > > 2.15.0 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework 2018-10-16 18:30 ` Benjamin Gaignard @ 2018-10-16 19:15 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2018-10-16 19:15 UTC (permalink / raw) To: Benjamin Gaignard; +Cc: Dave Airlie, dri-devel, Linux Kernel Mailing List On Tue, Oct 16, 2018 at 8:30 PM Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote: > > Le lun. 15 oct. 2018 à 10:00, Daniel Vetter <daniel@ffwll.ch> a écrit : > > > > On Fri, Oct 12, 2018 at 11:46:39AM +0200, Benjamin Gaignard wrote: > > > Since drm_atomic_helper_shutdown() rework it is possible to do additional > > > clean up in sti driver: custom plane destroy functions become useless and > > > clean up encoder is no more needed. > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > > --- > > > drivers/gpu/drm/sti/sti_cursor.c | 9 +-------- > > > drivers/gpu/drm/sti/sti_gdp.c | 9 +-------- > > > drivers/gpu/drm/sti/sti_hqvdp.c | 9 +-------- > > > drivers/gpu/drm/sti/sti_tvout.c | 24 ------------------------ > > > 4 files changed, 3 insertions(+), 48 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c > > > index bc908453ffb3..e1ba253055c7 100644 > > > --- a/drivers/gpu/drm/sti/sti_cursor.c > > > +++ b/drivers/gpu/drm/sti/sti_cursor.c > > > @@ -328,13 +328,6 @@ static const struct drm_plane_helper_funcs sti_cursor_helpers_funcs = { > > > .atomic_disable = sti_cursor_atomic_disable, > > > }; > > > > > > -static void sti_cursor_destroy(struct drm_plane *drm_plane) > > > -{ > > > - DRM_DEBUG_DRIVER("\n"); > > > - > > > - drm_plane_cleanup(drm_plane); > > > -} > > > - > > > static int sti_cursor_late_register(struct drm_plane *drm_plane) > > > { > > > struct sti_plane *plane = to_sti_plane(drm_plane); > > > @@ -346,7 +339,7 @@ static int sti_cursor_late_register(struct drm_plane *drm_plane) > > > static const struct drm_plane_funcs sti_cursor_plane_helpers_funcs = { > > > .update_plane = drm_atomic_helper_update_plane, > > > .disable_plane = drm_atomic_helper_disable_plane, > > > - .destroy = sti_cursor_destroy, > > > + .destroy = drm_plane_cleanup, > > > .reset = sti_plane_reset, > > > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > > > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > > > diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c > > > index 3c19614d3f75..87b50451afd7 100644 > > > --- a/drivers/gpu/drm/sti/sti_gdp.c > > > +++ b/drivers/gpu/drm/sti/sti_gdp.c > > > @@ -879,13 +879,6 @@ static const struct drm_plane_helper_funcs sti_gdp_helpers_funcs = { > > > .atomic_disable = sti_gdp_atomic_disable, > > > }; > > > > > > -static void sti_gdp_destroy(struct drm_plane *drm_plane) > > > -{ > > > - DRM_DEBUG_DRIVER("\n"); > > > - > > > - drm_plane_cleanup(drm_plane); > > > -} > > > - > > > static int sti_gdp_late_register(struct drm_plane *drm_plane) > > > { > > > struct sti_plane *plane = to_sti_plane(drm_plane); > > > @@ -897,7 +890,7 @@ static int sti_gdp_late_register(struct drm_plane *drm_plane) > > > static const struct drm_plane_funcs sti_gdp_plane_helpers_funcs = { > > > .update_plane = drm_atomic_helper_update_plane, > > > .disable_plane = drm_atomic_helper_disable_plane, > > > - .destroy = sti_gdp_destroy, > > > + .destroy = drm_plane_cleanup, > > > .reset = sti_plane_reset, > > > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > > > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > > > diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c > > > index 23565f52dd71..065a5b08a702 100644 > > > --- a/drivers/gpu/drm/sti/sti_hqvdp.c > > > +++ b/drivers/gpu/drm/sti/sti_hqvdp.c > > > @@ -1256,13 +1256,6 @@ static const struct drm_plane_helper_funcs sti_hqvdp_helpers_funcs = { > > > .atomic_disable = sti_hqvdp_atomic_disable, > > > }; > > > > > > -static void sti_hqvdp_destroy(struct drm_plane *drm_plane) > > > -{ > > > - DRM_DEBUG_DRIVER("\n"); > > > - > > > - drm_plane_cleanup(drm_plane); > > > -} > > > - > > > static int sti_hqvdp_late_register(struct drm_plane *drm_plane) > > > { > > > struct sti_plane *plane = to_sti_plane(drm_plane); > > > @@ -1274,7 +1267,7 @@ static int sti_hqvdp_late_register(struct drm_plane *drm_plane) > > > static const struct drm_plane_funcs sti_hqvdp_plane_helpers_funcs = { > > > .update_plane = drm_atomic_helper_update_plane, > > > .disable_plane = drm_atomic_helper_disable_plane, > > > - .destroy = sti_hqvdp_destroy, > > > + .destroy = drm_plane_cleanup, > > > .reset = sti_plane_reset, > > > .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > > > .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > > > diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c > > > index ea4a3b87fa55..4dc3b2ec40eb 100644 > > > --- a/drivers/gpu/drm/sti/sti_tvout.c > > > +++ b/drivers/gpu/drm/sti/sti_tvout.c > > > @@ -788,21 +788,6 @@ static void sti_tvout_create_encoders(struct drm_device *dev, > > > tvout->dvo = sti_tvout_create_dvo_encoder(dev, tvout); > > > } > > > > > > -static void sti_tvout_destroy_encoders(struct sti_tvout *tvout) > > > -{ > > > - if (tvout->hdmi) > > > - drm_encoder_cleanup(tvout->hdmi); > > > - tvout->hdmi = NULL; > > > - > > > - if (tvout->hda) > > > - drm_encoder_cleanup(tvout->hda); > > > - tvout->hda = NULL; > > > - > > > - if (tvout->dvo) > > > - drm_encoder_cleanup(tvout->dvo); > > > - tvout->dvo = NULL; > > > -} > > > - > > > static int sti_tvout_bind(struct device *dev, struct device *master, void *data) > > > { > > > struct sti_tvout *tvout = dev_get_drvdata(dev); > > > @@ -815,17 +800,8 @@ static int sti_tvout_bind(struct device *dev, struct device *master, void *data) > > > return 0; > > > } > > > > > > -static void sti_tvout_unbind(struct device *dev, struct device *master, > > > - void *data) > > > -{ > > > - struct sti_tvout *tvout = dev_get_drvdata(dev); > > > - > > > - sti_tvout_destroy_encoders(tvout); > > > -} > > > - > > > static const struct component_ops sti_tvout_ops = { > > > .bind = sti_tvout_bind, > > > - .unbind = sti_tvout_unbind, > > > > Hm, this here looks strange now. I'd put a comment somewhere that > > master_ops->unbind cleans up everything. Either way: > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Hi Daniel, > > unbind undo what has been done in bind even the functions aren't symetric: > encoder creation are done is this level of the driver while they > destruction is done > in the top level of the driver by calling drm shutdown function. From > module point of view > bind and unbind are balanced correctly. > I have test it on board :-) Yes that's my understanding too, but then I stumbled over this revert (I cc'ed you on it): https://lore.kernel.org/patchwork/patch/1000524/ Now I'm not sure anymore whether my r-b is correct. What happens if only one of the components gets unbound, and not the top level? > I will not merge this patch until I get a clear review from you. You had one, but I just retracted it because of the revert ... -Daniel > > Regards, > Benjamin > > > > > }; > > > > > > static int sti_tvout_probe(struct platform_device *pdev) > > > -- > > > 2.15.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: make crct disable atomic 2018-10-12 9:46 [PATCH] drm/sti: make crct disable atomic Benjamin Gaignard 2018-10-12 9:46 ` [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework Benjamin Gaignard @ 2018-10-18 12:06 ` Benjamin Gaignard 1 sibling, 0 replies; 6+ messages in thread From: Benjamin Gaignard @ 2018-10-18 12:06 UTC (permalink / raw) To: David Airlie, Daniel Vetter; +Cc: ML dri-devel, Linux Kernel Mailing List Le ven. 12 oct. 2018 à 11:47, Benjamin Gaignard <benjamin.gaignard@linaro.org> a écrit : > > Wait until the next vblank to be sure that crtc has been disabled. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Applied on drm-misc-next > --- > drivers/gpu/drm/sti/sti_crtc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/sti/sti_crtc.c b/drivers/gpu/drm/sti/sti_crtc.c > index 5824e6aca8f4..61c2379fba87 100644 > --- a/drivers/gpu/drm/sti/sti_crtc.c > +++ b/drivers/gpu/drm/sti/sti_crtc.c > @@ -40,6 +40,8 @@ static void sti_crtc_atomic_disable(struct drm_crtc *crtc, > DRM_DEBUG_DRIVER("\n"); > > mixer->status = STI_MIXER_DISABLING; > + > + drm_crtc_wait_one_vblank(crtc); > } > > static int > -- > 2.15.0 > -- Benjamin Gaignard Graphic Study Group Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-18 12:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-12 9:46 [PATCH] drm/sti: make crct disable atomic Benjamin Gaignard 2018-10-12 9:46 ` [PATCH] drm/sti: clean up after drm_atomic_helper_shutdown rework Benjamin Gaignard 2018-10-15 8:00 ` Daniel Vetter 2018-10-16 18:30 ` Benjamin Gaignard 2018-10-16 19:15 ` Daniel Vetter 2018-10-18 12:06 ` [PATCH] drm/sti: make crct disable atomic Benjamin Gaignard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).