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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 27AFEC433F5 for ; Fri, 15 Oct 2021 22:25:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EF3B36115C for ; Fri, 15 Oct 2021 22:25:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238875AbhJOW1W (ORCPT ); Fri, 15 Oct 2021 18:27:22 -0400 Received: from m43-7.mailgun.net ([69.72.43.7]:11648 "EHLO m43-7.mailgun.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231343AbhJOW1V (ORCPT ); Fri, 15 Oct 2021 18:27:21 -0400 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1634336714; h=Message-ID: References: In-Reply-To: Subject: Cc: To: From: Date: Content-Transfer-Encoding: Content-Type: MIME-Version: Sender; bh=4Z17o6a3w+quMxd2DnT/ckDhlwbtd0buMDjBfJo7lWI=; b=qGiuDg2TPsj0dUWEA2PmYAQpmm4PG4Qnl6DkihWz1oDGLcFmu6WTHS7DjCrQ1/uaKbah2jEw kK5qOHtXVMZhuS/vHZ3dvS9u1QKedQ5vAW0/OXUVASMkBp+l0fgpf/khG9u4Sa+ra1ckyXY8 b1DVV58rATNt20/H3JEIS6TzIO0= X-Mailgun-Sending-Ip: 69.72.43.7 X-Mailgun-Sid: WyI1MzIzYiIsICJsaW51eC1hcm0tbXNtQHZnZXIua2VybmVsLm9yZyIsICJiZTllNGEiXQ== Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by smtp-out-n06.prod.us-west-2.postgun.com with SMTP id 6169ffc230ce13d2b4838e02 (version=TLS1.2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256); Fri, 15 Oct 2021 22:25:06 GMT Sender: abhinavk=codeaurora.org@mg.codeaurora.org Received: by smtp.codeaurora.org (Postfix, from userid 1001) id 1516CC4361A; Fri, 15 Oct 2021 22:25:06 +0000 (UTC) Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: abhinavk) by smtp.codeaurora.org (Postfix) with ESMTPSA id 124F2C43460; Fri, 15 Oct 2021 22:25:04 +0000 (UTC) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 15 Oct 2021 15:25:03 -0700 From: abhinavk@codeaurora.org To: Dmitry Baryshkov Cc: Bjorn Andersson , Rob Clark , Sean Paul , Jonathan Marek , Stephen Boyd , David Airlie , Daniel Vetter , linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org Subject: Re: [Freedreno] [PATCH 2/2] drm/msm/hdmi: switch to drm_bridge_connector In-Reply-To: <20211015001100.4193241-2-dmitry.baryshkov@linaro.org> References: <20211015001100.4193241-1-dmitry.baryshkov@linaro.org> <20211015001100.4193241-2-dmitry.baryshkov@linaro.org> Message-ID: <8d90118d26b0e36fff84dc32ab047dd8@codeaurora.org> X-Sender: abhinavk@codeaurora.org User-Agent: Roundcube Webmail/1.3.9 Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org Hi Dmitry On 2021-10-14 17:11, Dmitry Baryshkov wrote: > Merge old hdmi_bridge and hdmi_connector implementations. Use > drm_bridge_connector instead. > Can you please comment on the validation done on this change? Has basic bootup been verified on db820c as thats the only platform which shall use this. > Signed-off-by: Dmitry Baryshkov > --- > drivers/gpu/drm/msm/Makefile | 2 +- > drivers/gpu/drm/msm/hdmi/hdmi.c | 12 +- > drivers/gpu/drm/msm/hdmi/hdmi.h | 19 ++- > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 81 ++++++++- > .../msm/hdmi/{hdmi_connector.c => hdmi_hpd.c} | 154 ++---------------- > 5 files changed, 109 insertions(+), 159 deletions(-) > rename drivers/gpu/drm/msm/hdmi/{hdmi_connector.c => hdmi_hpd.c} (62%) > > diff --git a/drivers/gpu/drm/msm/Makefile > b/drivers/gpu/drm/msm/Makefile > index 904535eda0c4..91b09cda8a9c 100644 > --- a/drivers/gpu/drm/msm/Makefile > +++ b/drivers/gpu/drm/msm/Makefile > @@ -19,7 +19,7 @@ msm-y := \ > hdmi/hdmi.o \ > hdmi/hdmi_audio.o \ > hdmi/hdmi_bridge.o \ > - hdmi/hdmi_connector.o \ > + hdmi/hdmi_hpd.o \ > hdmi/hdmi_i2c.o \ > hdmi/hdmi_phy.o \ > hdmi/hdmi_phy_8960.o \ > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c > b/drivers/gpu/drm/msm/hdmi/hdmi.c > index db17a000d968..d1cf4df7188c 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c > @@ -8,6 +8,8 @@ > #include > #include > > +#include > + > #include > #include "hdmi.h" > > @@ -41,7 +43,7 @@ static irqreturn_t msm_hdmi_irq(int irq, void > *dev_id) > struct hdmi *hdmi = dev_id; > > /* Process HPD: */ > - msm_hdmi_connector_irq(hdmi->connector); > + msm_hdmi_hpd_irq(hdmi->bridge); > > /* Process DDC: */ > msm_hdmi_i2c_irq(hdmi->i2c); > @@ -283,7 +285,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi, > goto fail; > } > > - hdmi->connector = msm_hdmi_connector_init(hdmi); > + hdmi->connector = drm_bridge_connector_init(hdmi->dev, encoder); > if (IS_ERR(hdmi->connector)) { > ret = PTR_ERR(hdmi->connector); > DRM_DEV_ERROR(dev->dev, "failed to create HDMI connector: %d\n", > ret); > @@ -291,6 +293,8 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi, > goto fail; > } > > + drm_connector_attach_encoder(hdmi->connector, hdmi->encoder); > + > hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > if (hdmi->irq < 0) { > ret = hdmi->irq; > @@ -307,7 +311,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi, > goto fail; > } > > - ret = msm_hdmi_hpd_enable(hdmi->connector); > + drm_bridge_connector_enable_hpd(hdmi->connector); > + > + ret = msm_hdmi_hpd_enable(hdmi->bridge); > if (ret < 0) { > DRM_DEV_ERROR(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret); > goto fail; > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h > b/drivers/gpu/drm/msm/hdmi/hdmi.h > index 82261078c6b1..736f348befb3 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi.h > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h > @@ -114,6 +114,13 @@ struct hdmi_platform_config { > struct hdmi_gpio_data gpios[HDMI_MAX_NUM_GPIO]; > }; > > +struct hdmi_bridge { > + struct drm_bridge base; > + struct hdmi *hdmi; > + struct work_struct hpd_work; > +}; > +#define to_hdmi_bridge(x) container_of(x, struct hdmi_bridge, base) > + > void msm_hdmi_set_mode(struct hdmi *hdmi, bool power_on); > > static inline void hdmi_write(struct hdmi *hdmi, u32 reg, u32 data) > @@ -230,13 +237,11 @@ void msm_hdmi_audio_set_sample_rate(struct hdmi > *hdmi, int rate); > struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi); > void msm_hdmi_bridge_destroy(struct drm_bridge *bridge); > > -/* > - * hdmi connector: > - */ > - > -void msm_hdmi_connector_irq(struct drm_connector *connector); > -struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi); > -int msm_hdmi_hpd_enable(struct drm_connector *connector); > +void msm_hdmi_hpd_irq(struct drm_bridge *bridge); > +enum drm_connector_status msm_hdmi_bridge_detect( > + struct drm_bridge *bridge); > +int msm_hdmi_hpd_enable(struct drm_bridge *bridge); > +void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge); > > /* > * i2c adapter for ddc: > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > index f04eb4a70f0d..211b73dddf65 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > @@ -5,17 +5,16 @@ > */ > > #include > +#include > > +#include "msm_kms.h" > #include "hdmi.h" > > -struct hdmi_bridge { > - struct drm_bridge base; > - struct hdmi *hdmi; > -}; > -#define to_hdmi_bridge(x) container_of(x, struct hdmi_bridge, base) > - > void msm_hdmi_bridge_destroy(struct drm_bridge *bridge) > { > + struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > + > + msm_hdmi_hpd_disable(hdmi_bridge); > } > > static void msm_hdmi_power_on(struct drm_bridge *bridge) > @@ -251,14 +250,76 @@ static void msm_hdmi_bridge_mode_set(struct > drm_bridge *bridge, > msm_hdmi_audio_update(hdmi); > } > > +static struct edid *msm_hdmi_bridge_get_edid(struct drm_bridge > *bridge, > + struct drm_connector *connector) > +{ > + struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > + struct hdmi *hdmi = hdmi_bridge->hdmi; > + struct edid *edid; > + uint32_t hdmi_ctrl; > + > + hdmi_ctrl = hdmi_read(hdmi, REG_HDMI_CTRL); > + hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl | HDMI_CTRL_ENABLE); > + > + edid = drm_get_edid(connector, hdmi->i2c); > + > + hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl); > + > + hdmi->hdmi_mode = drm_detect_hdmi_monitor(edid); > + > + return edid; > +} > + > +static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct > drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > + struct hdmi *hdmi = hdmi_bridge->hdmi; > + const struct hdmi_platform_config *config = hdmi->config; > + struct msm_drm_private *priv = bridge->dev->dev_private; > + struct msm_kms *kms = priv->kms; > + long actual, requested; > + > + requested = 1000 * mode->clock; > + actual = kms->funcs->round_pixclk(kms, > + requested, hdmi_bridge->hdmi->encoder); > + > + /* for mdp5/apq8074, we manage our own pixel clk (as opposed to > + * mdp4/dtv stuff where pixel clk is assigned to mdp/encoder > + * instead): > + */ > + if (config->pwr_clk_cnt > 0) > + actual = clk_round_rate(hdmi->pwr_clks[0], actual); > + > + DBG("requested=%ld, actual=%ld", requested, actual); > + > + if (actual != requested) > + return MODE_CLOCK_RANGE; > + > + return 0; > +} > + > static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = { > .pre_enable = msm_hdmi_bridge_pre_enable, > .enable = msm_hdmi_bridge_enable, > .disable = msm_hdmi_bridge_disable, > .post_disable = msm_hdmi_bridge_post_disable, > .mode_set = msm_hdmi_bridge_mode_set, > + .mode_valid = msm_hdmi_bridge_mode_valid, > + .get_edid = msm_hdmi_bridge_get_edid, > + .detect = msm_hdmi_bridge_detect, > }; > > +static void > +msm_hdmi_hotplug_work(struct work_struct *work) > +{ > + struct hdmi_bridge *hdmi_bridge = > + container_of(work, struct hdmi_bridge, hpd_work); > + struct drm_bridge *bridge = &hdmi_bridge->base; > + > + drm_bridge_hpd_notify(bridge, drm_bridge_detect(bridge)); > +} > > /* initialize bridge */ > struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi) > @@ -275,11 +336,17 @@ struct drm_bridge *msm_hdmi_bridge_init(struct > hdmi *hdmi) > } > > hdmi_bridge->hdmi = hdmi; > + INIT_WORK(&hdmi_bridge->hpd_work, msm_hdmi_hotplug_work); > > bridge = &hdmi_bridge->base; > bridge->funcs = &msm_hdmi_bridge_funcs; > + bridge->ddc = hdmi->i2c; > + bridge->type = DRM_MODE_CONNECTOR_HDMIA; > + bridge->ops = DRM_BRIDGE_OP_HPD | > + DRM_BRIDGE_OP_DETECT | > + DRM_BRIDGE_OP_EDID; > > - ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, 0); > + ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > if (ret) > goto fail; > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c > b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c > similarity index 62% > rename from drivers/gpu/drm/msm/hdmi/hdmi_connector.c > rename to drivers/gpu/drm/msm/hdmi/hdmi_hpd.c > index a7f729cdec7b..1cda7bf23b3b 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c > @@ -11,13 +11,6 @@ > #include "msm_kms.h" > #include "hdmi.h" > > -struct hdmi_connector { > - struct drm_connector base; > - struct hdmi *hdmi; > - struct work_struct hpd_work; > -}; > -#define to_hdmi_connector(x) container_of(x, struct hdmi_connector, > base) > - > static void msm_hdmi_phy_reset(struct hdmi *hdmi) > { > unsigned int val; > @@ -139,10 +132,10 @@ static void enable_hpd_clocks(struct hdmi *hdmi, > bool enable) > } > } > > -int msm_hdmi_hpd_enable(struct drm_connector *connector) > +int msm_hdmi_hpd_enable(struct drm_bridge *bridge) > { > - struct hdmi_connector *hdmi_connector = to_hdmi_connector(connector); > - struct hdmi *hdmi = hdmi_connector->hdmi; > + struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > + struct hdmi *hdmi = hdmi_bridge->hdmi; > const struct hdmi_platform_config *config = hdmi->config; > struct device *dev = &hdmi->pdev->dev; > uint32_t hpd_ctrl; > @@ -199,9 +192,9 @@ int msm_hdmi_hpd_enable(struct drm_connector > *connector) > return ret; > } > > -static void hdp_disable(struct hdmi_connector *hdmi_connector) > +void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge) > { > - struct hdmi *hdmi = hdmi_connector->hdmi; > + struct hdmi *hdmi = hdmi_bridge->hdmi; > const struct hdmi_platform_config *config = hdmi->config; > struct device *dev = &hdmi->pdev->dev; > int ret; > @@ -227,19 +220,10 @@ static void hdp_disable(struct hdmi_connector > *hdmi_connector) > dev_warn(dev, "failed to disable hpd regulator: %d\n", ret); > } > > -static void > -msm_hdmi_hotplug_work(struct work_struct *work) > -{ > - struct hdmi_connector *hdmi_connector = > - container_of(work, struct hdmi_connector, hpd_work); > - struct drm_connector *connector = &hdmi_connector->base; > - drm_helper_hpd_irq_event(connector->dev); > -} > - > -void msm_hdmi_connector_irq(struct drm_connector *connector) > +void msm_hdmi_hpd_irq(struct drm_bridge *bridge) > { > - struct hdmi_connector *hdmi_connector = to_hdmi_connector(connector); > - struct hdmi *hdmi = hdmi_connector->hdmi; > + struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > + struct hdmi *hdmi = hdmi_bridge->hdmi; > uint32_t hpd_int_status, hpd_int_ctrl; > > /* Process HPD: */ > @@ -262,7 +246,7 @@ void msm_hdmi_connector_irq(struct drm_connector > *connector) > hpd_int_ctrl |= HDMI_HPD_INT_CTRL_INT_CONNECT; > hdmi_write(hdmi, REG_HDMI_HPD_INT_CTRL, hpd_int_ctrl); > > - queue_work(hdmi->workq, &hdmi_connector->hpd_work); > + queue_work(hdmi->workq, &hdmi_bridge->hpd_work); > } > } > > @@ -293,11 +277,11 @@ static enum drm_connector_status > detect_gpio(struct hdmi *hdmi) > connector_status_disconnected; > } > > -static enum drm_connector_status hdmi_connector_detect( > - struct drm_connector *connector, bool force) > +enum drm_connector_status msm_hdmi_bridge_detect( > + struct drm_bridge *bridge) > { > - struct hdmi_connector *hdmi_connector = to_hdmi_connector(connector); > - struct hdmi *hdmi = hdmi_connector->hdmi; > + struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > + struct hdmi *hdmi = hdmi_bridge->hdmi; > const struct hdmi_platform_config *config = hdmi->config; > struct hdmi_gpio_data hpd_gpio = config->gpios[HPD_GPIO_INDEX]; > enum drm_connector_status stat_gpio, stat_reg; > @@ -331,115 +315,3 @@ static enum drm_connector_status > hdmi_connector_detect( > > return stat_gpio; > } > - > -static void hdmi_connector_destroy(struct drm_connector *connector) > -{ > - struct hdmi_connector *hdmi_connector = to_hdmi_connector(connector); > - > - hdp_disable(hdmi_connector); > - > - drm_connector_cleanup(connector); > - > - kfree(hdmi_connector); > -} > - > -static int msm_hdmi_connector_get_modes(struct drm_connector > *connector) > -{ > - struct hdmi_connector *hdmi_connector = to_hdmi_connector(connector); > - struct hdmi *hdmi = hdmi_connector->hdmi; > - struct edid *edid; > - uint32_t hdmi_ctrl; > - int ret = 0; > - > - hdmi_ctrl = hdmi_read(hdmi, REG_HDMI_CTRL); > - hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl | HDMI_CTRL_ENABLE); > - > - edid = drm_get_edid(connector, hdmi->i2c); > - > - hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl); > - > - hdmi->hdmi_mode = drm_detect_hdmi_monitor(edid); > - drm_connector_update_edid_property(connector, edid); > - > - if (edid) { > - ret = drm_add_edid_modes(connector, edid); > - kfree(edid); > - } > - > - return ret; > -} > - > -static int msm_hdmi_connector_mode_valid(struct drm_connector > *connector, > - struct drm_display_mode *mode) > -{ > - struct hdmi_connector *hdmi_connector = to_hdmi_connector(connector); > - struct hdmi *hdmi = hdmi_connector->hdmi; > - const struct hdmi_platform_config *config = hdmi->config; > - struct msm_drm_private *priv = connector->dev->dev_private; > - struct msm_kms *kms = priv->kms; > - long actual, requested; > - > - requested = 1000 * mode->clock; > - actual = kms->funcs->round_pixclk(kms, > - requested, hdmi_connector->hdmi->encoder); > - > - /* for mdp5/apq8074, we manage our own pixel clk (as opposed to > - * mdp4/dtv stuff where pixel clk is assigned to mdp/encoder > - * instead): > - */ > - if (config->pwr_clk_cnt > 0) > - actual = clk_round_rate(hdmi->pwr_clks[0], actual); > - > - DBG("requested=%ld, actual=%ld", requested, actual); > - > - if (actual != requested) > - return MODE_CLOCK_RANGE; > - > - return 0; > -} > - > -static const struct drm_connector_funcs hdmi_connector_funcs = { > - .detect = hdmi_connector_detect, > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = hdmi_connector_destroy, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = > drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static const struct drm_connector_helper_funcs > msm_hdmi_connector_helper_funcs = { > - .get_modes = msm_hdmi_connector_get_modes, > - .mode_valid = msm_hdmi_connector_mode_valid, > -}; > - > -/* initialize connector */ > -struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi) > -{ > - struct drm_connector *connector = NULL; > - struct hdmi_connector *hdmi_connector; > - > - hdmi_connector = kzalloc(sizeof(*hdmi_connector), GFP_KERNEL); > - if (!hdmi_connector) > - return ERR_PTR(-ENOMEM); > - > - hdmi_connector->hdmi = hdmi; > - INIT_WORK(&hdmi_connector->hpd_work, msm_hdmi_hotplug_work); > - > - connector = &hdmi_connector->base; > - > - drm_connector_init_with_ddc(hdmi->dev, connector, > - &hdmi_connector_funcs, > - DRM_MODE_CONNECTOR_HDMIA, > - hdmi->i2c); > - drm_connector_helper_add(connector, > &msm_hdmi_connector_helper_funcs); > - > - connector->polled = DRM_CONNECTOR_POLL_CONNECT | > - DRM_CONNECTOR_POLL_DISCONNECT; > - > - connector->interlace_allowed = 0; > - connector->doublescan_allowed = 0; > - > - drm_connector_attach_encoder(connector, hdmi->encoder); > - > - return connector; > -}