| Message ID | 20220211224006.1797846-4-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Simplify and correct msm/dp bridge implementation | expand |
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
Quoting Dmitry Baryshkov (2022-02-11 14:40:04) > 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> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
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 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).
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>
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
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(-)