drm/analogix_dp: Ensure the panel is properly prepared/unprepared
diff mbox

Message ID 1469819791-31860-1-git-send-email-seanpaul@chromium.org
State New
Headers show

Commit Message

Sean Paul July 29, 2016, 7:16 p.m. UTC
Instead of just preparing the panel on bind, actually prepare/unprepare
during modeset/disable. The panel must be prepared in order to read hpd
status, so we need to refcount the prepares in order to ensure we don't
accidentally turn the panel off at the wrong time.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---


Hi Yakir,
This is what I was talking about upthread. I've tested it and it seems to be working.

What do you think?

Sean



drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 +++++++++++++++++-----
 1 file changed, 37 insertions(+), 11 deletions(-)

Comments

Yakir Yang Aug. 1, 2016, 3:27 a.m. UTC | #1
Sean,

On 07/30/2016 03:16 AM, Sean Paul wrote:
> Instead of just preparing the panel on bind, actually prepare/unprepare
> during modeset/disable. The panel must be prepared in order to read hpd
> status, so we need to refcount the prepares in order to ensure we don't
> accidentally turn the panel off at the wrong time.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>
>
> Hi Yakir,
> This is what I was talking about upthread. I've tested it and it seems to be working.
>
> What do you think?

Thanks for your patch, and it works. But I have introduced two 
questions, and I haven't found a way to fixed them.

> Sean
>
>
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 +++++++++++++++++-----
>   1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 32715da..7b764a4 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -960,11 +960,27 @@ enum drm_connector_status
>   analogix_dp_detect(struct drm_connector *connector, bool force)
>   {
>   	struct analogix_dp_device *dp = to_dp(connector);
> +	enum drm_connector_status status = connector_status_disconnected;
> +	int ret;
>   
> -	if (analogix_dp_detect_hpd(dp))
> -		return connector_status_disconnected;
> +	if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
> +		ret = drm_panel_prepare(dp->plat_data->panel);
> +		if (ret) {
> +			DRM_ERROR("failed to setup panel (%d)\n", ret);
> +			return connector_status_disconnected;
> +		}
> +	}
> +
> +	if (!analogix_dp_detect_hpd(dp))
> +		status = connector_status_connected;
> +
> +	if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
> +		ret = drm_panel_unprepare(dp->plat_data->panel);
> +		if (ret)
> +			DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +	}
>   
> -	return connector_status_connected;
> +	return status;

1. Panel would flicker at system boot time. Your patch would flash the 
panel power in connector->detect() function when dp->dpms_mode isn't 
DRM_MODE_DPMS_OFF. So when userspace keep detect the connector status in 
boot time, we could see panel would flicker (light up for a while, and 
turn off again, and keep loop for several time). I have copied some 
kernel logs:


[   11.065267] YKK --------- analogix_dp_detect:1052
[   11.729596] YKK --------- analogix_dp_get_modes:1016
[   11.737608] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.749229] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.760799] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   13.315962] YKK --------- analogix_dp_detect:1052
[   13.984702] YKK --------- analogix_dp_get_modes:1016
[   13.992977] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.004414] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.015842] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
[   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery 
success
[   14.319130] rockchip-dp ff970000.edp: Link Training success!
[   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
[   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
[   14.443585] rockchip-dp ff970000.edp: unable to config video


>   }
>   
>   static void analogix_dp_connector_destroy(struct drm_connector *connector)
> @@ -1035,6 +1051,18 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>   	return 0;
>   }
>   
> +static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct analogix_dp_device *dp = bridge->driver_private;
> +	int ret;
> +
> +	if (dp->plat_data->panel) {
> +		ret = drm_panel_prepare(dp->plat_data->panel);
> +		if (ret)
> +			DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +	}

2. Driver would failed to read EDID in some case. Panel would only be 
powered up in bridge->pre_enable() function which later than 
connector->get_modes() function, and this would caused DPCD transfer 
failed in analogix_dp_handle_edid(). This seem won't caused too big 
issue, cause userspace would read EDID again after bridge/encoder is 
enabled. But it's better to avoid this potential bug.


[   11.065267] YKK --------- analogix_dp_detect:1052
[   11.729596] YKK --------- analogix_dp_get_modes:1016
[   11.737608] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.749229] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.760799] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   13.315962] YKK --------- analogix_dp_detect:1052
[   13.984702] YKK --------- analogix_dp_get_modes:1016
[   13.992977] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.004414] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.015842] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
[   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery 
success
[   14.319130] rockchip-dp ff970000.edp: Link Training success!
[   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
[   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
[   14.443585] rockchip-dp ff970000.edp: unable to config video
[   14.520565] YKK --------- analogix_dp_detect:1052
[   14.525339] YKK --------- analogix_dp_get_modes:1016
[   14.531233] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   15.595724] YKK --------- analogix_dp_detect:1052
[   15.600488] YKK --------- analogix_dp_get_modes:1016
[   15.606451] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   17.327354] YKK --------- analogix_dp_detect:1052
[   17.332105] YKK --------- analogix_dp_get_modes:1016
[   17.338171] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   56.236024] YKK --------- analogix_dp_detect:1052
[   56.240820] YKK --------- analogix_dp_get_modes:1016
[   56.246855] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   64.092477] YKK --------- analogix_dp_detect:1052
[   64.763712] YKK --------- analogix_dp_get_modes:1016



BR,
- Yakir

> +}
> +
>   static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>   {
>   	struct analogix_dp_device *dp = bridge->driver_private;
> @@ -1058,6 +1086,7 @@ static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>   static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>   {
>   	struct analogix_dp_device *dp = bridge->driver_private;
> +	int ret;
>   
>   	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>   		return;
> @@ -1077,6 +1106,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>   
>   	pm_runtime_put_sync(dp->dev);
>   
> +	ret = drm_panel_unprepare(dp->plat_data->panel);
> +	if (ret)
> +		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +
>   	dp->dpms_mode = DRM_MODE_DPMS_OFF;
>   }
>   
> @@ -1165,9 +1198,9 @@ static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
>   }
>   
>   static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
> +	.pre_enable = analogix_dp_bridge_pre_enable,
>   	.enable = analogix_dp_bridge_enable,
>   	.disable = analogix_dp_bridge_disable,
> -	.pre_enable = analogix_dp_bridge_nop,
>   	.post_disable = analogix_dp_bridge_nop,
>   	.mode_set = analogix_dp_bridge_mode_set,
>   	.attach = analogix_dp_bridge_attach,
> @@ -1333,13 +1366,6 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>   
>   	phy_power_on(dp->phy);
>   
> -	if (dp->plat_data->panel) {
> -		if (drm_panel_prepare(dp->plat_data->panel)) {
> -			DRM_ERROR("failed to setup the panel\n");
> -			return -EBUSY;
> -		}
> -	}
> -
>   	analogix_dp_init_dp(dp);
>   
>   	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
Yakir Yang Aug. 1, 2016, 3:28 a.m. UTC | #2
Sean,

On 07/30/2016 03:16 AM, Sean Paul wrote:
> Instead of just preparing the panel on bind, actually prepare/unprepare
> during modeset/disable. The panel must be prepared in order to read hpd
> status, so we need to refcount the prepares in order to ensure we don't
> accidentally turn the panel off at the wrong time.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>
>
> Hi Yakir,
> This is what I was talking about upthread. I've tested it and it seems to be working.
>
> What do you think?

Thanks for your patch, and it works. But I have introduced two 
questions, and I haven't found a way to fixed them.

> Sean
>
>
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 +++++++++++++++++-----
>   1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 32715da..7b764a4 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -960,11 +960,27 @@ enum drm_connector_status
>   analogix_dp_detect(struct drm_connector *connector, bool force)
>   {
>   	struct analogix_dp_device *dp = to_dp(connector);
> +	enum drm_connector_status status = connector_status_disconnected;
> +	int ret;
>   
> -	if (analogix_dp_detect_hpd(dp))
> -		return connector_status_disconnected;
> +	if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
> +		ret = drm_panel_prepare(dp->plat_data->panel);
> +		if (ret) {
> +			DRM_ERROR("failed to setup panel (%d)\n", ret);
> +			return connector_status_disconnected;
> +		}
> +	}
> +
> +	if (!analogix_dp_detect_hpd(dp))
> +		status = connector_status_connected;
> +
> +	if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
> +		ret = drm_panel_unprepare(dp->plat_data->panel);
> +		if (ret)
> +			DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +	}
>   
> -	return connector_status_connected;
> +	return status;

1. Panel would flicker at system boot time. Your patch would flash the 
panel power in connector->detect() function when dp->dpms_mode isn't 
DRM_MODE_DPMS_OFF. So when userspace keep detect the connector status in 
boot time, we could see panel would flicker (light up for a while, and 
turn off again, and keep loop for several time). I have copied some 
kernel logs:


[   11.065267] YKK --------- analogix_dp_detect:1052
[   11.729596] YKK --------- analogix_dp_get_modes:1016
[   11.737608] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.749229] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.760799] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   13.315962] YKK --------- analogix_dp_detect:1052
[   13.984702] YKK --------- analogix_dp_get_modes:1016
[   13.992977] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.004414] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.015842] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
[   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery 
success
[   14.319130] rockchip-dp ff970000.edp: Link Training success!
[   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
[   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
[   14.443585] rockchip-dp ff970000.edp: unable to config video


>   }
>   
>   static void analogix_dp_connector_destroy(struct drm_connector *connector)
> @@ -1035,6 +1051,18 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>   	return 0;
>   }
>   
> +static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct analogix_dp_device *dp = bridge->driver_private;
> +	int ret;
> +
> +	if (dp->plat_data->panel) {
> +		ret = drm_panel_prepare(dp->plat_data->panel);
> +		if (ret)
> +			DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +	}

2. Driver would failed to read EDID in some case. Panel would only be 
powered up in bridge->pre_enable() function which later than 
connector->get_modes() function, and this would caused DPCD transfer 
failed in analogix_dp_handle_edid(). This seem won't caused too big 
issue, cause userspace would read EDID again after bridge/encoder is 
enabled. But it's better to avoid this potential bug.


[   11.065267] YKK --------- analogix_dp_detect:1052
[   11.729596] YKK --------- analogix_dp_get_modes:1016
[   11.737608] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.749229] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   11.760799] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   13.315962] YKK --------- analogix_dp_detect:1052
[   13.984702] YKK --------- analogix_dp_get_modes:1016
[   13.992977] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.004414] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.015842] rockchip-dp ff970000.edp: 
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
[   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery 
success
[   14.319130] rockchip-dp ff970000.edp: Link Training success!
[   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
[   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
[   14.443585] rockchip-dp ff970000.edp: unable to config video
[   14.520565] YKK --------- analogix_dp_detect:1052
[   14.525339] YKK --------- analogix_dp_get_modes:1016
[   14.531233] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   15.595724] YKK --------- analogix_dp_detect:1052
[   15.600488] YKK --------- analogix_dp_get_modes:1016
[   15.606451] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   17.327354] YKK --------- analogix_dp_detect:1052
[   17.332105] YKK --------- analogix_dp_get_modes:1016
[   17.338171] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   56.236024] YKK --------- analogix_dp_detect:1052
[   56.240820] YKK --------- analogix_dp_get_modes:1016
[   56.246855] rockchip-dp ff970000.edp: EDID data does not include any 
extensions.
[   64.092477] YKK --------- analogix_dp_detect:1052
[   64.763712] YKK --------- analogix_dp_get_modes:1016



BR,
- Yakir

> +}
> +
>   static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>   {
>   	struct analogix_dp_device *dp = bridge->driver_private;
> @@ -1058,6 +1086,7 @@ static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>   static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>   {
>   	struct analogix_dp_device *dp = bridge->driver_private;
> +	int ret;
>   
>   	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>   		return;
> @@ -1077,6 +1106,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>   
>   	pm_runtime_put_sync(dp->dev);
>   
> +	ret = drm_panel_unprepare(dp->plat_data->panel);
> +	if (ret)
> +		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +
>   	dp->dpms_mode = DRM_MODE_DPMS_OFF;
>   }
>   
> @@ -1165,9 +1198,9 @@ static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
>   }
>   
>   static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
> +	.pre_enable = analogix_dp_bridge_pre_enable,
>   	.enable = analogix_dp_bridge_enable,
>   	.disable = analogix_dp_bridge_disable,
> -	.pre_enable = analogix_dp_bridge_nop,
>   	.post_disable = analogix_dp_bridge_nop,
>   	.mode_set = analogix_dp_bridge_mode_set,
>   	.attach = analogix_dp_bridge_attach,
> @@ -1333,13 +1366,6 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>   
>   	phy_power_on(dp->phy);
>   
> -	if (dp->plat_data->panel) {
> -		if (drm_panel_prepare(dp->plat_data->panel)) {
> -			DRM_ERROR("failed to setup the panel\n");
> -			return -EBUSY;
> -		}
> -	}
> -
>   	analogix_dp_init_dp(dp);
>   
>   	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
Sean Paul Aug. 1, 2016, 8:20 p.m. UTC | #3
On Sun, Jul 31, 2016 at 11:27 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> Sean,
>
> On 07/30/2016 03:16 AM, Sean Paul wrote:
>>
>> Instead of just preparing the panel on bind, actually prepare/unprepare
>> during modeset/disable. The panel must be prepared in order to read hpd
>> status, so we need to refcount the prepares in order to ensure we don't
>> accidentally turn the panel off at the wrong time.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>>
>>
>> Hi Yakir,
>> This is what I was talking about upthread. I've tested it and it seems to
>> be working.
>>
>> What do you think?
>
>
> Thanks for your patch, and it works. But I have introduced two questions,
> and I haven't found a way to fixed them.
>
>
>> Sean
>>
>>
>>
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48
>> +++++++++++++++++-----
>>   1 file changed, 37 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index 32715da..7b764a4 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -960,11 +960,27 @@ enum drm_connector_status
>>   analogix_dp_detect(struct drm_connector *connector, bool force)
>>   {
>>         struct analogix_dp_device *dp = to_dp(connector);
>> +       enum drm_connector_status status = connector_status_disconnected;
>> +       int ret;
>>   -     if (analogix_dp_detect_hpd(dp))
>> -               return connector_status_disconnected;
>> +       if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
>> +               ret = drm_panel_prepare(dp->plat_data->panel);
>> +               if (ret) {
>> +                       DRM_ERROR("failed to setup panel (%d)\n", ret);
>> +                       return connector_status_disconnected;
>> +               }
>> +       }
>> +
>> +       if (!analogix_dp_detect_hpd(dp))
>> +               status = connector_status_connected;
>> +
>> +       if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
>> +               ret = drm_panel_unprepare(dp->plat_data->panel);
>> +               if (ret)
>> +                       DRM_ERROR("failed to setup the panel ret = %d\n",
>> ret);
>> +       }
>>   -     return connector_status_connected;
>> +       return status;
>
>
> 1. Panel would flicker at system boot time. Your patch would flash the panel
> power in connector->detect() function when dp->dpms_mode isn't
> DRM_MODE_DPMS_OFF. So when userspace keep detect the connector status in
> boot time, we could see panel would flicker (light up for a while, and turn
> off again, and keep loop for several time). I have copied some kernel logs:
>


Hmm, yeah, the backlight does seem to flash. It seems like backlight
shouldn't be on if the panel isn't enabled. this would also fix the
issue below since you could add panel prepare/unprepare in get_modes.

Alternatively, have you played around with moving things around in
prepare/unprepare? Perhaps just enabling the supply regulator (without
the enable gpio set) is sufficient to read HPD/EDID, but will still
prevent the original backlight flicker/burn-in issue?

Sean



>
> [   11.065267] YKK --------- analogix_dp_detect:1052
> [   11.729596] YKK --------- analogix_dp_get_modes:1016
> [   11.737608] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   11.749229] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   11.760799] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   13.315962] YKK --------- analogix_dp_detect:1052
> [   13.984702] YKK --------- analogix_dp_get_modes:1016
> [   13.992977] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   14.004414] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   14.015842] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
> [   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
> success
> [   14.319130] rockchip-dp ff970000.edp: Link Training success!
> [   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
> [   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
> [   14.443585] rockchip-dp ff970000.edp: unable to config video
>
>
>>   }
>>     static void analogix_dp_connector_destroy(struct drm_connector
>> *connector)
>> @@ -1035,6 +1051,18 @@ static int analogix_dp_bridge_attach(struct
>> drm_bridge *bridge)
>>         return 0;
>>   }
>>   +static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
>> +{
>> +       struct analogix_dp_device *dp = bridge->driver_private;
>> +       int ret;
>> +
>> +       if (dp->plat_data->panel) {
>> +               ret = drm_panel_prepare(dp->plat_data->panel);
>> +               if (ret)
>> +                       DRM_ERROR("failed to setup the panel ret = %d\n",
>> ret);
>> +       }
>
>
> 2. Driver would failed to read EDID in some case. Panel would only be
> powered up in bridge->pre_enable() function which later than
> connector->get_modes() function, and this would caused DPCD transfer failed
> in analogix_dp_handle_edid(). This seem won't caused too big issue, cause
> userspace would read EDID again after bridge/encoder is enabled. But it's
> better to avoid this potential bug.
>
>
> [   11.065267] YKK --------- analogix_dp_detect:1052
> [   11.729596] YKK --------- analogix_dp_get_modes:1016
> [   11.737608] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   11.749229] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   11.760799] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   13.315962] YKK --------- analogix_dp_detect:1052
> [   13.984702] YKK --------- analogix_dp_get_modes:1016
> [   13.992977] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   14.004414] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   14.015842] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
> Aux Transaction fail!
> [   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
> [   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
> success
> [   14.319130] rockchip-dp ff970000.edp: Link Training success!
> [   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
> [   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
> [   14.443585] rockchip-dp ff970000.edp: unable to config video
> [   14.520565] YKK --------- analogix_dp_detect:1052
> [   14.525339] YKK --------- analogix_dp_get_modes:1016
> [   14.531233] rockchip-dp ff970000.edp: EDID data does not include any
> extensions.
> [   15.595724] YKK --------- analogix_dp_detect:1052
> [   15.600488] YKK --------- analogix_dp_get_modes:1016
> [   15.606451] rockchip-dp ff970000.edp: EDID data does not include any
> extensions.
> [   17.327354] YKK --------- analogix_dp_detect:1052
> [   17.332105] YKK --------- analogix_dp_get_modes:1016
> [   17.338171] rockchip-dp ff970000.edp: EDID data does not include any
> extensions.
> [   56.236024] YKK --------- analogix_dp_detect:1052
> [   56.240820] YKK --------- analogix_dp_get_modes:1016
> [   56.246855] rockchip-dp ff970000.edp: EDID data does not include any
> extensions.
> [   64.092477] YKK --------- analogix_dp_detect:1052
> [   64.763712] YKK --------- analogix_dp_get_modes:1016
>
>
>
> BR,
> - Yakir
>
>
>> +}
>> +
>>   static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>>   {
>>         struct analogix_dp_device *dp = bridge->driver_private;
>> @@ -1058,6 +1086,7 @@ static void analogix_dp_bridge_enable(struct
>> drm_bridge *bridge)
>>   static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>   {
>>         struct analogix_dp_device *dp = bridge->driver_private;
>> +       int ret;
>>         if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>>                 return;
>> @@ -1077,6 +1106,10 @@ static void analogix_dp_bridge_disable(struct
>> drm_bridge *bridge)
>>         pm_runtime_put_sync(dp->dev);
>>   +     ret = drm_panel_unprepare(dp->plat_data->panel);
>> +       if (ret)
>> +               DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>> +
>>         dp->dpms_mode = DRM_MODE_DPMS_OFF;
>>   }
>>   @@ -1165,9 +1198,9 @@ static void analogix_dp_bridge_nop(struct
>> drm_bridge *bridge)
>>   }
>>     static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
>> +       .pre_enable = analogix_dp_bridge_pre_enable,
>>         .enable = analogix_dp_bridge_enable,
>>         .disable = analogix_dp_bridge_disable,
>> -       .pre_enable = analogix_dp_bridge_nop,
>>         .post_disable = analogix_dp_bridge_nop,
>>         .mode_set = analogix_dp_bridge_mode_set,
>>         .attach = analogix_dp_bridge_attach,
>> @@ -1333,13 +1366,6 @@ int analogix_dp_bind(struct device *dev, struct
>> drm_device *drm_dev,
>>         phy_power_on(dp->phy);
>>   -     if (dp->plat_data->panel) {
>> -               if (drm_panel_prepare(dp->plat_data->panel)) {
>> -                       DRM_ERROR("failed to setup the panel\n");
>> -                       return -EBUSY;
>> -               }
>> -       }
>> -
>>         analogix_dp_init_dp(dp);
>>         ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sean Paul Aug. 1, 2016, 8:46 p.m. UTC | #4
On Mon, Aug 1, 2016 at 4:20 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Sun, Jul 31, 2016 at 11:27 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Sean,
>>
>> On 07/30/2016 03:16 AM, Sean Paul wrote:
>>>
>>> Instead of just preparing the panel on bind, actually prepare/unprepare
>>> during modeset/disable. The panel must be prepared in order to read hpd
>>> status, so we need to refcount the prepares in order to ensure we don't
>>> accidentally turn the panel off at the wrong time.
>>>
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> ---
>>>
>>>
>>> Hi Yakir,
>>> This is what I was talking about upthread. I've tested it and it seems to
>>> be working.
>>>
>>> What do you think?
>>
>>
>> Thanks for your patch, and it works. But I have introduced two questions,
>> and I haven't found a way to fixed them.
>>
>>
>>> Sean
>>>
>>>
>>>
>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48
>>> +++++++++++++++++-----
>>>   1 file changed, 37 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 32715da..7b764a4 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -960,11 +960,27 @@ enum drm_connector_status
>>>   analogix_dp_detect(struct drm_connector *connector, bool force)
>>>   {
>>>         struct analogix_dp_device *dp = to_dp(connector);
>>> +       enum drm_connector_status status = connector_status_disconnected;
>>> +       int ret;
>>>   -     if (analogix_dp_detect_hpd(dp))
>>> -               return connector_status_disconnected;
>>> +       if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
>>> +               ret = drm_panel_prepare(dp->plat_data->panel);
>>> +               if (ret) {
>>> +                       DRM_ERROR("failed to setup panel (%d)\n", ret);
>>> +                       return connector_status_disconnected;
>>> +               }
>>> +       }
>>> +
>>> +       if (!analogix_dp_detect_hpd(dp))
>>> +               status = connector_status_connected;
>>> +
>>> +       if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
>>> +               ret = drm_panel_unprepare(dp->plat_data->panel);
>>> +               if (ret)
>>> +                       DRM_ERROR("failed to setup the panel ret = %d\n",
>>> ret);
>>> +       }
>>>   -     return connector_status_connected;
>>> +       return status;
>>
>>
>> 1. Panel would flicker at system boot time. Your patch would flash the panel
>> power in connector->detect() function when dp->dpms_mode isn't
>> DRM_MODE_DPMS_OFF. So when userspace keep detect the connector status in
>> boot time, we could see panel would flicker (light up for a while, and turn
>> off again, and keep loop for several time). I have copied some kernel logs:
>>
>
>
> Hmm, yeah, the backlight does seem to flash. It seems like backlight
> shouldn't be on if the panel isn't enabled. this would also fix the
> issue below since you could add panel prepare/unprepare in get_modes.
>
> Alternatively, have you played around with moving things around in
> prepare/unprepare? Perhaps just enabling the supply regulator (without
> the enable gpio set) is sufficient to read HPD/EDID, but will still
> prevent the original backlight flicker/burn-in issue?
>

To answer my own question, the enable_gpio isn't populated in the dts,
so reordering things in prepare isn't feasible.

Sean


> Sean
>
>
>
>>
>> [   11.065267] YKK --------- analogix_dp_detect:1052
>> [   11.729596] YKK --------- analogix_dp_get_modes:1016
>> [   11.737608] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.749229] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.760799] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   13.315962] YKK --------- analogix_dp_detect:1052
>> [   13.984702] YKK --------- analogix_dp_get_modes:1016
>> [   13.992977] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.004414] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.015842] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
>> [   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
>> success
>> [   14.319130] rockchip-dp ff970000.edp: Link Training success!
>> [   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
>> [   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
>> [   14.443585] rockchip-dp ff970000.edp: unable to config video
>>
>>
>>>   }
>>>     static void analogix_dp_connector_destroy(struct drm_connector
>>> *connector)
>>> @@ -1035,6 +1051,18 @@ static int analogix_dp_bridge_attach(struct
>>> drm_bridge *bridge)
>>>         return 0;
>>>   }
>>>   +static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
>>> +{
>>> +       struct analogix_dp_device *dp = bridge->driver_private;
>>> +       int ret;
>>> +
>>> +       if (dp->plat_data->panel) {
>>> +               ret = drm_panel_prepare(dp->plat_data->panel);
>>> +               if (ret)
>>> +                       DRM_ERROR("failed to setup the panel ret = %d\n",
>>> ret);
>>> +       }
>>
>>
>> 2. Driver would failed to read EDID in some case. Panel would only be
>> powered up in bridge->pre_enable() function which later than
>> connector->get_modes() function, and this would caused DPCD transfer failed
>> in analogix_dp_handle_edid(). This seem won't caused too big issue, cause
>> userspace would read EDID again after bridge/encoder is enabled. But it's
>> better to avoid this potential bug.
>>
>>
>> [   11.065267] YKK --------- analogix_dp_detect:1052
>> [   11.729596] YKK --------- analogix_dp_get_modes:1016
>> [   11.737608] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.749229] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.760799] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   13.315962] YKK --------- analogix_dp_detect:1052
>> [   13.984702] YKK --------- analogix_dp_get_modes:1016
>> [   13.992977] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.004414] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.015842] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
>> [   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
>> success
>> [   14.319130] rockchip-dp ff970000.edp: Link Training success!
>> [   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
>> [   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
>> [   14.443585] rockchip-dp ff970000.edp: unable to config video
>> [   14.520565] YKK --------- analogix_dp_detect:1052
>> [   14.525339] YKK --------- analogix_dp_get_modes:1016
>> [   14.531233] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   15.595724] YKK --------- analogix_dp_detect:1052
>> [   15.600488] YKK --------- analogix_dp_get_modes:1016
>> [   15.606451] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   17.327354] YKK --------- analogix_dp_detect:1052
>> [   17.332105] YKK --------- analogix_dp_get_modes:1016
>> [   17.338171] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   56.236024] YKK --------- analogix_dp_detect:1052
>> [   56.240820] YKK --------- analogix_dp_get_modes:1016
>> [   56.246855] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   64.092477] YKK --------- analogix_dp_detect:1052
>> [   64.763712] YKK --------- analogix_dp_get_modes:1016
>>
>>
>>
>> BR,
>> - Yakir
>>
>>
>>> +}
>>> +
>>>   static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>>>   {
>>>         struct analogix_dp_device *dp = bridge->driver_private;
>>> @@ -1058,6 +1086,7 @@ static void analogix_dp_bridge_enable(struct
>>> drm_bridge *bridge)
>>>   static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>>   {
>>>         struct analogix_dp_device *dp = bridge->driver_private;
>>> +       int ret;
>>>         if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>>>                 return;
>>> @@ -1077,6 +1106,10 @@ static void analogix_dp_bridge_disable(struct
>>> drm_bridge *bridge)
>>>         pm_runtime_put_sync(dp->dev);
>>>   +     ret = drm_panel_unprepare(dp->plat_data->panel);
>>> +       if (ret)
>>> +               DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>>> +
>>>         dp->dpms_mode = DRM_MODE_DPMS_OFF;
>>>   }
>>>   @@ -1165,9 +1198,9 @@ static void analogix_dp_bridge_nop(struct
>>> drm_bridge *bridge)
>>>   }
>>>     static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
>>> +       .pre_enable = analogix_dp_bridge_pre_enable,
>>>         .enable = analogix_dp_bridge_enable,
>>>         .disable = analogix_dp_bridge_disable,
>>> -       .pre_enable = analogix_dp_bridge_nop,
>>>         .post_disable = analogix_dp_bridge_nop,
>>>         .mode_set = analogix_dp_bridge_mode_set,
>>>         .attach = analogix_dp_bridge_attach,
>>> @@ -1333,13 +1366,6 @@ int analogix_dp_bind(struct device *dev, struct
>>> drm_device *drm_dev,
>>>         phy_power_on(dp->phy);
>>>   -     if (dp->plat_data->panel) {
>>> -               if (drm_panel_prepare(dp->plat_data->panel)) {
>>> -                       DRM_ERROR("failed to setup the panel\n");
>>> -                       return -EBUSY;
>>> -               }
>>> -       }
>>> -
>>>         analogix_dp_init_dp(dp);
>>>         ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 32715da..7b764a4 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -960,11 +960,27 @@  enum drm_connector_status
 analogix_dp_detect(struct drm_connector *connector, bool force)
 {
 	struct analogix_dp_device *dp = to_dp(connector);
+	enum drm_connector_status status = connector_status_disconnected;
+	int ret;
 
-	if (analogix_dp_detect_hpd(dp))
-		return connector_status_disconnected;
+	if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
+		ret = drm_panel_prepare(dp->plat_data->panel);
+		if (ret) {
+			DRM_ERROR("failed to setup panel (%d)\n", ret);
+			return connector_status_disconnected;
+		}
+	}
+
+	if (!analogix_dp_detect_hpd(dp))
+		status = connector_status_connected;
+
+	if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
+		ret = drm_panel_unprepare(dp->plat_data->panel);
+		if (ret)
+			DRM_ERROR("failed to setup the panel ret = %d\n", ret);
+	}
 
-	return connector_status_connected;
+	return status;
 }
 
 static void analogix_dp_connector_destroy(struct drm_connector *connector)
@@ -1035,6 +1051,18 @@  static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
 	return 0;
 }
 
+static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct analogix_dp_device *dp = bridge->driver_private;
+	int ret;
+
+	if (dp->plat_data->panel) {
+		ret = drm_panel_prepare(dp->plat_data->panel);
+		if (ret)
+			DRM_ERROR("failed to setup the panel ret = %d\n", ret);
+	}
+}
+
 static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
 {
 	struct analogix_dp_device *dp = bridge->driver_private;
@@ -1058,6 +1086,7 @@  static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
 static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 {
 	struct analogix_dp_device *dp = bridge->driver_private;
+	int ret;
 
 	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
 		return;
@@ -1077,6 +1106,10 @@  static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 
 	pm_runtime_put_sync(dp->dev);
 
+	ret = drm_panel_unprepare(dp->plat_data->panel);
+	if (ret)
+		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
+
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
@@ -1165,9 +1198,9 @@  static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
 }
 
 static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
+	.pre_enable = analogix_dp_bridge_pre_enable,
 	.enable = analogix_dp_bridge_enable,
 	.disable = analogix_dp_bridge_disable,
-	.pre_enable = analogix_dp_bridge_nop,
 	.post_disable = analogix_dp_bridge_nop,
 	.mode_set = analogix_dp_bridge_mode_set,
 	.attach = analogix_dp_bridge_attach,
@@ -1333,13 +1366,6 @@  int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 
 	phy_power_on(dp->phy);
 
-	if (dp->plat_data->panel) {
-		if (drm_panel_prepare(dp->plat_data->panel)) {
-			DRM_ERROR("failed to setup the panel\n");
-			return -EBUSY;
-		}
-	}
-
 	analogix_dp_init_dp(dp);
 
 	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,