From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 92BBEC433E1 for ; Thu, 14 May 2020 20:55:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6294A20801 for ; Thu, 14 May 2020 20:55:50 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="P1QCTNzQ" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727976AbgENUzt (ORCPT ); Thu, 14 May 2020 16:55:49 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:47288 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726128AbgENUzt (ORCPT ); Thu, 14 May 2020 16:55:49 -0400 Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 4ADC626A; Thu, 14 May 2020 22:55:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1589489745; bh=f29cqelK5HHkGqAU0L84jjLxU3fDawEizqrCRSurEdI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=P1QCTNzQ/5H4u975GU1Ru/r7SadP8ZL0Iq1SFP9RpeARgWRAF8Z9vfAFjneDmM+88 aWK2GQgKb7lheaTULZrinb/L+Z+5BpM/KVdcFoqz9PRJxIv5XVn4jgifBABKRT8j39 YUOzvsR02Ih71Rt6++i4PDoRTscbaStUfcmylNBs= Date: Thu, 14 May 2020 23:55:36 +0300 From: Laurent Pinchart To: Enric Balletbo i Serra Cc: Chun-Kuang Hu , Enric Balletbo Serra , linux-kernel , Collabora Kernel ML , Nicolas Boichat , Philipp Zabel , David Airlie , dri-devel , "moderated list:ARM/Mediatek SoC support" , Daniel Vetter , Hsin-Yi Wang , Matthias Brugger , Sam Ravnborg , Linux ARM Subject: Re: [PATCH v4 7/7] drm/mediatek: mtk_dsi: Create connector for bridges Message-ID: <20200514205536.GN5955@pendragon.ideasonboard.com> References: <20200501152335.1805790-1-enric.balletbo@collabora.com> <20200501152335.1805790-8-enric.balletbo@collabora.com> <53683f2d-23c7-57ab-2056-520c50795ffe@collabora.com> <37191700-5832-2931-5764-7f7fddd023b9@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Enric, On Thu, May 14, 2020 at 07:35:33PM +0200, Enric Balletbo i Serra wrote: > On 14/5/20 19:12, Enric Balletbo i Serra wrote: > > On 14/5/20 18:44, Chun-Kuang Hu wrote: > >> Enric Balletbo i Serra 於 2020年5月14日 週四 下午11:42寫道： > >>> On 14/5/20 16:28, Chun-Kuang Hu wrote: > >>>> Enric Balletbo Serra 於 2020年5月14日 週四 上午12:41寫道： > >>>>> Missatge de Enric Balletbo i Serra del > >>>>> dia dv., 1 de maig 2020 a les 17:25: > >>>>>> > >>>>>> Use the drm_bridge_connector helper to create a connector for pipelines > >>>>>> that use drm_bridge. This allows splitting connector operations across > >>>>>> multiple bridges when necessary, instead of having the last bridge in > >>>>>> the chain creating the connector and handling all connector operations > >>>>>> internally. > >>>>>> > >>>>>> Signed-off-by: Enric Balletbo i Serra > >>>>>> Acked-by: Sam Ravnborg > >>>>> > >>>>> A gentle ping on this, I think that this one is the only one that > >>>>> still needs a review in the series. > >>>> > >>>> This is what I reply in patch v3: > >>> > >>> Sorry for missing this. > >>> > >>>> I think the panel is wrapped into next_bridge here, > >>> > >>> Yes, you can have for example: > >>> > >>> 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge > >>> (edp panel) > >>> > >>> or a > >>> > >>> 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) > >>> > >>> The _first_ one is my use case > >>> > >>>> if (panel) { > >>> > >>> This handles the second case, where you attach a dsi panel. > >>> > >>>> dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); > >>>> > >>>> so the next_bridge is a panel_bridge, in its attach function > >>>> panel_bridge_attach(), > >>>> according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, > >>>> it would create connector and attach connector to panel. > >>>> > >>>> I'm not sure this flag would exist or not, but for both case, it's strange. > >>>> If exist, you create connector in this patch but no where to attach > >>>> connector to panel. > >>> > >>> Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, > >>> mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() > >>> will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for > >>> dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The > >>> graphic controller driver should create connectors and CRTCs, as example you can > >>> take a look at drivers/gpu/drm/omapdrm/omap_drv.c > >> > >> I have such question because I've reviewed omap's driver. In omap's > >> driver, after it call drm_bridge_connector_init(), it does this: > >> > >> if (pipe->output->panel) { > >> ret = drm_panel_attach(pipe->output->panel, > >> pipe->connector); > >> if (ret < 0) > >> return ret; > >> } > >> > >> In this patch, you does not do this. > >> > > > > I see, so yes, I am probably missing call drm_panel_attach in case there is a > > direct panel attached. Thanks for pointing it. > > > > I'll send a new version adding the drm_panel_attach call. > > > > Wait, shouldn't panel be attached on the call of mtk_dsi_bridge_attach as > next_bridge points to a bridge or a panel? > > static int mtk_dsi_bridge_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > { > struct mtk_dsi *dsi = bridge_to_dsi(bridge); > > /* Attach the panel or bridge to the dsi bridge */ > return drm_bridge_attach(bridge->encoder, dsi->next_bridge, > &dsi->bridge, flags); > } > > Or I am continuing misunderstanding all this? Panels should always be wrapped in a drm_bridge, so I think you're doing right. I believe the call to drm_panel_attach() in omapdrm is a leftover that can be removed. I'll have a look at it. > >>>> If not exist, the next_brige would create one connector and this brige > >>>> would create another connector. > >>>> > >>>> I think in your case, mtk_dsi does not directly connect to a panel, so > >>> > >>> Exactly > >>> > >>>> I need a exact explain. Or someone could test this on a > >>>> directly-connect-panel platform. > >>> > >>> I don't think I am breaking this use case but AFAICS there is no users in > >>> mainline that directly connect a panel using the mediatek driver. As I said my > >>> use case is the other so I can't really test. Do you know anyone that can test this? > >> > >> I'm not sure who can test this, but [1], which is sent by YT Shen in a > >> series, is a patch to support dsi command mode so dsi could directly > >> connect to panel. > >> > >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.7-rc5&id=21898816831fc60c92dd634ab4316a24da7eb4af > >> > >> It's better that someone could test this case, but if no one would > >> test this, I could also accept a good-look patch. > >> > >>>>>> Changes in v4: None > >>>>>> Changes in v3: > >>>>>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) > >>>>>> > >>>>>> Changes in v2: None > >>>>>> > >>>>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- > >>>>>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > >>>>>> index 4f3bd095c1ee..471fcafdf348 100644 > >>>>>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > >>>>>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > >>>>>> @@ -17,6 +17,7 @@ > >>>>>> > >>>>>> #include > >>>>>> #include > >>>>>> +#include > >>>>>> #include > >>>>>> #include > >>>>>> #include > >>>>>> @@ -183,6 +184,7 @@ struct mtk_dsi { > >>>>>> struct drm_encoder encoder; > >>>>>> struct drm_bridge bridge; > >>>>>> struct drm_bridge *next_bridge; > >>>>>> + struct drm_connector *connector; > >>>>>> struct phy *phy; > >>>>>> > >>>>>> void __iomem *regs; > >>>>>> @@ -977,10 +979,19 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) > >>>>>> */ > >>>>>> dsi->encoder.possible_crtcs = 1; > >>>>>> > >>>>>> - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); > >>>>>> + ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > >>>>>> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > >>>>>> if (ret) > >>>>>> goto err_cleanup_encoder; > >>>>>> > >>>>>> + dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder); > >>>>>> + if (IS_ERR(dsi->connector)) { > >>>>>> + DRM_ERROR("Unable to create bridge connector\n"); > >>>>>> + ret = PTR_ERR(dsi->connector); > >>>>>> + goto err_cleanup_encoder; > >>>>>> + } > >>>>>> + drm_connector_attach_encoder(dsi->connector, &dsi->encoder); > >>>>>> + > >>>>>> return 0; > >>>>>> > >>>>>> err_cleanup_encoder: -- Regards, Laurent Pinchart