From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable Date: Thu, 25 Apr 2019 21:02:51 +0300 Message-ID: <20190425180251.GC12024@pendragon.ideasonboard.com> References: <20190424142148.25927-1-matt.redfearn@thinci.com> <3da4c0a3-a77b-5128-63f7-9c8d6b012c68@samsung.com> <95c36782-7eaf-2224-bb52-cff0a53d98d8@thinci.com> <20190425175915.GB12024@pendragon.ideasonboard.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Content-Disposition: inline In-Reply-To: <20190425175915.GB12024@pendragon.ideasonboard.com> Sender: linux-kernel-owner@vger.kernel.org To: Matt Redfearn Cc: Andrzej Hajda , Philippe Cornu , "dri-devel@lists.freedesktop.org" , Matthew Redfearn , Nickey Yang , Heiko Stuebner , Archit Taneja , "linux-kernel@vger.kernel.org" , David Airlie , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org On Thu, Apr 25, 2019 at 08:59:15PM +0300, Laurent Pinchart wrote: > On Thu, Apr 25, 2019 at 12:39:27PM +0000, 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