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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4AA66CE7A8C for ; Mon, 25 Sep 2023 15:29:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=n5qW05GAjjRGKKDTJPmMVUMXuqDznoMjsIDYId0uloU=; b=WOl4heCwo9gBIX imPkIeOGPCwITG8gUlWURu652fnTiyGJMedAHOW7SUC7sI+X+HAobdlsyQmAcz04DZum4q0MWpcdB lJ6pIs2cXVXIsbJgX9Ta5tFZThtkjgrrvY4MryMUMAY4hHgN4pCbS15ZYWKeL1G0VJQx5WDcc/1NN xJl7zSSgbTe774yBnG1+XAfzWvZ1bNUl0reX5nw4JtUsqWFPiJ29duo7lrvT8zcXzPYpOE8iL+6gz 6diZcVaTZ3dcUYkfSt9B8K8FZpIInqRkIlsKbpzBA9Ytr7qYgPPFjRTJIVhsF7Lg6RkrF+T6aCmfW /8F76Ey+ONZmk4PMuFhg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qknWa-00EYSr-1E; Mon, 25 Sep 2023 15:29:28 +0000 Received: from mail-ed1-x529.google.com ([2a00:1450:4864:20::529]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qknWX-00EYSU-28 for linux-arm-kernel@lists.infradead.org; Mon, 25 Sep 2023 15:29:27 +0000 Received: by mail-ed1-x529.google.com with SMTP id 4fb4d7f45d1cf-533f193fc8dso3273198a12.2 for ; Mon, 25 Sep 2023 08:29:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695655763; x=1696260563; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=zOrfKssBZ6tPiXkBwLOUz8n2rdUkUTBa6TNU5TWDfM0=; b=FkQ5Dx+47Av5X+MnQuz32bRPBBJiH1V4B36R6uYxF4C4eiNoPUV4f84/t4HVoXJWMs PeBWv1CVu7iXL4YpOXaBN/4YT6lSEvGFbVsram/39HNKz5PpAhtdT5i6hkAPQjHWM3vE RiQBDLAbIJWhj0Gob7gIv6BFzmGPSBzDS/79SxfD/3W4QyHJn6HL+qQFAJTuWhG0BJ9U VIGU5vP0fVeeX5I1iLQmeKe4qG/mI4HmJ7NDsKnYWK40IafWOVe4vdT8UGyKZCdXgSDM X/gIpPiCVege4Ml9Xhk8uUusyTW2ADSnA/OdSrHA2qSR2iuCPtc70OfLGs8Wf5Cl3+Vc gMAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695655763; x=1696260563; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=zOrfKssBZ6tPiXkBwLOUz8n2rdUkUTBa6TNU5TWDfM0=; b=CH1fqZoL3iCWhlk3USrADdmNA2Qx5IrZHHuvFGQn7gY2Fy5otHEvboO9WZZ5xoBNHY uvmO9QaBNyDq6oJy6oGGJEAUuyiz2qIrb0utSVvX2USuenf3HpJluAE/DylImHvxje/x H1X9dYfUcvcAPNghcJJ9H/W9wQifZhNDFkTZU5FFqSekUdD+A97r5PXpljqu9FGHSYsP ETDwnGcPsNoKObvZHWNHnZhfRT3uS0h+lnWTT+YIOmkKI/nSQRw1gMah44MJdc2sU5Kh wb/JgxW4V8IxAaUFqJSTLGCiFbuWxKBUrogT/R9bUeH8mTp6yEbyYorbB/+SSvFwpr14 KnTw== X-Gm-Message-State: AOJu0YxL2yasCsv9fERlAgbj29TK3cf+7P1SfssQHZ0VqbXHOrWllq2D 6m97WelWcYyU9jDaKWTxzEM6xPKIPg3/NEJd X-Google-Smtp-Source: AGHT+IHY89kh3JJby1pbvhe6WVwOjRasFpbMMz9jFOaCxEh+NVsguyRHHY9tQsRWFSkQWTar4MEUog== X-Received: by 2002:a17:906:8b:b0:9a2:474:4aa0 with SMTP id 11-20020a170906008b00b009a204744aa0mr6407443ejc.48.1695655762414; Mon, 25 Sep 2023 08:29:22 -0700 (PDT) Received: from jernej-laptop.localnet (82-149-12-148.dynamic.telemach.net. [82.149.12.148]) by smtp.gmail.com with ESMTPSA id qw25-20020a1709066a1900b009b293d1f2eesm553884ejc.107.2023.09.25.08.29.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Sep 2023 08:29:21 -0700 (PDT) From: Jernej =?utf-8?B?xaBrcmFiZWM=?= To: Maxime Ripard Cc: wens@csie.org, airlied@gmail.com, daniel@ffwll.ch, samuel@sholland.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/7] drm/sun4i: dw-hdmi: Switch to bridge functions Date: Mon, 25 Sep 2023 17:29:20 +0200 Message-ID: <2167865.Icojqenx9y@jernej-laptop> In-Reply-To: References: <20230924192604.3262187-1-jernej.skrabec@gmail.com> <20230924192604.3262187-4-jernej.skrabec@gmail.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230925_082925_725156_2F54D15E X-CRM114-Status: GOOD ( 38.97 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Dne ponedeljek, 25. september 2023 ob 09:57:22 CEST je Maxime Ripard napisal(a): > Hi, > > On Sun, Sep 24, 2023 at 09:26:00PM +0200, Jernej Skrabec wrote: > > Since ddc-en property handling was moved from sun8i dw-hdmi driver to > > display connector driver, probe order of drivers determines if EDID is > > properly read at boot time or not. > > > > In order to fix this, let's switch to bridge functions which allows us > > to build proper chain and defer execution until all drivers are probed. > > > > Fixes: 920169041baa ("drm/sun4i: dw-hdmi: Fix ddc-en GPIO consumer conflict") > > Signed-off-by: Jernej Skrabec > > --- > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 114 +++++++++++++++++++++++++- > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 5 ++ > > 2 files changed, 117 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > index 8f8d3bdba5ce..93831cdf1917 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > @@ -8,14 +8,82 @@ > > #include > > #include > > > > +#include > > +#include > > #include > > #include > > #include > > #include > > > > +#include > > + > > #include "sun8i_dw_hdmi.h" > > #include "sun8i_tcon_top.h" > > > > +#define bridge_to_sun8i_dw_hdmi(x) \ > > + container_of(x, struct sun8i_dw_hdmi, enc_bridge) > > + > > +static int sun8i_hdmi_enc_attach(struct drm_bridge *bridge, > > + enum drm_bridge_attach_flags flags) > > +{ > > + struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge); > > + > > + return drm_bridge_attach(&hdmi->encoder, hdmi->hdmi_bridge, > > + &hdmi->enc_bridge, flags); > > +} > > + > > +static void sun8i_hdmi_enc_detach(struct drm_bridge *bridge) > > +{ > > + struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge); > > + > > + cec_notifier_conn_unregister(hdmi->cec_notifier); > > + hdmi->cec_notifier = NULL; > > +} > > + > > +static void sun8i_hdmi_enc_hpd_notify(struct drm_bridge *bridge, > > + enum drm_connector_status status) > > +{ > > + struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge); > > + struct edid *edid; > > + > > + if (!hdmi->cec_notifier) > > + return; > > + > > + if (status == connector_status_connected) { > > + edid = drm_bridge_get_edid(hdmi->hdmi_bridge, hdmi->connector); > > + if (edid) > > + cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, > > + edid); > > + } else { > > + cec_notifier_phys_addr_invalidate(hdmi->cec_notifier); > > + } > > +} > > + > > +static int sun8i_hdmi_enc_atomic_check(struct drm_bridge *bridge, > > + struct drm_bridge_state *bridge_state, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state) > > +{ > > + struct drm_connector_state *old_conn_state = > > + drm_atomic_get_old_connector_state(conn_state->state, > > + conn_state->connector); > > + > > + if (!drm_connector_atomic_hdr_metadata_equal(old_conn_state, conn_state)) > > + crtc_state->mode_changed = true; > > + > > + return 0; > > +} > > + > > +static const struct drm_bridge_funcs sun8i_hdmi_enc_bridge_funcs = { > > + .attach = sun8i_hdmi_enc_attach, > > + .detach = sun8i_hdmi_enc_detach, > > + .hpd_notify = sun8i_hdmi_enc_hpd_notify, > > + .atomic_check = sun8i_hdmi_enc_atomic_check, > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > + .atomic_reset = drm_atomic_helper_bridge_reset, > > +}; > > + > > static void sun8i_dw_hdmi_encoder_mode_set(struct drm_encoder *encoder, > > struct drm_display_mode *mode, > > struct drm_display_mode *adj_mode) > > @@ -99,6 +167,8 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master, > > { > > struct platform_device *pdev = to_platform_device(dev); > > struct dw_hdmi_plat_data *plat_data; > > + struct cec_connector_info conn_info; > > + struct drm_connector *connector; > > struct drm_device *drm = data; > > struct device_node *phy_node; > > struct drm_encoder *encoder; > > @@ -187,18 +257,57 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master, > > > > plat_data->mode_valid = hdmi->quirks->mode_valid; > > plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe; > > + plat_data->output_port = 1; > > sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data); > > > > platform_set_drvdata(pdev, hdmi); > > > > - hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); > > + hdmi->hdmi = dw_hdmi_probe(pdev, plat_data); > > if (IS_ERR(hdmi->hdmi)) { > > ret = PTR_ERR(hdmi->hdmi); > > goto err_deinit_phy; > > } > > > > + hdmi->hdmi_bridge = of_drm_find_bridge(dev->of_node); > > So you're also adding child bridge support? This should be mentioned in > the commit log. > > > + hdmi->enc_bridge.funcs = &sun8i_hdmi_enc_bridge_funcs; > > + hdmi->enc_bridge.type = DRM_MODE_CONNECTOR_HDMIA; > > + hdmi->enc_bridge.interlace_allowed = true; > > + > > + drm_bridge_add(&hdmi->enc_bridge); > > + > > + ret = drm_bridge_attach(encoder, &hdmi->enc_bridge, NULL, > > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > + if (ret) > > + goto err_remove_dw_hdmi; > > + > > + connector = drm_bridge_connector_init(drm, encoder); > > + if (IS_ERR(connector)) { > > + dev_err(dev, "Unable to create HDMI bridge connector\n"); > > + ret = PTR_ERR(connector); > > + goto err_remove_dw_hdmi; > > + } > > + > > + hdmi->connector = connector; > > + drm_connector_attach_encoder(connector, encoder); > > + > > + if (hdmi->quirks->use_drm_infoframe) > > + drm_connector_attach_hdr_output_metadata_property(connector); > > + > > + cec_fill_conn_info_from_drm(&conn_info, connector); > > + > > + hdmi->cec_notifier = cec_notifier_conn_register(&pdev->dev, NULL, > > + &conn_info); > > + if (!hdmi->cec_notifier) { > > + ret = -ENOMEM; > > + goto err_remove_dw_hdmi; > > + } > > + > > return 0; > > Yeah, I'm not sure. We now have two different yet redundant set of > operations with no clear definition wrt what belongs where. I'm really > not impressed with the use of bridges for things that are clearly not > bridges. DRM bridges should be taken as more broad concept than just a physical bridge. > > The "nothing happens until all drivers are loaded" argument is also > supposed to be covered by the component framework, so why do we even > have to use a bridge here? There is more than one reason: - Display connector driver, which sets ddc-en gpio, is purely bridge driver. - In long term, I plan to add format negotiation between display, dw-hdmi and DE3 (I already have WIP code). This is already implemented in bridge infrastructure. - There is a plan to remove connector handling from DW HDMI handling and use display connector driver instead. This again involves bridges. sun4i-drm and rockchip-drm are the only remaining drivers, which are not yet converted. > > You were saying that it's an issue of probe order going wrong, could you > explain a bit more what goes wrong so we can try to figure something out > that doesn't involve a bridge? Not sure how to make this clearer than in cover letter and this commit message. ddc-en pin is responsibility of display-connector driver (see commit mentioned in fixes tag), so it must probe before sun8i dw hdmi. Why are you so against bridges? As I explained above, format negotiation is really implemented only there, so they are needed at some point anyway. Best regards, Jernej _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel