Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable
On Thu, Apr 25, 2019 at 08:59:15PM +0300, Laurent Pinchart wrote: > On Thu, Apr 25, 2019 at 12:39:27PM +, Matt Redfearn wrote: > > On 25/04/2019 13:13, Andrzej Hajda wrote: > >> On 24.04.2019 16:22, Matt Redfearn wrote: > >>> The DRM documentation states that post_disable is an optional callback. > >>> As such an implementing device may not populate it. To avoid panicing > >>> the kernel by calling a NULL function pointer, we should NULL check it > >>> before blindy calling it. > >>> > >>> Signed-off-by: Matt Redfearn > >> > >>> --- > >>> > >>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>> index 38e88071363..0ee440216b8 100644 > >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>> @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct > >>> drm_bridge *bridge) > >>>* This needs to be fixed in the drm_bridge framework and the > >>> API > >>>* needs to be updated to manage our own call chains... > >>>*/ > >>> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >>> + if (dsi->panel_bridge->funcs->post_disable) > >>> + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >>> > >> > >> Why not drm_bridge_post_disable ? > > > > Ah - that seems like a nicer fix! Do you think the comment above > > describing why this function pointer is called directly can be removed > > as well if we go this route? > > It shouldn't be necessary to call ->post_disable manually here as the > bridge core handles it internally. This is a hack to work around a > problem, and should be fixed properly. > > > If someone calls drm_bridge_post_disable() on the Synposys DSI > > drm_bridge it will go on to call post_disable on all other bridges in > > the chain, in addition to us calling them here. Is it an issue to call > > it multiple times? > > It depends on the panel implementation, but in general it's not a good > idea. It may happen to work, but could break at any time in the future. Double-checking the driver, the .attach() operation doesn't propagate to the next bridge, so the bridge core will not know about it, and will not propagate .post_disable() either. I think this should be fixed in a way that uses the drm bridge core infrastructure. > >>> if (dsi->slave) { > >>> dw_mipi_dsi_disable(dsi->slave); -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable
Hi Matt, On Thu, Apr 25, 2019 at 12:39:27PM +, Matt Redfearn wrote: > On 25/04/2019 13:13, Andrzej Hajda wrote: > > On 24.04.2019 16:22, Matt Redfearn wrote: > >> The DRM documentation states that post_disable is an optional callback. > >> As such an implementing device may not populate it. To avoid panicing > >> the kernel by calling a NULL function pointer, we should NULL check it > >> before blindy calling it. > >> > >> Signed-off-by: Matt Redfearn > > > >> --- > >> > >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> index 38e88071363..0ee440216b8 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct > >> drm_bridge *bridge) > >> * This needs to be fixed in the drm_bridge framework and the API > >> * needs to be updated to manage our own call chains... > >> */ > >> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >> + if (dsi->panel_bridge->funcs->post_disable) > >> + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >> > > > > Why not drm_bridge_post_disable ? > > Ah - that seems like a nicer fix! Do you think the comment above > describing why this function pointer is called directly can be removed > as well if we go this route? It shouldn't be necessary to call ->post_disable manually here as the bridge core handles it internally. This is a hack to work around a problem, and should be fixed properly. > If someone calls drm_bridge_post_disable() on the Synposys DSI > drm_bridge it will go on to call post_disable on all other bridges in > the chain, in addition to us calling them here. Is it an issue to call > it multiple times? It depends on the panel implementation, but in general it's not a good idea. It may happen to work, but could break at any time in the future. > >>if (dsi->slave) { > >>dw_mipi_dsi_disable(dsi->slave); -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable
Hi Andrzej, On 25/04/2019 13:13, Andrzej Hajda wrote: > On 24.04.2019 16:22, Matt Redfearn wrote: >> The DRM documentation states that post_disable is an optional callback. >> As such an implementing device may not populate it. To avoid panicing >> the kernel by calling a NULL function pointer, we should NULL check it >> before blindy calling it. >> >> Signed-off-by: Matt Redfearn > >> --- >> >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index 38e88071363..0ee440216b8 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct >> drm_bridge *bridge) >> * This needs to be fixed in the drm_bridge framework and the API >> * needs to be updated to manage our own call chains... >> */ >> -dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); >> +if (dsi->panel_bridge->funcs->post_disable) >> +dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); >> > > Why not drm_bridge_post_disable ? Ah - that seems like a nicer fix! Do you think the comment above describing why this function pointer is called directly can be removed as well if we go this route? If someone calls drm_bridge_post_disable() on the Synposys DSI drm_bridge it will go on to call post_disable on all other bridges in the chain, in addition to us calling them here. Is it an issue to call it multiple times? Thanks, Matt > > > Regards > > Andrzej > > >> if (dsi->slave) { >> dw_mipi_dsi_disable(dsi->slave); > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable
On 24.04.2019 16:22, Matt Redfearn wrote: > The DRM documentation states that post_disable is an optional callback. > As such an implementing device may not populate it. To avoid panicing > the kernel by calling a NULL function pointer, we should NULL check it > before blindy calling it. > > Signed-off-by: Matt Redfearn > --- > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index 38e88071363..0ee440216b8 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct > drm_bridge *bridge) >* This needs to be fixed in the drm_bridge framework and the API >* needs to be updated to manage our own call chains... >*/ > - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > + if (dsi->panel_bridge->funcs->post_disable) > + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > Why not drm_bridge_post_disable ? Regards Andrzej > if (dsi->slave) { > dw_mipi_dsi_disable(dsi->slave); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable
Hi Matt, Thank you for the patch. On Wed, Apr 24, 2019 at 02:22:00PM +, Matt Redfearn wrote: > The DRM documentation states that post_disable is an optional callback. > As such an implementing device may not populate it. To avoid panicing > the kernel by calling a NULL function pointer, we should NULL check it > before blindy calling it. > > Signed-off-by: Matt Redfearn Reviewed-by: Laurent Pinchart > --- > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index 38e88071363..0ee440216b8 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct > drm_bridge *bridge) >* This needs to be fixed in the drm_bridge framework and the API >* needs to be updated to manage our own call chains... >*/ > - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > + if (dsi->panel_bridge->funcs->post_disable) > + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > > if (dsi->slave) { > dw_mipi_dsi_disable(dsi->slave); -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable
The DRM documentation states that post_disable is an optional callback. As such an implementing device may not populate it. To avoid panicing the kernel by calling a NULL function pointer, we should NULL check it before blindy calling it. Signed-off-by: Matt Redfearn --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 38e88071363..0ee440216b8 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge) * This needs to be fixed in the drm_bridge framework and the API * needs to be updated to manage our own call chains... */ - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); + if (dsi->panel_bridge->funcs->post_disable) + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); if (dsi->slave) { dw_mipi_dsi_disable(dsi->slave); -- 2.17.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel