| Message ID | 20230306103533.4915-1-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/meson: fix missing component unbind on bind errors | expand |
Hi Johan, thanks for your patch! On Mon, Mar 6, 2023 at 11:35 AM Johan Hovold <johan+linaro@kernel.org> wrote: [...] > @@ -325,23 +325,23 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) > > ret = meson_encoder_hdmi_init(priv); I'm wondering if component_bind_all() can be moved further down. Right now it's between meson_encoder_cvbs_init() and meson_encoder_hdmi_init(). So it seems that encoders don't rely on component registration. Unfortunately I am also not familiar with this and I'm hoping that Neil can comment on this. Best regards, Martin
On Thu, Mar 09, 2023 at 10:41:18PM +0100, Martin Blumenstingl wrote: > On Mon, Mar 6, 2023 at 11:35 AM Johan Hovold <johan+linaro@kernel.org> wrote: > [...] > > @@ -325,23 +325,23 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) > > > > ret = meson_encoder_hdmi_init(priv); > I'm wondering if component_bind_all() can be moved further down. > Right now it's between meson_encoder_cvbs_init() and > meson_encoder_hdmi_init(). So it seems that encoders don't rely on > component registration. Perhaps it can, but that would be a separate change (unless there is something inherently wrong with the current initialisation order). > Unfortunately I am also not familiar with this and I'm hoping that > Neil can comment on this. Any comments on this one, Neil? Johan
On 21/03/2023 14:14, Johan Hovold wrote: > On Thu, Mar 09, 2023 at 10:41:18PM +0100, Martin Blumenstingl wrote: > >> On Mon, Mar 6, 2023 at 11:35 AM Johan Hovold <johan+linaro@kernel.org> wrote: >> [...] >>> @@ -325,23 +325,23 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) >>> >>> ret = meson_encoder_hdmi_init(priv); > >> I'm wondering if component_bind_all() can be moved further down. >> Right now it's between meson_encoder_cvbs_init() and >> meson_encoder_hdmi_init(). So it seems that encoders don't rely on >> component registration. > > Perhaps it can, but that would be a separate change (unless there is > something inherently wrong with the current initialisation order). The CVBS doesn't rely on any components unlike HDMI, this explains the current position of component_bind_all(). > >> Unfortunately I am also not familiar with this and I'm hoping that >> Neil can comment on this. > > Any comments on this one, Neil? Yep, components should be bind for HDMI encoder/bridge init. Anyway for this patch, sorry for the delay, but it's looks fine: Acked-by: Neil Armstrong <neil.armstrong@linaro.org> > > Johan
On Tue, Mar 21, 2023 at 02:27:08PM +0100, Neil Armstrong wrote: > Anyway for this patch, sorry for the delay, but it's looks fine: > > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> Thanks for reviewing! Johan
Hi, On Mon, 06 Mar 2023 11:35:33 +0100, Johan Hovold wrote: > Make sure to unbind all subcomponents when binding the aggregate device > fails. > > Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-fixes) [1/1] drm/meson: fix missing component unbind on bind errors https://cgit.freedesktop.org/drm/drm-misc/commit/?id=ba98413bf45edbf33672e2539e321b851b2cfbd1
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index 79bfe3938d3c..7caf937c3c90 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -325,23 +325,23 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) ret = meson_encoder_hdmi_init(priv); if (ret) - goto exit_afbcd; + goto unbind_all; ret = meson_plane_create(priv); if (ret) - goto exit_afbcd; + goto unbind_all; ret = meson_overlay_create(priv); if (ret) - goto exit_afbcd; + goto unbind_all; ret = meson_crtc_create(priv); if (ret) - goto exit_afbcd; + goto unbind_all; ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, drm); if (ret) - goto exit_afbcd; + goto unbind_all; drm_mode_config_reset(drm); @@ -359,6 +359,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) uninstall_irq: free_irq(priv->vsync_irq, drm); +unbind_all: + if (has_components) + component_unbind_all(drm->dev, drm); exit_afbcd: if (priv->afbcd.ops) priv->afbcd.ops->exit(priv);
Make sure to unbind all subcomponents when binding the aggregate device fails. Fixes: a41e82e6c457 ("drm/meson: Add support for components") Cc: stable@vger.kernel.org # 4.12 Cc: Neil Armstrong <neil.armstrong@linaro.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- Note that this one has only been compile tested. Johan drivers/gpu/drm/meson/meson_drv.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)