| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <30e4f4db-9c4c-af90-f1d2-ec39019f95a5@quicinc.com>
Date: Fri, 6 May 2022 14:03:43 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
<dri-devel@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
<freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>
CC: <robdclark@...il.com>, <seanpaul@...omium.org>,
<swboyd@...omium.org>, <quic_kalyant@...cinc.com>,
<dianders@...omium.org>, <quic_khsieh@...cinc.com>,
<bjorn.andersson@...aro.org>, <sean@...rly.run>,
<airlied@...ux.ie>, <daniel@...ll.ch>, <quic_vproddut@...cinc.com>,
<quic_aravindh@...cinc.com>, <steev@...i.org>
Subject: Re: [PATCH v10 1/4] drm/msm/dp: Add eDP support via aux_bus
On 5/6/2022 1:49 PM, Dmitry Baryshkov wrote:
> On 25/04/2022 14:44, Sankeerth Billakanti wrote:
>> This patch adds support for generic eDP sink through aux_bus. The eDP/DP
>> controller driver should support aux transactions originating from the
>> panel-edp driver and hence should be initialized and ready.
>>
>> The panel bridge supporting the panel should be ready before the bridge
>> connector is initialized. The generic panel probe needs the controller
>> resources to be enabled to support the aux transactions originating from
>> the panel probe.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@...cinc.com>
>> Reviewed-by: Douglas Anderson <dianders@...omium.org>
>> Reviewed-by: Stephen Boyd <swboyd@...omium.org>
>
> An additional side effect from this patch. Previously missing panel
> would have caused the bind error. Now it is the dp_modeset_init error,
> which translates to kms_hw_init returning -517. I kind ask to move the
> next_bridge acquisition back to the dp_bind in one of the followup patches.
>
This is true. But the end result would be same isnt it?
When dp_display_bind() failed earlier, it would cause master bind
failure too due to component model.
Even now, it causes the same result?
>> ---
>> Changes in v10:
>> - modify the error handling condition
>> - modify the kernel doc
>>
>> Changes in v9:
>> - add comments for panel probe
>> - modify the error handling checks
>>
>> Changes in v8:
>> - handle corner cases
>> - add comment for the bridge ops
>>
>> Changes in v7:
>> - aux_bus is mandatory for eDP
>> - connector type check modified to just check for eDP
>>
>> Changes in v6:
>> - Remove initialization
>> - Fix aux_bus node leak
>> - Split the patches
>>
>> drivers/gpu/drm/msm/dp/dp_display.c | 72
>> ++++++++++++++++++++++++++++++++++---
>> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
>> drivers/gpu/drm/msm/dp/dp_drm.c | 21 ++++++++---
>> drivers/gpu/drm/msm/dp/dp_parser.c | 23 ++----------
>> drivers/gpu/drm/msm/dp/dp_parser.h | 14 +++++++-
>> 5 files changed, 101 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index d7a19d6..f772d84 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -10,6 +10,7 @@
>> #include <linux/component.h>
>> #include <linux/of_irq.h>
>> #include <linux/delay.h>
>> +#include <drm/dp/drm_dp_aux_bus.h>
>> #include "msm_drv.h"
>> #include "msm_kms.h"
>> @@ -259,14 +260,12 @@ static int dp_display_bind(struct device *dev,
>> struct device *master,
>> dp->dp_display.drm_dev = drm;
>> priv->dp[dp->id] = &dp->dp_display;
>> - rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
>> + rc = dp->parser->parse(dp->parser);
>> if (rc) {
>> DRM_ERROR("device tree parsing failed\n");
>> goto end;
>> }
>> - dp->dp_display.next_bridge = dp->parser->next_bridge;
>> -
>> dp->aux->drm_dev = drm;
>> rc = dp_aux_register(dp->aux);
>> if (rc) {
>> @@ -1319,6 +1318,8 @@ static int dp_display_probe(struct
>> platform_device *pdev)
>> dp->pdev = pdev;
>> dp->name = "drm_dp";
>> dp->dp_display.connector_type = desc->connector_type;
>> + dp->dp_display.is_edp =
>> + (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
>> rc = dp_init_sub_modules(dp);
>> if (rc) {
>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>> *dp_display)
>> dp_hpd_event_setup(dp);
>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>> + if (!dp_display->is_edp)
>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>> }
>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
>> *minor)
>> @@ -1530,6 +1532,64 @@ void msm_dp_debugfs_init(struct msm_dp
>> *dp_display, struct drm_minor *minor)
>> }
>> }
>> +static int dp_display_get_next_bridge(struct msm_dp *dp)
>> +{
>> + int rc;
>> + struct dp_display_private *dp_priv;
>> + struct device_node *aux_bus;
>> + struct device *dev;
>> +
>> + dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> + dev = &dp_priv->pdev->dev;
>> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> +
>> + if (aux_bus && dp->is_edp) {
>> + dp_display_host_init(dp_priv);
>> + dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> + dp_display_host_phy_init(dp_priv);
>> + enable_irq(dp_priv->irq);
>> +
>> + /*
>> + * The code below assumes that the panel will finish probing
>> + * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> + * This isn't a great assumption since it will fail if the
>> + * panel driver is probed asynchronously but is the best we
>> + * can do without a bigger driver reorganization.
>> + */
>> + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> + of_node_put(aux_bus);
>> + if (rc)
>> + goto error;
>> + } else if (dp->is_edp) {
>> + DRM_ERROR("eDP aux_bus not found\n");
>> + return -ENODEV;
>> + }
>> +
>> + /*
>> + * External bridges are mandatory for eDP interfaces: one has to
>> + * provide at least an eDP panel (which gets wrapped into
>> panel-bridge).
>> + *
>> + * For DisplayPort interfaces external bridges are optional, so
>> + * silently ignore an error if one is not present (-ENODEV).
>> + */
>> + rc = dp_parser_find_next_bridge(dp_priv->parser);
>> + if (!dp->is_edp && rc == -ENODEV)
>> + return 0;
>> +
>> + if (!rc) {
>> + dp->next_bridge = dp_priv->parser->next_bridge;
>> + return 0;
>> + }
>> +
>> +error:
>> + if (dp->is_edp) {
>> + disable_irq(dp_priv->irq);
>> + dp_display_host_phy_exit(dp_priv);
>> + dp_display_host_deinit(dp_priv);
>> + }
>> + return rc;
>> +}
>> +
>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>> *dev,
>> struct drm_encoder *encoder)
>> {
>> @@ -1553,6 +1613,10 @@ int msm_dp_modeset_init(struct msm_dp
>> *dp_display, struct drm_device *dev,
>> dp_display->encoder = encoder;
>> + ret = dp_display_get_next_bridge(dp_display);
>> + if (ret)
>> + return ret;
>> +
>> dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
>> if (IS_ERR(dp_display->bridge)) {
>> ret = PTR_ERR(dp_display->bridge);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
>> b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 49a1d89..1377cc3 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -21,6 +21,7 @@ struct msm_dp {
>> bool audio_enabled;
>> bool power_on;
>> unsigned int connector_type;
>> + bool is_edp;
>> hdmi_codec_plugged_cb plugged_cb;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
>> b/drivers/gpu/drm/msm/dp/dp_drm.c
>> index 7ce1aca..8a75c55 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>> @@ -114,10 +114,23 @@ struct drm_bridge *dp_bridge_init(struct msm_dp
>> *dp_display, struct drm_device *
>> bridge->funcs = &dp_bridge_ops;
>> bridge->type = dp_display->connector_type;
>> - bridge->ops =
>> - DRM_BRIDGE_OP_DETECT |
>> - DRM_BRIDGE_OP_HPD |
>> - DRM_BRIDGE_OP_MODES;
>> + /*
>> + * Many ops only make sense for DP. Why?
>> + * - Detect/HPD are used by DRM to know if a display is _physically_
>> + * there, not whether the display is powered on / finished
>> initting.
>> + * On eDP we assume the display is always there because you can't
>> + * know until power is applied. If we don't implement the ops
>> DRM will
>> + * assume our display is always there.
>> + * - Currently eDP mode reading is driven by the panel driver. This
>> + * allows the panel driver to properly power itself on to read the
>> + * modes.
>> + */
>> + if (!dp_display->is_edp) {
>> + 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) {
>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
>> b/drivers/gpu/drm/msm/dp/dp_parser.c
>> index 1056b8d..4bdbf91 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
>> @@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser)
>> return 0;
>> }
>> -static int dp_parser_find_next_bridge(struct dp_parser *parser)
>> +int dp_parser_find_next_bridge(struct dp_parser *parser)
>> {
>> struct device *dev = &parser->pdev->dev;
>> struct drm_bridge *bridge;
>> @@ -279,7 +279,7 @@ static int dp_parser_find_next_bridge(struct
>> dp_parser *parser)
>> return 0;
>> }
>> -static int dp_parser_parse(struct dp_parser *parser, int connector_type)
>> +static int dp_parser_parse(struct dp_parser *parser)
>> {
>> int rc = 0;
>> @@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser
>> *parser, int connector_type)
>> if (rc)
>> return rc;
>> - /*
>> - * External bridges are mandatory for eDP interfaces: one has to
>> - * provide at least an eDP panel (which gets wrapped into
>> panel-bridge).
>> - *
>> - * For DisplayPort interfaces external bridges are optional, so
>> - * silently ignore an error if one is not present (-ENODEV).
>> - */
>> - 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
>> * version. Currently, since we only have one supported platform,
>> * mapping the regulator directly.
>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
>> b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index d371bae..3a4d797 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
>> @@ -125,7 +125,7 @@ struct dp_parser {
>> u32 max_dp_lanes;
>> struct drm_bridge *next_bridge;
>> - int (*parse)(struct dp_parser *parser, int connector_type);
>> + int (*parse)(struct dp_parser *parser);
>> };
>> /**
>> @@ -141,4 +141,16 @@ struct dp_parser {
>> */
>> struct dp_parser *dp_parser_get(struct platform_device *pdev);
>> +/**
>> + * dp_parser_find_next_bridge() - find an additional bridge to DP
>> + *
>> + * @parser: dp_parser data from client
>> + *
>> + * This function is used to find any additional bridge attached to
>> + * the DP controller. The eDP interface requires a panel bridge.
>> + *
>> + * Return: 0 if able to get the bridge, otherwise negative errno for
>> failure.
>> + */
>> +int dp_parser_find_next_bridge(struct dp_parser *parser);
>> +
>> #endif
>
>
Powered by blists - more mailing lists