Message-ID: <f3abbab4-f390-414f-8374-cc6f9e75faf4@linaro.org>
Date:   Sat, 7 May 2022 00:17:54 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Abhinav Kumar <quic_abhinavk@...cinc.com>,
        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:     quic_kalyant@...cinc.com, bjorn.andersson@...aro.org,
        quic_vproddut@...cinc.com, airlied@...ux.ie, dianders@...omium.org,
        steev@...i.org, quic_khsieh@...cinc.com, robdclark@...il.com,
        seanpaul@...omium.org, daniel@...ll.ch, quic_aravindh@...cinc.com,
        swboyd@...omium.org, sean@...rly.run
Subject: Re: [Freedreno] [PATCH v10 1/4] drm/msm/dp: Add eDP support via
 aux_bus

On 07/05/2022 00:17, Abhinav Kumar wrote:
> 
> 
> On 5/6/2022 2:05 PM, Dmitry Baryshkov wrote:
>> On 07/05/2022 00:03, Abhinav Kumar wrote:
>>>
>>>
>>> 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?
>>
>> Yes, it helped us to uncover several error. But I'd still prefer to 
>> have EPROBE_DEFER being returned earlier rather than later.
> 
> Alright, point noted to try moving this earlier.
> 
> We will be following this up with rounds of cleanups to implement the 
> suggestions given by you and Doug earlier.
> 
> This point shall be noted too.

Thanks a lot.

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


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists