diff mbox series

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

Message ID 20220211224006.1797846-5-dmitry.baryshkov@linaro.org
State Accepted
Commit a586191c141ef413d899e136c1d06407c9454971
Headers show
Series Simplify and correct msm/dp bridge implementation | expand

Commit Message

Dmitry Baryshkov Feb. 11, 2022, 10:40 p.m. UTC
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>
---
 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(-)

Comments

Kuogee Hsieh Feb. 18, 2022, 9:31 p.m. UTC | #1
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

> ---
>   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;
> +}
Dmitry Baryshkov Feb. 18, 2022, 9:52 p.m. UTC | #2
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

Thanks for testing this series!

Could you please post the result of `modetest -c` after this patch?


> 
>> ---
>>   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;
>> +}
Dmitry Baryshkov Feb. 18, 2022, 10:32 p.m. UTC | #3
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;
>> +}
Stephen Boyd Feb. 19, 2022, 12:55 a.m. UTC | #4
Quoting Dmitry Baryshkov (2022-02-18 14:32:53)
> 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?

I don't see anywhere where the ddc pointer is set for the dp bridge in
msm_dp_bridge_init(). Is that required though? I'd think simple panel is
still being used here so reading EDID isn't required.

> - either num_timing or num_modes in your panel desc.
>
Dmitry Baryshkov Feb. 19, 2022, 2:22 a.m. UTC | #5
On Sat, 19 Feb 2022 at 03:55, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2022-02-18 14:32:53)
> > 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?
>
> I don't see anywhere where the ddc pointer is set for the dp bridge in
> msm_dp_bridge_init(). Is that required though? I'd think simple panel is
> still being used here so reading EDID isn't required.

I meant the 'ddc-i2c-bus' property for the corresponding eDP panel.

> > - either num_timing or num_modes in your panel desc.

After reading the panel-edp's code I don't have another cause for
panel_edp_get_modes(). It should either have a DDC bus specified using
the mentioned device tree property, or it should have specified the
timings.

Kuogee, which platform were you using when testing this patch? Could
you please share the dts fragment?
Kuogee Hsieh Feb. 23, 2022, 5:21 p.m. UTC | #6
On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote:
> On Sat, 19 Feb 2022 at 03:55, Stephen Boyd <swboyd@chromium.org> wrote:
>> Quoting Dmitry Baryshkov (2022-02-18 14:32:53)
>>> 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?
>> I don't see anywhere where the ddc pointer is set for the dp bridge in
>> msm_dp_bridge_init(). Is that required though? I'd think simple panel is
>> still being used here so reading EDID isn't required.
> I meant the 'ddc-i2c-bus' property for the corresponding eDP panel.
>
>>> - either num_timing or num_modes in your panel desc.
> After reading the panel-edp's code I don't have another cause for
> panel_edp_get_modes(). It should either have a DDC bus specified using
> the mentioned device tree property, or it should have specified the
> timings.
>
> Kuogee, which platform were you using when testing this patch? Could
> you please share the dts fragment?

I cherry-picked your patches on top of our internal release which is 
usually have some (or many) patches behind msm-next.

where is "ddc-i2c-bus" located?

                         msm_edp: edp@aea0000 {
                                 compatible = "qcom,sc7280-edp";

                                 reg = <0 0xaea0000 0 0x200>,
                                       <0 0xaea0200 0 0x200>,
                                       <0 0xaea0400 0 0xc00>,
                                       <0 0xaea1000 0 0x400>;

                                 interrupt-parent = <&mdss>;
                                 interrupts = <14>;

                                 clocks = <&rpmhcc RPMH_CXO_CLK>,
                                          <&gcc GCC_EDP_CLKREF_EN>,
                                          <&dispcc DISP_CC_MDSS_AHB_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_AUX_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_LINK_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_PIXEL_CLK>;
                                 clock-names = "core_xo",
                                               "core_ref",
                                               "core_iface",
                                               "core_aux",
                                               "ctrl_link",
                                               "ctrl_link_iface",
                                               "stream_pixel";
                                 #clock-cells = <1>;
                                 assigned-clocks = <&dispcc 
DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
                                                   <&dispcc 
DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
                                 assigned-clock-parents = <&edp_phy 0>, 
<&edp_phy 1>;

                                 phys = <&edp_phy>;
                                 phy-names = "dp";

                                 operating-points-v2 = <&edp_opp_table>;
                                 power-domains = <&rpmhpd SC7280_CX>;

                                 #address-cells = <1>;
                                 #size-cells = <0>;

                                 status = "disabled";

                                 ports {
                                         #address-cells = <1>;
                                         #size-cells = <0>;
                                         port@0 {
                                                 reg = <0>;
                                                 edp_in: endpoint {
remote-endpoint = <&dpu_intf5_out>;
                                                 };
                                         };
                                 };

                             edp_opp_table: opp-table {
                                         compatible = "operating-points-v2";

                                         opp-160000000 {
                                                 opp-hz = /bits/ 64 
<160000000>;
                                                 required-opps = 
<&rpmhpd_opp_low_svs>;
                                         };

                                         opp-270000000 {
                                                 opp-hz = /bits/ 64 
<270000000>;
                                                 required-opps = 
<&rpmhpd_opp_svs>;
                                         };

                                         opp-540000000 {
                                                 opp-hz = /bits/ 64 
<540000000>;
                                                 required-opps = 
<&rpmhpd_opp_nom>;
                                         };

                                         opp-810000000 {
                                                 opp-hz = /bits/ 64 
<810000000>;
                                                 required-opps = 
<&rpmhpd_opp_nom>;
                                         };
                                 };
                         };
Kuogee Hsieh Feb. 23, 2022, 6:27 p.m. UTC | #7
On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
> On 23/02/2022 20:21, Kuogee Hsieh wrote:
>>
>> On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote:
>>> On Sat, 19 Feb 2022 at 03:55, Stephen Boyd <swboyd@chromium.org> wrote:
>>>> Quoting Dmitry Baryshkov (2022-02-18 14:32:53)
>>>>> 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?
>>>> I don't see anywhere where the ddc pointer is set for the dp bridge in
>>>> msm_dp_bridge_init(). Is that required though? I'd think simple 
>>>> panel is
>>>> still being used here so reading EDID isn't required.
>>> I meant the 'ddc-i2c-bus' property for the corresponding eDP panel.
>>>
>>>>> - either num_timing or num_modes in your panel desc.
>>> After reading the panel-edp's code I don't have another cause for
>>> panel_edp_get_modes(). It should either have a DDC bus specified using
>>> the mentioned device tree property, or it should have specified the
>>> timings.
>>>
>>> Kuogee, which platform were you using when testing this patch? Could
>>> you please share the dts fragment?
>>
>> I cherry-picked your patches on top of our internal release which is 
>> usually have some (or many) patches behind msm-next.
>>
>> where is "ddc-i2c-bus" located?
>
> In the panel device node.
>
> Can you please share it too?


&soc {
         edp_power_supply: edp_power {
                 compatible = "regulator-fixed";
                 regulator-name = "edp_backlight_power";

                 regulator-always-on;
                 regulator-boot-on;
         };

         edp_backlight: edp_backlight {
                 compatible = "pwm-backlight";

                 pwms = <&pm8350c_pwm 3 65535>;
                 power-supply = <&edp_power_supply>;
                 enable-gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>;

                 pinctrl-names = "default";
                 pinctrl-0 = <&backlight_pwm_default>;
         };

         edp_panel: edp_panel {
                 compatible = "sharp_lq140m1jw46";

                 pinctrl-names = "default";
                 pinctrl-0 = <&edp_hot_plug_det>, 
<&edp_panel_power_default>;

                 power-supply = <&edp_power_supply>;
                 backlight = <&edp_backlight>;

                 ports {
                         #address-cells = <1>;
                         #size-cells = <0>;
                         port@0 {
                                 reg = <0>;
                                 edp_panel_in: endpoint {
                                         remote-endpoint = <&edp_out>;
                                 };
                         };
                 };
         };
};


>
>>
>>                          msm_edp: edp@aea0000 {
>>                                  compatible = "qcom,sc7280-edp";
>>
>>                                  reg = <0 0xaea0000 0 0x200>,
>>                                        <0 0xaea0200 0 0x200>,
>>                                        <0 0xaea0400 0 0xc00>,
>>                                        <0 0xaea1000 0 0x400>;
>>
>>                                  interrupt-parent = <&mdss>;
>>                                  interrupts = <14>;
>>
>>                                  clocks = <&rpmhcc RPMH_CXO_CLK>,
>>                                           <&gcc GCC_EDP_CLKREF_EN>,
>>                                           <&dispcc 
>> DISP_CC_MDSS_AHB_CLK>,
>>                                           <&dispcc 
>> DISP_CC_MDSS_EDP_AUX_CLK>,
>>                                           <&dispcc 
>> DISP_CC_MDSS_EDP_LINK_CLK>,
>>                                           <&dispcc 
>> DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
>>                                           <&dispcc 
>> DISP_CC_MDSS_EDP_PIXEL_CLK>;
>>                                  clock-names = "core_xo",
>>                                                "core_ref",
>>                                                "core_iface",
>>                                                "core_aux",
>>                                                "ctrl_link",
>> "ctrl_link_iface",
>>                                                "stream_pixel";
>>                                  #clock-cells = <1>;
>>                                  assigned-clocks = <&dispcc 
>> DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
>> <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
>>                                  assigned-clock-parents = <&edp_phy 
>> 0>, <&edp_phy 1>;
>>
>>                                  phys = <&edp_phy>;
>>                                  phy-names = "dp";
>>
>>                                  operating-points-v2 = <&edp_opp_table>;
>>                                  power-domains = <&rpmhpd SC7280_CX>;
>>
>>                                  #address-cells = <1>;
>>                                  #size-cells = <0>;
>>
>>                                  status = "disabled";
>>
>>                                  ports {
>>                                          #address-cells = <1>;
>>                                          #size-cells = <0>;
>>                                          port@0 {
>>                                                  reg = <0>;
>>                                                  edp_in: endpoint {
>> remote-endpoint = <&dpu_intf5_out>;
>>                                                  };
>>                                          };
>>                                  };
>>
>>                              edp_opp_table: opp-table {
>>                                          compatible = 
>> "operating-points-v2";
>>
>>                                          opp-160000000 {
>>                                                  opp-hz = /bits/ 64 
>> <160000000>;
>>                                                  required-opps = 
>> <&rpmhpd_opp_low_svs>;
>>                                          };
>>
>>                                          opp-270000000 {
>>                                                  opp-hz = /bits/ 64 
>> <270000000>;
>>                                                  required-opps = 
>> <&rpmhpd_opp_svs>;
>>                                          };
>>
>>                                          opp-540000000 {
>>                                                  opp-hz = /bits/ 64 
>> <540000000>;
>>                                                  required-opps = 
>> <&rpmhpd_opp_nom>;
>>                                          };
>>
>>                                          opp-810000000 {
>>                                                  opp-hz = /bits/ 64 
>> <810000000>;
>>                                                  required-opps = 
>> <&rpmhpd_opp_nom>;
>>                                          };
>>                                  };
>>                          };
>>
>
>
diff mbox series

Patch

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