| Message ID | 20220107020132.587811-3-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/7] drm/msm/dp: fix panel bridge attachment | expand |
Quoting Dmitry Baryshkov (2022-01-06 18:01:27) > Currently DP driver will allocate panel bridge for eDP panels. > 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. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > 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 | 26 ++++++++------------------ > drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- > 5 files changed, 13 insertions(+), 23 deletions(-) I like this one, it certainly makes it easier to understand. > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c > index a7acc23f742b..5de21f3d0812 100644 > --- a/drivers/gpu/drm/msm/dp/dp_parser.c > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c > @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) > if (rc) > return rc; > > - if (connector_type == DRM_MODE_CONNECTOR_eDP) { It feels like this is on purpose, but I don't see any comment so I have no idea. I think qcom folks are concerned about changing how not eDP works. I'll have to test it out locally. > - rc = dp_parser_find_panel(parser); > - if (rc) > - return rc; > - } > + rc = dp_parser_find_next_bridge(parser); > + if (rc) > + return rc; > > /* Map the corresponding regulator information according to > * version. Currently, since we only have one supported platform,
On 07/01/2022 06:42, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-01-06 18:01:27) >> Currently DP driver will allocate panel bridge for eDP panels. >> 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. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> 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 | 26 ++++++++------------------ >> drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- >> 5 files changed, 13 insertions(+), 23 deletions(-) > > I like this one, it certainly makes it easier to understand. > >> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c >> index a7acc23f742b..5de21f3d0812 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_parser.c >> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c >> @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) >> if (rc) >> return rc; >> >> - if (connector_type == DRM_MODE_CONNECTOR_eDP) { > > It feels like this is on purpose, but I don't see any comment so I have > no idea. I think qcom folks are concerned about changing how not eDP > works. I'll have to test it out locally. Ah, another thing that should go into the commit message. Current situation: - DP: no external bridges supported. - eDP: only a drm_panel wrapped into the panel bridge After this patch: - both DP and eDP support any chain of bridges attached. While the change means nothing for the DP (IIUC, it will not have any bridges), it simplifies the code path, lowering the amount of checks. And for eDP this means that we can attach any eDP-to-something bridges (e.g. NXP PTN3460). Well... After re-checking the source code for devm_drm_of_get_bridge/drm_of_find_panel_or_bridge I should probably revert removal of the check. The function will return -ENODEV if neither bridge nor panel are specified. > >> - rc = dp_parser_find_panel(parser); >> - if (rc) >> - return rc; >> - } >> + rc = dp_parser_find_next_bridge(parser); >> + if (rc) >> + return rc; >> >> /* Map the corresponding regulator information according to >> * version. Currently, since we only have one supported platform,
On 1/6/2022 9:26 PM, Dmitry Baryshkov wrote: > On 07/01/2022 06:42, Stephen Boyd wrote: >> Quoting Dmitry Baryshkov (2022-01-06 18:01:27) >>> Currently DP driver will allocate panel bridge for eDP panels. >>> 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. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> 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 | 26 ++++++++------------------ >>> drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- >>> 5 files changed, 13 insertions(+), 23 deletions(-) >> >> I like this one, it certainly makes it easier to understand. >> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c >>> b/drivers/gpu/drm/msm/dp/dp_parser.c >>> index a7acc23f742b..5de21f3d0812 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_parser.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c >>> @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser >>> *parser, int connector_type) >>> if (rc) >>> return rc; >>> >>> - if (connector_type == DRM_MODE_CONNECTOR_eDP) { >> >> It feels like this is on purpose, but I don't see any comment so I have >> no idea. I think qcom folks are concerned about changing how not eDP >> works. I'll have to test it out locally. > > Ah, another thing that should go into the commit message. > > Current situation: > - DP: no external bridges supported. > - eDP: only a drm_panel wrapped into the panel bridge > > After this patch: > - both DP and eDP support any chain of bridges attached. > > > While the change means nothing for the DP (IIUC, it will not have any > bridges), it simplifies the code path, lowering the amount of checks. > > And for eDP this means that we can attach any eDP-to-something bridges > (e.g. NXP PTN3460). > > > Well... After re-checking the source code for > devm_drm_of_get_bridge/drm_of_find_panel_or_bridge I should probably > revert removal of the check. The function will return -ENODEV if > neither bridge nor panel are specified. > I am new to drm and confusing with bridge here. Isn't bridge used to bridging two different kind of interface together? for example, dsi <--> bridge <--> dp. why edp need bridge here? Can you give me more info regrading what bridge try to do here. >> >>> - rc = dp_parser_find_panel(parser); >>> - if (rc) >>> - return rc; >>> - } >>> + rc = dp_parser_find_next_bridge(parser); >>> + if (rc) >>> + return rc; >>> >>> /* Map the corresponding regulator information according to >>> * version. Currently, since we only have one supported >>> platform, > >
On Wed, 12 Jan 2022 at 02:12, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > On 1/6/2022 9:26 PM, Dmitry Baryshkov wrote: > > On 07/01/2022 06:42, Stephen Boyd wrote: > >> Quoting Dmitry Baryshkov (2022-01-06 18:01:27) > >>> Currently DP driver will allocate panel bridge for eDP panels. > >>> 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. > >>> > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> --- > >>> 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 | 26 ++++++++------------------ > >>> drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- > >>> 5 files changed, 13 insertions(+), 23 deletions(-) > >> > >> I like this one, it certainly makes it easier to understand. > >> > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c > >>> b/drivers/gpu/drm/msm/dp/dp_parser.c > >>> index a7acc23f742b..5de21f3d0812 100644 > >>> --- a/drivers/gpu/drm/msm/dp/dp_parser.c > >>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c > >>> @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser > >>> *parser, int connector_type) > >>> if (rc) > >>> return rc; > >>> > >>> - if (connector_type == DRM_MODE_CONNECTOR_eDP) { > >> > >> It feels like this is on purpose, but I don't see any comment so I have > >> no idea. I think qcom folks are concerned about changing how not eDP > >> works. I'll have to test it out locally. > > > > Ah, another thing that should go into the commit message. > > > > Current situation: > > - DP: no external bridges supported. > > - eDP: only a drm_panel wrapped into the panel bridge > > > > After this patch: > > - both DP and eDP support any chain of bridges attached. > > > > > > While the change means nothing for the DP (IIUC, it will not have any > > bridges), it simplifies the code path, lowering the amount of checks. > > > > And for eDP this means that we can attach any eDP-to-something bridges > > (e.g. NXP PTN3460). > > > > > > Well... After re-checking the source code for > > devm_drm_of_get_bridge/drm_of_find_panel_or_bridge I should probably > > revert removal of the check. The function will return -ENODEV if > > neither bridge nor panel are specified. > > > I am new to drm and confusing with bridge here. > > Isn't bridge used to bridging two different kind of interface together? > > for example, dsi <--> bridge <--> dp. > > why edp need bridge here? > > Can you give me more info regrading what bridge try to do here. First, there are bridges converting the eDP interface to another interface. The mentioned NXP PTN3460 converts (embedded) DisplayPort to LVDS. Second (and this is the case here) drm_bridge can be used to wrap drm_panel (panel-bridge), so that the driver doesn't have to care about the drm_panel interface. Last, but not least, external display connectors can also be abstracted as bridges (see display-connector.c). This becomes even more appealing as the driver can then switch to drm_bridge_connector, supporting any kinds of pipelines attached to the encoder, supporting any kind of converters, panel or external connector pipelines.Think about the following (sometimes crazy, but possible) examples. With drm_bridge/panel-bridge/display-connector/drm_bridge_connector there is no difference for the driver at all: - DP encoder ⇒ DP port ⇒ DP monitor - DP encoder ⇒ DP connector supporting generic GPIO as HPD ⇒ DP port ⇒ DP monitor - eDP encoder ⇒ eDP panel - 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 > >> > >>> - rc = dp_parser_find_panel(parser); > >>> - if (rc) > >>> - return rc; > >>> - } > >>> + rc = dp_parser_find_next_bridge(parser); > >>> + if (rc) > >>> + return rc; > >>> > >>> /* Map the corresponding regulator information according to > >>> * version. Currently, since we only have one supported > >>> platform, > > > >
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 7cc4d21f2091..184a1d1470e6 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -246,7 +246,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..5de21f3d0812 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -265,22 +265,14 @@ 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; - 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); + parser->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(parser->next_bridge)) { + DRM_ERROR("failed to create next bridge\n"); + return PTR_ERR(parser->next_bridge); } return 0; @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc; - if (connector_type == DRM_MODE_CONNECTOR_eDP) { - rc = dp_parser_find_panel(parser); - if (rc) - return rc; - } + rc = dp_parser_find_next_bridge(parser); + if (rc) + return rc; /* Map the corresponding regulator information according to * version. Currently, since we only have one supported platform, 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); };
Currently DP driver will allocate panel bridge for eDP panels. 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. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- 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 | 26 ++++++++------------------ drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 13 insertions(+), 23 deletions(-)