On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels. The panel bridge attaches to the encoder before the "dp" bridge has a chace to do so. Change panel_bridge attachment to come after dp_bridge attachment.

Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") Cc: Kuogee Hsieh quic_khsieh@quicinc.com Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

Tested-by: Kuogee Hsieh quic_khsieh@quicinc.com

---

drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d19eba..26ef41a4c1b6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -169,16 +169,6 @@ 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; }

@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }

• if (dp_display->panel_bridge) {

• rc = drm_bridge_attach(dp_display->encoder,
  

• 			dp_display->panel_bridge, bridge,
  

• 			DRM_BRIDGE_ATTACH_NO_CONNECTOR);
  

• if (rc < 0) {
  

• 	DRM_ERROR("failed to attach panel bridge: %d\n", rc);
  

• 	drm_bridge_remove(bridge);
  

• 	return ERR_PTR(rc);
  

• }
  

• }
• return bridge; }

On 19/02/2022 02:56, Stephen Boyd wrote:

Quoting Dmitry Baryshkov (2022-02-11 14:40:02)

...

In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels. The panel bridge attaches to the encoder before the "dp" bridge has a chace to do so. Change

s/chace/chance/

...

panel_bridge attachment to come after dp_bridge attachment.

s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge is the "DP driver's drm_bridge instance created in msm_dp_bridge_init()"?

My understanding is that we want to pass the bridge created in msm_dp_bridge_init() as the 'previous' bridge so that it attaches the panel bridge to the output of the DP bridge that's attached to the encoder.

Thanks! I'll update the commit log when pushing the patches.

...

Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") Cc: Kuogee Hsieh quic_khsieh@quicinc.com Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

---

Reviewed-by: Stephen Boyd swboyd@chromium.org

...

drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d19eba..26ef41a4c1b6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -169,16 +169,6 @@ 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;
  

  }

@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }

•   if (dp_display->panel_bridge) {
  

•           rc = drm_bridge_attach(dp_display->encoder,
  

•                                   dp_display->panel_bridge, bridge,
  

•                                   DRM_BRIDGE_ATTACH_NO_CONNECTOR);
  

•           if (rc < 0) {
  

•                   DRM_ERROR("failed to attach panel bridge: %d\n", rc);
  

•                   drm_bridge_remove(bridge);
  

•                   return ERR_PTR(rc);
  

•           }
  

•   }
  

•    return bridge;
  

Not a problem with this patch, but what is this pointer used for? I see it's assigned to priv->bridges and num_bridges is incremented but nobody seems to look at that.

That's on my todo list. to remove connectors array and to destroy created bridges during drm device destruction.

On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

On 2/18/2022 6:26 PM, Dmitry Baryshkov wrote:

...

On 19/02/2022 02:56, Stephen Boyd wrote:

...

Quoting Dmitry Baryshkov (2022-02-11 14:40:02)

...

In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels. The panel bridge attaches to the encoder before the "dp" bridge has a chace to do so. Change

s/chace/chance/

...

panel_bridge attachment to come after dp_bridge attachment.

s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge is the "DP driver's drm_bridge instance created in msm_dp_bridge_init()"?

My understanding is that we want to pass the bridge created in msm_dp_bridge_init() as the 'previous' bridge so that it attaches the panel bridge to the output of the DP bridge that's attached to the encoder.

Thanks! I'll update the commit log when pushing the patches.

Please correct if i am missing something here.

You are right that the eDP panel's panel bridge attaches to the encoder in dp_drm_connector_init() which happens before msm_dp_bridge_init() and hence it will attach directly to the encoder.

But we are talking about different encoders here. DP's dp_display has a different encoder compared to the EDP's dp_display.

The encoders are different. However both encoders use the same codepath, which includes dp_bridge. It controls dp_display by calling msm_dp_display_foo() functions.

So DP's bridge will still be attached to its encoder's root.

There is one dp_bridge per each encoder. Consider sc8180x which has 3 DP controllers (and thus 3 dp_bridges).

So what are we achieving with this change?

...

...

...

Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") Cc: Kuogee Hsieh quic_khsieh@quicinc.com Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

---

Reviewed-by: Stephen Boyd swboyd@chromium.org

...

drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d19eba..26ef41a4c1b6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -169,16 +169,6 @@ 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;
  

  }

@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }

•   if (dp_display->panel_bridge) {
  

•           rc = drm_bridge_attach(dp_display->encoder,
  

•                                   dp_display->panel_bridge,
  

bridge,

•                                   DRM_BRIDGE_ATTACH_NO_CONNECTOR);
  

•           if (rc < 0) {
  

•                   DRM_ERROR("failed to attach panel bridge:
  

%d\n", rc);

•                   drm_bridge_remove(bridge);
  

•                   return ERR_PTR(rc);
  

•           }
  

•   }
  

•    return bridge;
  

Not a problem with this patch, but what is this pointer used for? I see it's assigned to priv->bridges and num_bridges is incremented but nobody seems to look at that.

That's on my todo list. to remove connectors array and to destroy created bridges during drm device destruction.

On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote:

...

On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

...

On 2/18/2022 6:26 PM, Dmitry Baryshkov wrote:

...

On 19/02/2022 02:56, Stephen Boyd wrote:

...

Quoting Dmitry Baryshkov (2022-02-11 14:40:02)

...

In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels. The panel bridge attaches to the encoder before the "dp" bridge has a chace to do so. Change

s/chace/chance/

...

panel_bridge attachment to come after dp_bridge attachment.

s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge is the "DP driver's drm_bridge instance created in msm_dp_bridge_init()"?

My understanding is that we want to pass the bridge created in msm_dp_bridge_init() as the 'previous' bridge so that it attaches the panel bridge to the output of the DP bridge that's attached to the encoder.

Thanks! I'll update the commit log when pushing the patches.

Please correct if i am missing something here.

You are right that the eDP panel's panel bridge attaches to the encoder in dp_drm_connector_init() which happens before msm_dp_bridge_init() and hence it will attach directly to the encoder.

But we are talking about different encoders here. DP's dp_display has a different encoder compared to the EDP's dp_display.

The encoders are different. However both encoders use the same codepath, which includes dp_bridge. It controls dp_display by calling msm_dp_display_foo() functions.

...

...

So DP's bridge will still be attached to its encoder's root.

There is one dp_bridge per each encoder. Consider sc8180x which has 3 DP controllers (and thus 3 dp_bridges).

Sorry, but I still didnt follow this.

So for eDP, dp_drm_connector_init() will attach the panel_bridge and then msm_dp_bridge_init() will add a drm_bridge.

And yes in that case, the drm_bridge will be after the panel_bridge

But since panel_bridge is at the root for eDP it should be okay.

No. It is not. For both DP and eDP the chain should be: dpu_encoder -> dp_bridge -> external (panel) bridge, optional for DP -> [other bridges] -> connector

Otherwise drm_bridge_chain_foo() functions would be called in the incorrect order.

Thus the dp_bridge should be attached directly to the encoder (drm_encoder) and panel_bridge should use dp_bridge as the 'previous' bridge.

For example, for the DP port one can use a display-connector (which actually implements drm_bridge) as an external bridge to provide hpd or dp power GPIOs.

Note, that the current dp_connector breaks layering. It makes calls directly into dp_display, not allowing external bridge (and other bridges) to override get_modes/mode_valid and other callbacks. Thus one of the next patches in series (the one that Kuogee had issues with) tries to replace the chain with the following one: dpu_encoder -> dp_bridge -> external (panel) bridge -> [other bridges] -> drm_bridge_connector.

Your commit text was mentioning about DP.

For DP, for each controller in the catalog, we will call modeset_init() which should skip this part for DP

if (dp_display->panel_bridge) { ret = drm_bridge_attach(dp_display->encoder, dp_display->panel_bridge, NULL,

as you see, NULL is incorrect. It should be a pointer to dp_bridge instead

                 DRM_BRIDGE_ATTACH_NO_CONNECTOR);
     if (ret < 0) {
         DRM_ERROR("failed to attach panel bridge: %d\n", ret);
         return ERR_PTR(ret);
     }
 }

Followed by calling msm_dp_bridge_init() which will actually attach the bridge:

drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);

And this bridge should be attached before the external bridge.

Now, even for 3 DP controllers, this shall be true as there will be 3 separate encoders and 3 dp_displays and hence 3 drm_bridges.

What am i missing here?

...

...

So what are we achieving with this change?

...

...

...

Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") Cc: Kuogee Hsieh quic_khsieh@quicinc.com Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

---

Reviewed-by: Stephen Boyd swboyd@chromium.org

...

drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d19eba..26ef41a4c1b6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -169,16 +169,6 @@ 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;
  

  }

@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }

•   if (dp_display->panel_bridge) {
  

•           rc = drm_bridge_attach(dp_display->encoder,
  

•                                   dp_display->panel_bridge,
  

bridge,

•                                   DRM_BRIDGE_ATTACH_NO_CONNECTOR);
  

•           if (rc < 0) {
  

•                   DRM_ERROR("failed to attach panel bridge:
  

%d\n", rc);

•                   drm_bridge_remove(bridge);
  

•                   return ERR_PTR(rc);
  

•           }
  

•   }
  

•     return bridge;
  

Not a problem with this patch, but what is this pointer used for? I see it's assigned to priv->bridges and num_bridges is incremented but nobody seems to look at that.

That's on my todo list. to remove connectors array and to destroy created bridges during drm device destruction.

On Fri, 25 Feb 2022 at 07:45, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

On 2/24/2022 8:22 PM, Dmitry Baryshkov wrote:

...

On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

...

On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote:

...

On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

...

On 2/18/2022 6:26 PM, Dmitry Baryshkov wrote:

...

On 19/02/2022 02:56, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-02-11 14:40:02) >> In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display >> enable and disable") the DP driver received a drm_bridge instance, which >> is always attached to the encoder as a root bridge. However it conflicts >> with the panel_bridge support for eDP panels. The panel bridge attaches >> to the encoder before the "dp" bridge has a chace to do so. Change > > s/chace/chance/ > >> panel_bridge attachment to come after dp_bridge attachment. > > s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge > is the "DP driver's drm_bridge instance created in > msm_dp_bridge_init()"? > > My understanding is that we want to pass the bridge created in > msm_dp_bridge_init() as the 'previous' bridge so that it attaches the > panel bridge to the output of the DP bridge that's attached to the > encoder.

Thanks! I'll update the commit log when pushing the patches.

Please correct if i am missing something here.

You are right that the eDP panel's panel bridge attaches to the encoder in dp_drm_connector_init() which happens before msm_dp_bridge_init() and hence it will attach directly to the encoder.

But we are talking about different encoders here. DP's dp_display has a different encoder compared to the EDP's dp_display.

The encoders are different. However both encoders use the same codepath, which includes dp_bridge. It controls dp_display by calling msm_dp_display_foo() functions.

...

...

...

So DP's bridge will still be attached to its encoder's root.

There is one dp_bridge per each encoder. Consider sc8180x which has 3 DP controllers (and thus 3 dp_bridges).

Sorry, but I still didnt follow this.

So for eDP, dp_drm_connector_init() will attach the panel_bridge and then msm_dp_bridge_init() will add a drm_bridge.

And yes in that case, the drm_bridge will be after the panel_bridge

But since panel_bridge is at the root for eDP it should be okay.

No. It is not. For both DP and eDP the chain should be: dpu_encoder -> dp_bridge -> external (panel) bridge, optional for DP -> [other bridges] -> connector

Otherwise drm_bridge_chain_foo() functions would be called in the incorrect order.

Agreed. For drm_bridge_chain_foo() ordering to be correct, for eDP chain the order should be what you mentioned and panel_bridge should be at the end ( should be the last one ).

For the above reason,

Reviewed-by: Abhinav Kumar quic_abhinavk@quicinc.com

I still didnt understand what gets affected by this for the msm_dp_display_foo() functions you mentioned earlier and wanted to get some clarity on that.

They should be called at the correct time.

...

Thus the dp_bridge should be attached directly to the encoder (drm_encoder) and panel_bridge should use dp_bridge as the 'previous' bridge.

Agreed.

...

For example, for the DP port one can use a display-connector (which actually implements drm_bridge) as an external bridge to provide hpd or dp power GPIOs.

Note, that the current dp_connector breaks layering. It makes calls directly into dp_display, not allowing external bridge (and other bridges) to override get_modes/mode_valid and other callbacks. Thus one of the next patches in series (the one that Kuogee had issues with) tries to replace the chain with the following one: dpu_encoder -> dp_bridge -> external (panel) bridge -> [other bridges] -> drm_bridge_connector.

...

So originally the plan was always that the DP connector layer intercepts the call because panel-eDP file did not support reading of the EDID ( we have not provided the aux bus ). So it was intended that we did not want to goto the eDP panel to get the modes. Not an error but something which we wanted to cleanup later when we moved to panel-eDP completely.

panel_edp_get_modes() correctly handles this case and returns modes specified in the panel description. So the code should work even with panel-eDP and w/o the AUX bus.

Till then we wanted the dp_connector to read the EDID and get the modes.

So this was actually intended to happen till the point where we moved to panel-eDP to get the modes.

Hence what you have mentioned in your cover letter is right that the chain was broken but there was no loss of functionality due to that today.

Now that these changes are made, we can still goto panel-eDP file for the modes because of the below change from Sankeerth where the mode is hard-coded:

https://patchwork.freedesktop.org/patch/473431/

With this change cherry-picked it should work for kuogee. We will re-test that with this change.

I suppose he had both of the changes in the tree. Otherwise the panel_edp_get_modes() wouldn't be called.

...

...

Your commit text was mentioning about DP.

For DP, for each controller in the catalog, we will call modeset_init() which should skip this part for DP

if (dp_display->panel_bridge) {
      ret = drm_bridge_attach(dp_display->encoder,
                  dp_display->panel_bridge, NULL,

as you see, NULL is incorrect. It should be a pointer to dp_bridge instead

...

                  DRM_BRIDGE_ATTACH_NO_CONNECTOR);
      if (ret < 0) {
          DRM_ERROR("failed to attach panel bridge: %d\n", ret);
          return ERR_PTR(ret);
      }
  }

Followed by calling msm_dp_bridge_init() which will actually attach the bridge:

drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);

And this bridge should be attached before the external bridge.

...

Now, even for 3 DP controllers, this shall be true as there will be 3 separate encoders and 3 dp_displays and hence 3 drm_bridges.

What am i missing here?

...

...

So what are we achieving with this change?

...

> >> >> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display >> enable and disable") >> Cc: Kuogee Hsieh quic_khsieh@quicinc.com >> Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org >> --- > > Reviewed-by: Stephen Boyd swboyd@chromium.org > >> drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c >> b/drivers/gpu/drm/msm/dp/dp_drm.c >> index d4d360d19eba..26ef41a4c1b6 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_drm.c >> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c >> @@ -169,16 +169,6 @@ 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; >> } >> >> @@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct >> msm_dp *dp_display, struct drm_devi >> return ERR_PTR(rc); >> } >> >> + if (dp_display->panel_bridge) { >> + rc = drm_bridge_attach(dp_display->encoder, >> + dp_display->panel_bridge, >> bridge, >> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); >> + if (rc < 0) { >> + DRM_ERROR("failed to attach panel bridge: >> %d\n", rc); >> + drm_bridge_remove(bridge); >> + return ERR_PTR(rc); >> + } >> + } >> + >> return bridge; > > Not a problem with this patch, but what is this pointer used for? I see > it's assigned to priv->bridges and num_bridges is incremented but nobody > seems to look at that.

That's on my todo list. to remove connectors array and to destroy created bridges during drm device destruction.

On Fri, 25 Feb 2022 at 20:11, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

On 2/25/2022 1:04 AM, Dmitry Baryshkov wrote:

...

On Fri, 25 Feb 2022 at 07:45, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

...

On 2/24/2022 8:22 PM, Dmitry Baryshkov wrote:

...

On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

...

On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote:

...

On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar quic_abhinavk@quicinc.com wrote: > > > > On 2/18/2022 6:26 PM, Dmitry Baryshkov wrote: >> On 19/02/2022 02:56, Stephen Boyd wrote: >>> Quoting Dmitry Baryshkov (2022-02-11 14:40:02) >>>> In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display >>>> enable and disable") the DP driver received a drm_bridge instance, which >>>> is always attached to the encoder as a root bridge. However it conflicts >>>> with the panel_bridge support for eDP panels. The panel bridge attaches >>>> to the encoder before the "dp" bridge has a chace to do so. Change >>> >>> s/chace/chance/ >>> >>>> panel_bridge attachment to come after dp_bridge attachment. >>> >>> s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge >>> is the "DP driver's drm_bridge instance created in >>> msm_dp_bridge_init()"? >>> >>> My understanding is that we want to pass the bridge created in >>> msm_dp_bridge_init() as the 'previous' bridge so that it attaches the >>> panel bridge to the output of the DP bridge that's attached to the >>> encoder. >> >> Thanks! I'll update the commit log when pushing the patches. > > Please correct if i am missing something here. > > You are right that the eDP panel's panel bridge attaches to the encoder > in dp_drm_connector_init() which happens before msm_dp_bridge_init() and > hence it will attach directly to the encoder. > > But we are talking about different encoders here. DP's dp_display has a > different encoder compared to the EDP's dp_display.

The encoders are different. However both encoders use the same codepath, which includes dp_bridge. It controls dp_display by calling msm_dp_display_foo() functions.

...

...

> So DP's bridge will still be attached to its encoder's root.

There is one dp_bridge per each encoder. Consider sc8180x which has 3 DP controllers (and thus 3 dp_bridges).

Sorry, but I still didnt follow this.

So for eDP, dp_drm_connector_init() will attach the panel_bridge and then msm_dp_bridge_init() will add a drm_bridge.

And yes in that case, the drm_bridge will be after the panel_bridge

But since panel_bridge is at the root for eDP it should be okay.

No. It is not. For both DP and eDP the chain should be: dpu_encoder -> dp_bridge -> external (panel) bridge, optional for DP -> [other bridges] -> connector

Otherwise drm_bridge_chain_foo() functions would be called in the incorrect order.

Agreed. For drm_bridge_chain_foo() ordering to be correct, for eDP chain the order should be what you mentioned and panel_bridge should be at the end ( should be the last one ).

For the above reason,

Reviewed-by: Abhinav Kumar quic_abhinavk@quicinc.com

I still didnt understand what gets affected by this for the msm_dp_display_foo() functions you mentioned earlier and wanted to get some clarity on that.

They should be called at the correct time.

...

...

Thus the dp_bridge should be attached directly to the encoder (drm_encoder) and panel_bridge should use dp_bridge as the 'previous' bridge.

Agreed.

...

For example, for the DP port one can use a display-connector (which actually implements drm_bridge) as an external bridge to provide hpd or dp power GPIOs.

Note, that the current dp_connector breaks layering. It makes calls directly into dp_display, not allowing external bridge (and other bridges) to override get_modes/mode_valid and other callbacks. Thus one of the next patches in series (the one that Kuogee had issues with) tries to replace the chain with the following one: dpu_encoder -> dp_bridge -> external (panel) bridge -> [other bridges] -> drm_bridge_connector.

...

So originally the plan was always that the DP connector layer intercepts the call because panel-eDP file did not support reading of the EDID ( we have not provided the aux bus ). So it was intended that we did not want to goto the eDP panel to get the modes. Not an error but something which we wanted to cleanup later when we moved to panel-eDP completely.

panel_edp_get_modes() correctly handles this case and returns modes specified in the panel description. So the code should work even with panel-eDP and w/o the AUX bus.

If hard-coded modes are not present, it will fail in below case:

 /*
  * Add hard-coded panel modes. Don't call this if there are no timings
  * and no modes (the generic edp-panel case) because it will clobber
  * the display_info that was already set by drm_add_edid_modes().
  */
 if (p->desc->num_timings || p->desc->num_modes)
     num += panel_edp_get_non_edid_modes(p, connector);
 else if (!num)
     dev_warn(p->base.dev, "No display modes\n");

Thats exactly the error he was seeing.

Ah, so he had neither timings nor modes. The rest of the panels does.

...

...

Till then we wanted the dp_connector to read the EDID and get the modes.

So this was actually intended to happen till the point where we moved to panel-eDP to get the modes.

Hence what you have mentioned in your cover letter is right that the chain was broken but there was no loss of functionality due to that today.

Now that these changes are made, we can still goto panel-eDP file for the modes because of the below change from Sankeerth where the mode is hard-coded:

https://patchwork.freedesktop.org/patch/473431/

With this change cherry-picked it should work for kuogee. We will re-test that with this change.

I suppose he had both of the changes in the tree. Otherwise the panel_edp_get_modes() wouldn't be called.

No he did not.

That brings up another point.

Even Bjorn was relying on the DP connector's get_modes() for his eDP panel if I am not mistaken.

Unless he makes an equivalent change for his panel OR supports reading EDID in panel-edp, then he will hit the same error.

Given that your changes were only compile tested, lets wait for both Kuogee and Bjorn to validate their resp setups.

Sure, anyway I think it's too late to bring in this patch during this cycle.

...

...

...

...

Your commit text was mentioning about DP.

For DP, for each controller in the catalog, we will call modeset_init() which should skip this part for DP

 if (dp_display->panel_bridge) {
       ret = drm_bridge_attach(dp_display->encoder,
                   dp_display->panel_bridge, NULL,

as you see, NULL is incorrect. It should be a pointer to dp_bridge instead

...

                   DRM_BRIDGE_ATTACH_NO_CONNECTOR);
       if (ret < 0) {
           DRM_ERROR("failed to attach panel bridge: %d\n", ret);
           return ERR_PTR(ret);
       }
   }

Followed by calling msm_dp_bridge_init() which will actually attach the bridge:

drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);

And this bridge should be attached before the external bridge.

...

Now, even for 3 DP controllers, this shall be true as there will be 3 separate encoders and 3 dp_displays and hence 3 drm_bridges.

What am i missing here?

...

> > So what are we achieving with this change? > >> >>> >>>> >>>> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display >>>> enable and disable") >>>> Cc: Kuogee Hsieh quic_khsieh@quicinc.com >>>> Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org >>>> --- >>> >>> Reviewed-by: Stephen Boyd swboyd@chromium.org >>> >>>> drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- >>>> 1 file changed, 11 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c >>>> b/drivers/gpu/drm/msm/dp/dp_drm.c >>>> index d4d360d19eba..26ef41a4c1b6 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c >>>> @@ -169,16 +169,6 @@ 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; >>>> } >>>> >>>> @@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct >>>> msm_dp *dp_display, struct drm_devi >>>> return ERR_PTR(rc); >>>> } >>>> >>>> + if (dp_display->panel_bridge) { >>>> + rc = drm_bridge_attach(dp_display->encoder, >>>> + dp_display->panel_bridge, >>>> bridge, >>>> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); >>>> + if (rc < 0) { >>>> + DRM_ERROR("failed to attach panel bridge: >>>> %d\n", rc); >>>> + drm_bridge_remove(bridge); >>>> + return ERR_PTR(rc); >>>> + } >>>> + } >>>> + >>>> return bridge; >>> >>> Not a problem with this patch, but what is this pointer used for? I see >>> it's assigned to priv->bridges and num_bridges is incremented but nobody >>> seems to look at that. >> >> >> That's on my todo list. to remove connectors array and to destroy >> created bridges during drm device destruction. >>

On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

Currently DP driver will allocate panel bridge for eDP panels. This supports only the following topology:

• eDP encoder ⇒ eDP panel (wrapped using panel-bridge)

Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly.

This allows one to use e.g. one of the following display topologies:

• eDP encoder ⇒ ptn3460 ⇒ fixed LVDS panel
• eDP encoder ⇒ ptn3460 ⇒ LVDS connector with EDID lines for panel autodetect
• eDP encoder ⇒ ptn3460 ⇒ THC63LVD1024 ⇒ DPI panel.
• eDP encoder ⇒ LT8912 ⇒ DSI panel

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

Tested-by: Kuogee Hsieh quic_khsieh@quicinc.com

---

drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 31 +++++++++++++++-------------- drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 44d42c76c2a3..45f9a912ecc5 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -266,7 +266,7 @@ static int dp_display_bind(struct device *dev, struct device *master, goto end; }

• dp->dp_display.panel_bridge = dp->parser->panel_bridge;

• dp->dp_display.next_bridge = dp->parser->next_bridge;

  dp->aux->drm_dev = drm; rc = dp_aux_register(dp->aux);

diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index e3adcd578a90..7af2b186d2d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -16,7 +16,7 @@ struct msm_dp { struct drm_bridge *bridge; struct drm_connector *connector; struct drm_encoder *encoder;

• struct drm_bridge *panel_bridge;

• struct drm_bridge *next_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 26ef41a4c1b6..80f59cf99089 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -236,9 +236,9 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }

• if (dp_display->panel_bridge) {

• if (dp_display->next_bridge) { rc = drm_bridge_attach(dp_display->encoder,

• 			dp_display->panel_bridge, bridge,
  

• 			dp_display->next_bridge, bridge,
  		DRM_BRIDGE_ATTACH_NO_CONNECTOR);
  

  if (rc < 0) { DRM_ERROR("failed to attach panel bridge: %d\n", rc);

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..901d7967370f 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -265,23 +265,16 @@ static int dp_parser_clock(struct dp_parser *parser) return 0; }

-static int dp_parser_find_panel(struct dp_parser *parser) +static int dp_parser_find_next_bridge(struct dp_parser *parser) { struct device *dev = &parser->pdev->dev;

• struct drm_panel *panel;
• int rc;

• struct drm_bridge *bridge;

• 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;
  

• }

• bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
• if (IS_ERR(bridge))

• return PTR_ERR(bridge);
  
  

• 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);
  

• }

• parser->next_bridge = bridge;

  return 0; }

@@ -307,10 +300,18 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc;

• /*

• * Currently we support external bridges only for eDP connectors.
  

• *
  

• * No external bridges are expected for the DisplayPort connector,
  

• * it is physically present in a form of a DP or USB-C connector.
  

• */
  

  if (connector_type == DRM_MODE_CONNECTOR_eDP) {

• rc = dp_parser_find_panel(parser);
  

• if (rc)
  

• rc = dp_parser_find_next_bridge(parser);
  

• if (rc) {
  

• 	DRM_ERROR("DP: failed to find next bridge\n");
  return rc;
  

• }
  

  }

  /* Map the corresponding regulator information according to

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h index 3172da089421..4cec851e38d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.h +++ b/drivers/gpu/drm/msm/dp/dp_parser.h @@ -123,7 +123,7 @@ 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;

• struct drm_bridge *next_bridge;

  int (*parse)(struct dp_parser *parser, int connector_type); };

On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

Currently DP driver will allocate panel bridge for eDP panels. This supports only the following topology:

• eDP encoder ⇒ eDP panel (wrapped using panel-bridge)

Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly.

This allows one to use e.g. one of the following display topologies:

• eDP encoder ⇒ ptn3460 ⇒ fixed LVDS panel
• eDP encoder ⇒ ptn3460 ⇒ LVDS connector with EDID lines for panel autodetect
• eDP encoder ⇒ ptn3460 ⇒ THC63LVD1024 ⇒ DPI panel.
• eDP encoder ⇒ LT8912 ⇒ DSI panel

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

Reviewed-by: Abhinav Kumar quic_abhinavk@quicinc.com

---

drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 31 +++++++++++++++-------------- drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 44d42c76c2a3..45f9a912ecc5 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -266,7 +266,7 @@ static int dp_display_bind(struct device *dev, struct device *master, goto end; }

• dp->dp_display.panel_bridge = dp->parser->panel_bridge;

• dp->dp_display.next_bridge = dp->parser->next_bridge;

  dp->aux->drm_dev = drm; rc = dp_aux_register(dp->aux);

diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index e3adcd578a90..7af2b186d2d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -16,7 +16,7 @@ struct msm_dp { struct drm_bridge *bridge; struct drm_connector *connector; struct drm_encoder *encoder;

• struct drm_bridge *panel_bridge;

• struct drm_bridge *next_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 26ef41a4c1b6..80f59cf99089 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -236,9 +236,9 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }

• if (dp_display->panel_bridge) {

• if (dp_display->next_bridge) { rc = drm_bridge_attach(dp_display->encoder,

• 			dp_display->panel_bridge, bridge,
  

• 			dp_display->next_bridge, bridge,
  		DRM_BRIDGE_ATTACH_NO_CONNECTOR);
  

  if (rc < 0) { DRM_ERROR("failed to attach panel bridge: %d\n", rc);

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..901d7967370f 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -265,23 +265,16 @@ static int dp_parser_clock(struct dp_parser *parser) return 0; }

-static int dp_parser_find_panel(struct dp_parser *parser) +static int dp_parser_find_next_bridge(struct dp_parser *parser) { struct device *dev = &parser->pdev->dev;

• struct drm_panel *panel;
• int rc;

• struct drm_bridge *bridge;

• 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;
  

• }

• bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
• if (IS_ERR(bridge))

• return PTR_ERR(bridge);
  
  

• 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);
  

• }

• parser->next_bridge = bridge;

  return 0; }

@@ -307,10 +300,18 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc;

• /*

• * Currently we support external bridges only for eDP connectors.
  

• *
  

• * No external bridges are expected for the DisplayPort connector,
  

• * it is physically present in a form of a DP or USB-C connector.
  

• */
  

  if (connector_type == DRM_MODE_CONNECTOR_eDP) {

• rc = dp_parser_find_panel(parser);
  

• if (rc)
  

• rc = dp_parser_find_next_bridge(parser);
  

• if (rc) {
  

• 	DRM_ERROR("DP: failed to find next bridge\n");
  return rc;
  

• }
  

  }

  /* Map the corresponding regulator information according to

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h index 3172da089421..4cec851e38d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.h +++ b/drivers/gpu/drm/msm/dp/dp_parser.h @@ -123,7 +123,7 @@ 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;

• struct drm_bridge *next_bridge;

  int (*parse)(struct dp_parser *parser, int connector_type); };

On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

It is possible to supply display-connector (bridge) to the DP interface, add support for parsing it too.

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

Tested-by: Kuogee Hsieh quic_khsieh@quicinc.com

---

drivers/gpu/drm/msm/dp/dp_parser.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index 901d7967370f..1056b8d5755b 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -301,17 +301,22 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) return rc;

/*

• * Currently we support external bridges only for eDP connectors.
  

• * External bridges are mandatory for eDP interfaces: one has to
  

• * provide at least an eDP panel (which gets wrapped into panel-bridge).
  

• * No external bridges are expected for the DisplayPort connector,
  

• * it is physically present in a form of a DP or USB-C connector.
  

• * For DisplayPort interfaces external bridges are optional, so
  

• * silently ignore an error if one is not present (-ENODEV).
  

  */

• if (connector_type == DRM_MODE_CONNECTOR_eDP) {

• rc = dp_parser_find_next_bridge(parser);
  

• if (rc) {
  

• 	DRM_ERROR("DP: failed to find next bridge\n");
  

• rc = dp_parser_find_next_bridge(parser);

• if (rc == -ENODEV) {

• if (connector_type == DRM_MODE_CONNECTOR_eDP) {
  

• 	DRM_ERROR("eDP: next bridge is not present\n");
  return rc;
  

  }

• } else if (rc) {

• if (rc != -EPROBE_DEFER)
  

• 	DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
  

• return rc;
  

  }

  /* Map the corresponding regulator information according to

On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

It is possible to supply display-connector (bridge) to the DP interface, add support for parsing it too.

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

drivers/gpu/drm/msm/dp/dp_parser.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index 901d7967370f..1056b8d5755b 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -301,17 +301,22 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) return rc;

/*

• * Currently we support external bridges only for eDP connectors.
  

• * External bridges are mandatory for eDP interfaces: one has to
  

• * provide at least an eDP panel (which gets wrapped into panel-bridge).
  

• * No external bridges are expected for the DisplayPort connector,
  

• * it is physically present in a form of a DP or USB-C connector.
  

• * For DisplayPort interfaces external bridges are optional, so
  

• * silently ignore an error if one is not present (-ENODEV).
  

  */

• if (connector_type == DRM_MODE_CONNECTOR_eDP) {

• rc = dp_parser_find_next_bridge(parser);
  

• if (rc) {
  

• 	DRM_ERROR("DP: failed to find next bridge\n");
  

• rc = dp_parser_find_next_bridge(parser);
• if (rc == -ENODEV) {

• if (connector_type == DRM_MODE_CONNECTOR_eDP) {
  

• 	DRM_ERROR("eDP: next bridge is not present\n");
  return rc;
  

  }
• } else if (rc) {

• if (rc != -EPROBE_DEFER)
  

• 	DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
  

• return rc;
  

  }

How is this silently ignoring?

static int dp_display_bind(struct device *dev, struct device *master, void *data) { int rc = 0; struct dp_display_private *dp = dev_get_dp_display_private(dev); struct msm_drm_private *priv = dev_get_drvdata(master); struct drm_device *drm = priv->dev;

dp->dp_display.drm_dev = drm; priv->dp[dp->id] = &dp->dp_display;

rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type); if (rc) { DRM_ERROR("device tree parsing failed\n"); goto end; }

dp_display_bind will still fail if a bridge is not found.

If supplying a bridge is optional even this should succeed right?

/* Map the corresponding regulator information according to

On 2/24/2022 12:49 PM, Dmitry Baryshkov wrote:

On Thu, 24 Feb 2022 at 23:13, Abhinav Kumar quic_abhinavk@quicinc.com wrote:

...

On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

...

It is possible to supply display-connector (bridge) to the DP interface, add support for parsing it too.

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

drivers/gpu/drm/msm/dp/dp_parser.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index 901d7967370f..1056b8d5755b 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -301,17 +301,22 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) return rc;

   /*

•  * Currently we support external bridges only for eDP connectors.
  

•  * External bridges are mandatory for eDP interfaces: one has to
  

•  * provide at least an eDP panel (which gets wrapped into panel-bridge).
    *
  

•  * No external bridges are expected for the DisplayPort connector,
  

•  * it is physically present in a form of a DP or USB-C connector.
  

•  * For DisplayPort interfaces external bridges are optional, so
  

•  * silently ignore an error if one is not present (-ENODEV).
    */
  

• if (connector_type == DRM_MODE_CONNECTOR_eDP) {
  

•         rc = dp_parser_find_next_bridge(parser);
  

•         if (rc) {
  

•                 DRM_ERROR("DP: failed to find next bridge\n");
  

• rc = dp_parser_find_next_bridge(parser);
  

• if (rc == -ENODEV) {
  

•         if (connector_type == DRM_MODE_CONNECTOR_eDP) {
  

•                 DRM_ERROR("eDP: next bridge is not present\n");
                   return rc;
           }
  

• } else if (rc) {
  

•         if (rc != -EPROBE_DEFER)
  

•                 DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
  

•         return rc;
   }
  

How is this silently ignoring?

static int dp_display_bind(struct device *dev, struct device *master, void *data) { int rc = 0; struct dp_display_private *dp = dev_get_dp_display_private(dev); struct msm_drm_private *priv = dev_get_drvdata(master); struct drm_device *drm = priv->dev;

  dp->dp_display.drm_dev = drm;
  priv->dp[dp->id] = &dp->dp_display;

  rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
  if (rc) {
      DRM_ERROR("device tree parsing failed\n");
      goto end;
  }

dp_display_bind will still fail if a bridge is not found.

If supplying a bridge is optional even this should succeed right?

It will succeed as dp_parser_parse() will not return -ENODEV if the connector is not eDP. To rephrase the comment: For the dp_parser_find_next_bridge() result:

• for eDP the driver passes all errors to the calling function.
• for DP the driver ignores -ENODEV (no external bridge is supplied),

but passes all other errors (which can mean e.g. that the bridge is not properly declared or that it did hasn't been probed yet).

Ah okay, I just noticed that dp_parser_parse() returns 0 by default and not rc.

So in this case it will still return 0.

Hence this change LGTM,

Reviewed-by: Abhinav Kumar quic_abhinavk@quicinc.com

On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

There is little point in having both connector and root bridge implementation in the same driver. Move connector's functionality to the bridge to let next bridge in chain to override it.

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

This patch break primary (edp) display

-- right half of screen garbled

-- screen shift vertically

below are error messages seen --

[   36.679216] panel-edp soc@0:edp_panel: No display modes [   36.687272] panel-edp soc@0:edp_panel: No display modes [   40.593709] panel-edp soc@0:edp_panel: No display modes [   40.600285] panel-edp soc@0:edp_panel: No display modes

---

drivers/gpu/drm/msm/dp/dp_display.c | 22 +++--- drivers/gpu/drm/msm/dp/dp_drm.c | 113 ++++++++++------------------ 2 files changed, 52 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 45f9a912ecc5..59e5e5b8e5b4 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1501,6 +1501,17 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,

dp_display->encoder = encoder;

• dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
• if (IS_ERR(dp_display->bridge)) {

• ret = PTR_ERR(dp_display->bridge);
  

• DRM_DEV_ERROR(dev->dev,
  

• 	"failed to create dp bridge: %d\n", ret);
  

• dp_display->bridge = NULL;
  

• return ret;
  

• }
• priv->bridges[priv->num_bridges++] = dp_display->bridge;
• dp_display->connector = dp_drm_connector_init(dp_display); if (IS_ERR(dp_display->connector)) { ret = PTR_ERR(dp_display->connector);

@@ -1514,17 +1525,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,

priv->connectors[priv->num_connectors++] = dp_display->connector;

• dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
• if (IS_ERR(dp_display->bridge)) {

• ret = PTR_ERR(dp_display->bridge);
  

• DRM_DEV_ERROR(dev->dev,
  

• 	"failed to create dp bridge: %d\n", ret);
  

• dp_display->bridge = NULL;
  

• return ret;
  

• }
• priv->bridges[priv->num_bridges++] = dp_display->bridge;
• return 0; }

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 80f59cf99089..57800b865fe6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -6,6 +6,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_atomic.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_crtc.h>

#include "msm_drv.h" @@ -20,24 +21,16 @@ struct msm_dp_bridge {

#define to_dp_display(x) container_of((x), struct msm_dp_bridge, bridge)

-struct dp_connector {

• struct drm_connector base;
• struct msm_dp *dp_display;

-}; -#define to_dp_connector(x) container_of(x, struct dp_connector, base)

• /**
• • dp_connector_detect - callback to determine if connector is connected
• • @conn: Pointer to drm connector structure
• • @force: Force detect setting from drm framework
• • Returns: Connector 'is connected' status

• • dp_bridge_detect - callback to determine if connector is connected
• • @bridge: Pointer to drm bridge structure
• • Returns: Bridge's 'is connected' status
  */

-static enum drm_connector_status dp_connector_detect(struct drm_connector *conn,

• bool force)
  

+static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge) { struct msm_dp *dp;

• dp = to_dp_connector(conn)->dp_display;

• dp = to_dp_display(bridge)->dp_display;

  DRM_DEBUG_DP("is_connected = %s\n", (dp->is_connected) ? "true" : "false");

@@ -47,11 +40,12 @@ static enum drm_connector_status dp_connector_detect(struct drm_connector *conn, }

/**

• • dp_connector_get_modes - callback to add drm modes via drm_mode_probed_add()

• • dp_bridge_get_modes - callback to add drm modes via drm_mode_probed_add()
• • @bridge: Poiner to drm bridge
  • @connector: Pointer to drm connector structure
  • Returns: Number of modes added
  */

-static int dp_connector_get_modes(struct drm_connector *connector) +static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *connector) { int rc = 0; struct msm_dp *dp; @@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct drm_connector *connector) if (!connector) return 0;

• dp = to_dp_connector(connector)->dp_display;

• dp = to_dp_display(bridge)->dp_display;

  dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL); if (!dp_mode)

@@ -102,18 +96,20 @@ static int dp_connector_get_modes(struct drm_connector *connector) }

/**

• • dp_connector_mode_valid - callback to determine if specified mode is valid
• • @connector: Pointer to drm connector structure

• • dp_bridge_mode_valid - callback to determine if specified mode is valid
• • @bridge: Pointer to drm bridge structure
• • @info: display info
  • @mode: Pointer to drm mode structure
  • Returns: Validity status for specified mode
  */

-static enum drm_mode_status dp_connector_mode_valid(

• struct drm_connector *connector,
  

• struct drm_display_mode *mode)
  

+static enum drm_mode_status dp_bridge_mode_valid(

• struct drm_bridge *bridge,
  

• const struct drm_display_info *info,
  

• const struct drm_display_mode *mode)
  

  { struct msm_dp *dp_disp;

• dp_disp = to_dp_connector(connector)->dp_display;

• dp_disp = to_dp_display(bridge)->dp_display;

  if ((dp_disp->max_pclk_khz <= 0) || (dp_disp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||

@@ -123,55 +119,6 @@ static enum drm_mode_status dp_connector_mode_valid( return dp_display_validate_mode(dp_disp, mode->clock); }

-static const struct drm_connector_funcs dp_connector_funcs = {

• .detect = dp_connector_detect,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = drm_connector_cleanup,
• .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 dp_connector_helper_funcs = {

• .get_modes = dp_connector_get_modes,
• .mode_valid = dp_connector_mode_valid,

-};

-/* connector initialization */ -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) -{

• struct drm_connector *connector = NULL;
• struct dp_connector *dp_connector;
• int ret;
• dp_connector = devm_kzalloc(dp_display->drm_dev->dev,

• 			sizeof(*dp_connector),
  

• 			GFP_KERNEL);
  

• if (!dp_connector)

• return ERR_PTR(-ENOMEM);
  

• dp_connector->dp_display = dp_display;
• connector = &dp_connector->base;
• ret = drm_connector_init(dp_display->drm_dev, connector,

• 	&dp_connector_funcs,
  

• 	dp_display->connector_type);
  

• if (ret)

• return ERR_PTR(ret);
  

• drm_connector_helper_add(connector, &dp_connector_helper_funcs);
• /*

• * Enable HPD to let hpd event is handled when cable is connected.
  

• */
  

• connector->polled = DRM_CONNECTOR_POLL_HPD;
• drm_connector_attach_encoder(connector, dp_display->encoder);
• return connector;

-}

• static void dp_bridge_mode_set(struct drm_bridge *drm_bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode)

@@ -211,6 +158,9 @@ static const struct drm_bridge_funcs dp_bridge_ops = { .disable = dp_bridge_disable, .post_disable = dp_bridge_post_disable, .mode_set = dp_bridge_mode_set,

• .mode_valid = dp_bridge_mode_valid,

• .get_modes = dp_bridge_get_modes,

• .detect = dp_bridge_detect, };

  struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,

@@ -228,7 +178,12 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi

bridge = &dp_bridge->bridge; bridge->funcs = &dp_bridge_ops;

• bridge->encoder = encoder;

• bridge->type = dp_display->connector_type;

• bridge->ops =

• DRM_BRIDGE_OP_DETECT |
  

• DRM_BRIDGE_OP_HPD |
  

• DRM_BRIDGE_OP_MODES;
  
  

  rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc) {

@@ -249,3 +204,17 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi

return bridge; }

+/* connector initialization */ +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) +{

• struct drm_connector *connector = NULL;
• connector = drm_bridge_connector_init(dp_display->drm_dev, dp_display->encoder);
• if (IS_ERR(connector))

• return connector;
  

• drm_connector_attach_encoder(connector, dp_display->encoder);
• return connector;

+}

On 19/02/2022 00:31, Kuogee Hsieh wrote:

On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:

...

There is little point in having both connector and root bridge implementation in the same driver. Move connector's functionality to the bridge to let next bridge in chain to override it.

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

This patch break primary (edp) display

-- right half of screen garbled

-- screen shift vertically

below are error messages seen --

[   36.679216] panel-edp soc@0:edp_panel: No display modes [   36.687272] panel-edp soc@0:edp_panel: No display modes [   40.593709] panel-edp soc@0:edp_panel: No display modes [   40.600285] panel-edp soc@0:edp_panel: No display modes

So, before the patch the drm core was getting modes from the drm_connector (which means, modes from drm driver itself). With this patch the panel-edp tries to get modes.

Could you please check, why panel_edp_get_modes() fails? Assuming that you use platform panel-edp binding (rather than 'edp-panel') could you please check you have either of the following: - ddc bus for EDID? - either num_timing or num_modes in your panel desc.

...

---

drivers/gpu/drm/msm/dp/dp_display.c |  22 +++---   drivers/gpu/drm/msm/dp/dp_drm.c     | 113 ++++++++++------------------   2 files changed, 52 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 45f9a912ecc5..59e5e5b8e5b4 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1501,6 +1501,17 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,       dp_display->encoder = encoder; +    dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder); +    if (IS_ERR(dp_display->bridge)) { +        ret = PTR_ERR(dp_display->bridge); +        DRM_DEV_ERROR(dev->dev, +            "failed to create dp bridge: %d\n", ret); +        dp_display->bridge = NULL; +        return ret; +    }

+    priv->bridges[priv->num_bridges++] = dp_display->bridge;

dp_display->connector = dp_drm_connector_init(dp_display);       if (IS_ERR(dp_display->connector)) {           ret = PTR_ERR(dp_display->connector); @@ -1514,17 +1525,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,       priv->connectors[priv->num_connectors++] = dp_display->connector; -    dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder); -    if (IS_ERR(dp_display->bridge)) { -        ret = PTR_ERR(dp_display->bridge); -        DRM_DEV_ERROR(dev->dev, -            "failed to create dp bridge: %d\n", ret); -        dp_display->bridge = NULL; -        return ret; -    }

-    priv->bridges[priv->num_bridges++] = dp_display->bridge;

return 0;   } diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 80f59cf99089..57800b865fe6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -6,6 +6,7 @@   #include <drm/drm_atomic_helper.h>   #include <drm/drm_atomic.h>   #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h>   #include <drm/drm_crtc.h>   #include "msm_drv.h" @@ -20,24 +21,16 @@ struct msm_dp_bridge {   #define to_dp_display(x)     container_of((x), struct msm_dp_bridge, bridge) -struct dp_connector { -    struct drm_connector base; -    struct msm_dp *dp_display; -}; -#define to_dp_connector(x) container_of(x, struct dp_connector, base)

/**

• • dp_connector_detect - callback to determine if connector is connected
• • @conn: Pointer to drm connector structure
• • @force: Force detect setting from drm framework
• • Returns: Connector 'is connected' status

• • dp_bridge_detect - callback to determine if connector is connected
• • @bridge: Pointer to drm bridge structure
• • Returns: Bridge's 'is connected' status

*/ -static enum drm_connector_status dp_connector_detect(struct drm_connector *conn, -        bool force) +static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)   {       struct msm_dp *dp; -    dp = to_dp_connector(conn)->dp_display; +    dp = to_dp_display(bridge)->dp_display;       DRM_DEBUG_DP("is_connected = %s\n",           (dp->is_connected) ? "true" : "false"); @@ -47,11 +40,12 @@ static enum drm_connector_status dp_connector_detect(struct drm_connector *conn,   }   /**

• • dp_connector_get_modes - callback to add drm modes via

drm_mode_probed_add()

• • dp_bridge_get_modes - callback to add drm modes via

drm_mode_probed_add()

• • @bridge: Poiner to drm bridge

* @connector: Pointer to drm connector structure    * Returns: Number of modes added    */ -static int dp_connector_get_modes(struct drm_connector *connector) +static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *connector)   {       int rc = 0;       struct msm_dp *dp; @@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct drm_connector *connector)       if (!connector)           return 0; -    dp = to_dp_connector(connector)->dp_display; +    dp = to_dp_display(bridge)->dp_display;       dp_mode = kzalloc(sizeof(*dp_mode),  GFP_KERNEL);       if (!dp_mode) @@ -102,18 +96,20 @@ static int dp_connector_get_modes(struct drm_connector *connector)   }   /**

• • dp_connector_mode_valid - callback to determine if specified mode

is valid

• • @connector: Pointer to drm connector structure

• • dp_bridge_mode_valid - callback to determine if specified mode is

valid

• • @bridge: Pointer to drm bridge structure
• • @info: display info

* @mode: Pointer to drm mode structure    * Returns: Validity status for specified mode    */ -static enum drm_mode_status dp_connector_mode_valid( -        struct drm_connector *connector, -        struct drm_display_mode *mode) +static enum drm_mode_status dp_bridge_mode_valid( +        struct drm_bridge *bridge, +        const struct drm_display_info *info, +        const struct drm_display_mode *mode)   {       struct msm_dp *dp_disp; -    dp_disp = to_dp_connector(connector)->dp_display; +    dp_disp = to_dp_display(bridge)->dp_display;       if ((dp_disp->max_pclk_khz <= 0) ||               (dp_disp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || @@ -123,55 +119,6 @@ static enum drm_mode_status dp_connector_mode_valid(       return dp_display_validate_mode(dp_disp, mode->clock);   } -static const struct drm_connector_funcs dp_connector_funcs = { -    .detect = dp_connector_detect, -    .fill_modes = drm_helper_probe_single_connector_modes, -    .destroy = drm_connector_cleanup, -    .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 dp_connector_helper_funcs = { -    .get_modes = dp_connector_get_modes, -    .mode_valid = dp_connector_mode_valid, -};

-/* connector initialization */ -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) -{ -    struct drm_connector *connector = NULL; -    struct dp_connector *dp_connector; -    int ret;

-    dp_connector = devm_kzalloc(dp_display->drm_dev->dev, -                    sizeof(*dp_connector), -                    GFP_KERNEL); -    if (!dp_connector) -        return ERR_PTR(-ENOMEM);

-    dp_connector->dp_display = dp_display;

-    connector = &dp_connector->base;

-    ret = drm_connector_init(dp_display->drm_dev, connector, -            &dp_connector_funcs, -            dp_display->connector_type); -    if (ret) -        return ERR_PTR(ret);

-    drm_connector_helper_add(connector, &dp_connector_helper_funcs);

-    /* -     * Enable HPD to let hpd event is handled when cable is connected. -     */ -    connector->polled = DRM_CONNECTOR_POLL_HPD;

-    drm_connector_attach_encoder(connector, dp_display->encoder);

-    return connector; -}

static void dp_bridge_mode_set(struct drm_bridge *drm_bridge,                   const struct drm_display_mode *mode,                   const struct drm_display_mode *adjusted_mode) @@ -211,6 +158,9 @@ static const struct drm_bridge_funcs dp_bridge_ops = {       .disable      = dp_bridge_disable,       .post_disable = dp_bridge_post_disable,       .mode_set     = dp_bridge_mode_set, +    .mode_valid   = dp_bridge_mode_valid, +    .get_modes    = dp_bridge_get_modes, +    .detect       = dp_bridge_detect,   };   struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, @@ -228,7 +178,12 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi       bridge = &dp_bridge->bridge;       bridge->funcs = &dp_bridge_ops; -    bridge->encoder = encoder; +    bridge->type = dp_display->connector_type;

+    bridge->ops = +        DRM_BRIDGE_OP_DETECT | +        DRM_BRIDGE_OP_HPD | +        DRM_BRIDGE_OP_MODES;       rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);       if (rc) { @@ -249,3 +204,17 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi       return bridge;   }

+/* connector initialization */ +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) +{ +    struct drm_connector *connector = NULL;

+    connector = drm_bridge_connector_init(dp_display->drm_dev, dp_display->encoder); +    if (IS_ERR(connector)) +        return connector;

+    drm_connector_attach_encoder(connector, dp_display->encoder);

+    return connector; +}