LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: abhinavk@codeaurora.org
Cc: Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Kalyan Thota <kalyan_t@codeaurora.org>,
	Kuogee Hsieh <khsieh@codeaurora.org>,
	Rob Herring <robh+dt@kernel.org>,
	Stephen Boyd <swboyd@chromium.org>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel
Date: Tue, 5 Oct 2021 19:09:10 -0700	[thread overview]
Message-ID: <YV0FRvYS8vQYKLI2@ripper> (raw)
In-Reply-To: <28fbd8f5b2d6bae7bedfc7e81e3fddd9@codeaurora.org>

On Tue 05 Oct 17:35 PDT 2021, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> On 2021-10-05 16:13, Bjorn Andersson wrote:
> > eDP panels might need some power sequencing and backlight management,
> > so make it possible to associate a drm_panel with an eDP instance and
> > prepare and enable the panel accordingly.
> > 
> > Now that we know which hardware instance is DP and which is eDP,
> > parser->parse() is passed the connector_type and the parser is limited
> > to only search for a panel in the eDP case.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v3:
> > - Previously posted separately, now added to series
> > - Make the search for a panel conditional on it being an eDP connector
> > - Move the search to of_graph port 1 (was 2 to distinguish from DP
> > link to USB-C connector)
> > 
> >  drivers/gpu/drm/msm/dp/dp_display.c |  9 ++++++---
> >  drivers/gpu/drm/msm/dp/dp_display.h |  1 +
> >  drivers/gpu/drm/msm/dp/dp_drm.c     | 11 +++++++++++
> >  drivers/gpu/drm/msm/dp/dp_parser.c  | 30 ++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ++-
> >  5 files changed, 49 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index eaf08f9e7d87..bdaf227f05dc 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/component.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/delay.h>
> > +#include <drm/drm_panel.h>
> > 
> >  #include "msm_drv.h"
> >  #include "msm_kms.h"
> > @@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev,
> > struct device *master,
> >  	priv = drm->dev_private;
> >  	priv->dp = &(dp->dp_display);
> > 
> > -	rc = dp->parser->parse(dp->parser);
> > +	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
> >  	if (rc) {
> >  		DRM_ERROR("device tree parsing failed\n");
> >  		goto end;
> >  	}
> > 
> > +	dp->dp_display.panel_bridge = dp->parser->panel_bridge;
> > +
> >  	dp->aux->drm_dev = drm;
> >  	rc = dp_aux_register(dp->aux);
> >  	if (rc) {
> > @@ -822,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp
> > *dp_display,
> >  	return 0;
> >  }
> > 
> > -static int dp_display_prepare(struct msm_dp *dp)
> > +static int dp_display_prepare(struct msm_dp *dp_display)
> >  {
> >  	return 0;
> >  }
> > @@ -896,7 +899,7 @@ static int dp_display_disable(struct
> > dp_display_private *dp, u32 data)
> >  	return 0;
> >  }
> > 
> > -static int dp_display_unprepare(struct msm_dp *dp)
> > +static int dp_display_unprepare(struct msm_dp *dp_display)
> >  {
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> > b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 02999408c052..24aefca66029 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > @@ -15,6 +15,7 @@ struct msm_dp {
> >  	struct device *codec_dev;
> >  	struct drm_connector *connector;
> >  	struct drm_encoder *encoder;
> > +	struct drm_bridge *panel_bridge;
> >  	bool is_connected;
> >  	bool audio_enabled;
> >  	bool power_on;
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> > b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index f33e31523f56..76856c4ee1d6 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -5,6 +5,7 @@
> > 
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_atomic.h>
> > +#include <drm/drm_bridge.h>
> >  #include <drm/drm_crtc.h>
> > 
> >  #include "msm_drv.h"
> > @@ -160,5 +161,15 @@ struct drm_connector
> > *dp_drm_connector_init(struct msm_dp *dp_display)
> > 
> >  	drm_connector_attach_encoder(connector, dp_display->encoder);
> > 
> > +	if (dp_display->panel_bridge) {
> > +		ret = drm_bridge_attach(dp_display->encoder,
> > +					dp_display->panel_bridge, NULL,
> > +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +		if (ret < 0) {
> > +			DRM_ERROR("failed to attach panel bridge: %d\n", ret);
> > +			return ERR_PTR(ret);
> > +		}
> > +	}
> > +
> >  	return connector;
> >  }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> > b/drivers/gpu/drm/msm/dp/dp_parser.c
> > index 4d6e047f803d..eb6bbfbea484 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/of_gpio.h>
> >  #include <linux/phy/phy.h>
> > 
> > +#include <drm/drm_of.h>
> >  #include <drm/drm_print.h>
> > 
> >  #include "dp_parser.h"
> > @@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser
> > *parser)
> >  	return 0;
> >  }
> > 
> > -static int dp_parser_parse(struct dp_parser *parser)
> > +static int dp_parser_find_panel(struct dp_parser *parser)
> > +{
> > +	struct device *dev = &parser->pdev->dev;
> > +	struct drm_panel *panel;
> > +	int rc;
> > +
> > +	rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> > +	if (rc) {
> > +		DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> > +		return rc;
> > +	}
> > +
> > +	parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +	if (IS_ERR(parser->panel_bridge)) {
> > +		DRM_ERROR("failed to create panel bridge\n");
> > +		return PTR_ERR(parser->panel_bridge);
> > +	}
> 
> When we add a bridge using devm_drm_panel_bridge_add(), it will register
> with default bridge functions which is fine
> because we need the panel power to be controlled here.
> 
> 
> 140 static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
> 141 	.attach = panel_bridge_attach,
> 142 	.detach = panel_bridge_detach,
> 143 	.pre_enable = panel_bridge_pre_enable,
> 144 	.enable = panel_bridge_enable,
> 145 	.disable = panel_bridge_disable,
> 146 	.post_disable = panel_bridge_post_disable,
> 147 	.get_modes = panel_bridge_get_modes,
> 
> But what about the EDID related things, the DP/eDP driver already reads the
> EDID and gets the modes so we need to skip
> that in this case as otherwise it will end up calling the panel_get_modes in
> the eDP panel which will be redundant.
> 
> Let me know if I am missing something in this proposal.
> 

I'm slightly lost when it comes to these types, so can you please help
me understand why the panel_bridge_bridge_funcs->get_modes() would be
invoked with the proposed patches.

Because adding a log print in panel_bridge_get_modes() shows that it's
not invoked.


So I think we have the opposite problem, rather than having two
redundant get_modes we only support relying on the EDID information
directly in the DP driver, at this point.

So unless I'm missing something critical, this works and provides panel
and backlight controls and the additional work to move the EDID read out
to the panel driver would better be done after this initial support
lands?

Regards,
Bjorn

> > +
> > +	return 0;
> > +}
> > +
> > +static int dp_parser_parse(struct dp_parser *parser, int
> > connector_type)
> >  {
> >  	int rc = 0;
> > 
> > @@ -284,6 +306,12 @@ static int dp_parser_parse(struct dp_parser
> > *parser)
> >  	if (rc)
> >  		return rc;
> > 
> > +	if (connector_type == DRM_MODE_CONNECTOR_eDP) {
> > +		rc = dp_parser_find_panel(parser);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> >  	/* Map the corresponding regulator information according to
> >  	 * version. Currently, since we only have one supported platform,
> >  	 * mapping the regulator directly.
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
> > b/drivers/gpu/drm/msm/dp/dp_parser.h
> > index dac10923abde..3172da089421 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> > @@ -123,8 +123,9 @@ struct dp_parser {
> >  	struct dp_display_data disp_data;
> >  	const struct dp_regulator_cfg *regulator_cfg;
> >  	u32 max_dp_lanes;
> > +	struct drm_bridge *panel_bridge;
> > 
> > -	int (*parse)(struct dp_parser *parser);
> > +	int (*parse)(struct dp_parser *parser, int connector_type);
> >  };
> > 
> >  /**

  reply	other threads:[~2021-10-06  2:07 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-05 23:13 [PATCH v4 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 1/7] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 2/7] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller Bjorn Andersson
2021-10-06  0:15   ` Stephen Boyd
2021-10-06  0:29   ` Stephen Boyd
2021-10-06  3:35     ` Bjorn Andersson
2021-10-06  0:31   ` Stephen Boyd
2021-10-05 23:13 ` [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel Bjorn Andersson
2021-10-06  0:35   ` [Freedreno] " abhinavk
2021-10-06  2:09     ` Bjorn Andersson [this message]
2021-10-06  3:09       ` abhinavk
2021-10-08 15:01     ` Doug Anderson
2021-10-05 23:13 ` [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
2021-10-06  0:43   ` Stephen Boyd
2021-10-06  1:43     ` Bjorn Andersson
2021-10-06  2:06       ` Stephen Boyd
2021-10-06  2:37         ` Bjorn Andersson
2021-10-06  4:26           ` Stephen Boyd
2021-10-06  6:10             ` Dmitry Baryshkov
2021-10-06  7:06               ` Stephen Boyd
2021-10-06 11:35                 ` Dmitry Baryshkov
2021-10-06 17:07             ` Bjorn Andersson
2021-10-06 17:19               ` Stephen Boyd
2021-10-06 18:05                 ` Bjorn Andersson
2021-10-06 18:59                   ` Stephen Boyd
2021-10-06 20:39                     ` Bjorn Andersson
2021-10-07 22:29                       ` abhinavk
2021-10-06 17:19               ` Dmitry Baryshkov
2021-10-06 18:37                 ` Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 6/7] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 7/7] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
2021-10-06  0:36   ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YV0FRvYS8vQYKLI2@ripper \
    --to=bjorn.andersson@linaro.org \
    --cc=abhinavk@codeaurora.org \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=kalyan_t@codeaurora.org \
    --cc=khsieh@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robdclark@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=sean@poorly.run \
    --cc=swboyd@chromium.org \
    --subject='Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).