Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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;
>> +}


-- 
With best wishes
Dmitry