* [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31
@ 2016-07-21 13:14 Yakir Yang
2016-07-21 13:14 ` [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Yakir Yang @ 2016-07-21 13:14 UTC (permalink / raw)
To: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner
Cc: Jingoo Han, Javier Martinez Canillas, Stéphane Marchesin,
Sean Paul, Tomasz Figa, dianders, David Airlie, daniel.vetter,
Krzysztof Kozlowski, emil.l.velikov, Yakir Yang, linux-kernel,
dri-devel, linux-samsung-soc, linux-rockchip
According to page 16 of Sharp LQ123P1JX31 datasheet, we need to add the
missing delay timing. Panel prepare time should be t1 (0.5ms~10ms) plus
t3 (0ms~100ms), and panel enable time should equal to t7 (0ms~50ms), and
panel unprepare time should be t11 (1ms~50ms) plus t12 (500ms~).
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
drivers/gpu/drm/panel/panel-simple.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 85143d1..f178998 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1384,6 +1384,11 @@ static const struct panel_desc sharp_lq123p1jx31 = {
.width = 259,
.height = 173,
},
+ .delay = {
+ .prepare = 110,
+ .enable = 50,
+ .unprepare = 550,
+ },
};
static const struct drm_display_mode shelly_sca07010_bfn_lnn_mode = {
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
2016-07-21 13:14 [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Yakir Yang
@ 2016-07-21 13:14 ` Yakir Yang
2016-07-21 14:28 ` Sean Paul
2016-07-21 14:14 ` [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
2016-07-21 14:30 ` [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Sean Paul
2 siblings, 1 reply; 26+ messages in thread
From: Yakir Yang @ 2016-07-21 13:14 UTC (permalink / raw)
To: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner
Cc: Jingoo Han, Javier Martinez Canillas, Stéphane Marchesin,
Sean Paul, Tomasz Figa, dianders, David Airlie, daniel.vetter,
Krzysztof Kozlowski, emil.l.velikov, Yakir Yang, linux-kernel,
dri-devel, linux-samsung-soc, linux-rockchip
Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
controller stop to send valid video signal, otherwhise panel would
go burn in, and keep flicker and flicker.
So it's better to turn off the panel when eDP need to disable, and
we need to turn on the panel in connector->detect() callback, so
that driver would detect panel status rightly.
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 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..ea059b3 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector, bool force)
{
struct analogix_dp_device *dp = to_dp(connector);
+ /*
+ * Panle would prepare for several times here, but don't worry it
+ * would only enable the hardware at the first prepare time.
+ */
+ if (dp->plat_data->panel)
+ if (drm_panel_prepare(dp->plat_data->panel))
+ DRM_ERROR("failed to setup the panel\n");
+
if (analogix_dp_detect_hpd(dp))
return connector_status_disconnected;
@@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
return;
if (dp->plat_data->panel) {
- if (drm_panel_disable(dp->plat_data->panel)) {
+ if (drm_panel_disable(dp->plat_data->panel) ||
+ drm_panel_unprepare(dp->plat_data->panel)) {
DRM_ERROR("failed to disable the panel\n");
return;
}
@@ -1333,13 +1342,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,
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
2016-07-21 13:14 [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Yakir Yang
2016-07-21 13:14 ` [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
@ 2016-07-21 14:14 ` Yakir Yang
2016-07-21 14:30 ` [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Sean Paul
2 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-07-21 14:14 UTC (permalink / raw)
To: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner
Cc: Jingoo Han, Javier Martinez Canillas, Stéphane Marchesin,
Sean Paul, Tomasz Figa, dianders, David Airlie, daniel.vetter,
Krzysztof Kozlowski, emil.l.velikov, Yakir Yang, linux-kernel,
dri-devel, linux-samsung-soc, linux-rockchip
Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
controller stop to send valid video signal, otherwhise panel would
go burn in, and keep flicker and flicker.
So it's better to turn off the panel when eDP need to disable, and
we need to turn on the panel in connector->detect() callback, so
that driver would detect panel status rightly.
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v1.1:
- unprepare the panel at the end of bridge->disable() function
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 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..2e519bd 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector, bool force)
{
struct analogix_dp_device *dp = to_dp(connector);
+ /*
+ * Panle would prepare for several times here, but don't worry it
+ * would only enable the hardware at the first prepare time.
+ */
+ if (dp->plat_data->panel)
+ if (drm_panel_prepare(dp->plat_data->panel))
+ DRM_ERROR("failed to setup the panel\n");
+
if (analogix_dp_detect_hpd(dp))
return connector_status_disconnected;
@@ -1077,6 +1085,13 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
pm_runtime_put_sync(dp->dev);
+ if (dp->plat_data->panel) {
+ if (drm_panel_unprepare(dp->plat_data->panel)) {
+ DRM_ERROR("failed to turnoff the panel\n");
+ return;
+ }
+ }
+
dp->dpms_mode = DRM_MODE_DPMS_OFF;
}
@@ -1333,13 +1348,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,
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
2016-07-21 13:14 ` [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
@ 2016-07-21 14:28 ` Sean Paul
2016-07-22 1:00 ` Yakir Yang
0 siblings, 1 reply; 26+ messages in thread
From: Sean Paul @ 2016-07-21 14:28 UTC (permalink / raw)
To: Yakir Yang
Cc: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Emil Velikov, Linux Kernel Mailing List,
dri-devel, linux-samsung-soc, linux-rockchip
On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
> controller stop to send valid video signal, otherwhise panel would
> go burn in, and keep flicker and flicker.
>
> So it's better to turn off the panel when eDP need to disable, and
> we need to turn on the panel in connector->detect() callback, so
> that driver would detect panel status rightly.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 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..ea059b3 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector, bool force)
> {
> struct analogix_dp_device *dp = to_dp(connector);
>
> + /*
> + * Panle would prepare for several times here, but don't worry it
s/Panle/Panel/
> + * would only enable the hardware at the first prepare time.
Errr, this shouldn't go in detect. How about putting this in
bridge_enable instead?
> + */
> + if (dp->plat_data->panel)
> + if (drm_panel_prepare(dp->plat_data->panel))
Personally, I don't like doing work in a conditional since you're
throwing the return code away. Could you break this out into:
ret = drm_panel_prepare(dp->plat_data->panel);
if (ret)
DRM_ERROR("failed to setup the panel ret=%d\n", ret);
> + DRM_ERROR("failed to setup the panel\n");
> +
> if (analogix_dp_detect_hpd(dp))
> return connector_status_disconnected;
>
> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> return;
>
> if (dp->plat_data->panel) {
> - if (drm_panel_disable(dp->plat_data->panel)) {
> + if (drm_panel_disable(dp->plat_data->panel) ||
> + drm_panel_unprepare(dp->plat_data->panel)) {
Same comment here, please break this out into separate statements for
better readability/logging.
> DRM_ERROR("failed to disable the panel\n");
> return;
> }
> @@ -1333,13 +1342,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,
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31
2016-07-21 13:14 [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Yakir Yang
2016-07-21 13:14 ` [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
2016-07-21 14:14 ` [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
@ 2016-07-21 14:30 ` Sean Paul
2016-07-22 1:00 ` Yakir Yang
2 siblings, 1 reply; 26+ messages in thread
From: Sean Paul @ 2016-07-21 14:30 UTC (permalink / raw)
To: Yakir Yang
Cc: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Emil Velikov, Linux Kernel Mailing List,
dri-devel, linux-samsung-soc, linux-rockchip
On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> According to page 16 of Sharp LQ123P1JX31 datasheet, we need to add the
> missing delay timing. Panel prepare time should be t1 (0.5ms~10ms) plus
> t3 (0ms~100ms), and panel enable time should equal to t7 (0ms~50ms), and
> panel unprepare time should be t11 (1ms~50ms) plus t12 (500ms~).
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> drivers/gpu/drm/panel/panel-simple.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 85143d1..f178998 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1384,6 +1384,11 @@ static const struct panel_desc sharp_lq123p1jx31 = {
> .width = 259,
> .height = 173,
> },
> + .delay = {
> + .prepare = 110,
> + .enable = 50,
> + .unprepare = 550,
> + },
> };
>
> static const struct drm_display_mode shelly_sca07010_bfn_lnn_mode = {
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
2016-07-21 14:28 ` Sean Paul
@ 2016-07-22 1:00 ` Yakir Yang
2016-07-22 15:03 ` Sean Paul
0 siblings, 1 reply; 26+ messages in thread
From: Yakir Yang @ 2016-07-22 1:00 UTC (permalink / raw)
To: Sean Paul
Cc: Krzysztof Kozlowski, linux-samsung-soc, linux-rockchip, Mark Yao,
Jingoo Han, Emil Velikov, Douglas Anderson, dri-devel,
Tomasz Figa, Javier Martinez Canillas, Daniel Vetter,
Stéphane Marchesin, Thierry Reding,
Linux Kernel Mailing List
[-- Attachment #1.1: Type: text/plain, Size: 3878 bytes --]
Sean,
Thanks for your fast respond :-)
But this patch is not the latest one, I have upgraded this to "v1.1"
version to fix the eDP can't be disabled problem:
[PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when
eDP need to disable
Changes in v1.1: - unprepare the panel at the end of
bridge->disable() function
In spite of this, I would take your comments with my "v1.1" patch.
On 07/21/2016 10:28 PM, Sean Paul wrote:
> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
>> controller stop to send valid video signal, otherwhise panel would
>> go burn in, and keep flicker and flicker.
>>
>> So it's better to turn off the panel when eDP need to disable, and
>> we need to turn on the panel in connector->detect() callback, so
>> that driver would detect panel status rightly.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
>> 1 file changed, 10 insertions(+), 8 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..ea059b3 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector, bool force)
>> {
>> struct analogix_dp_device *dp = to_dp(connector);
>>
>> + /*
>> + * Panle would prepare for several times here, but don't worry it
> s/Panle/Panel/
Done
>> + * would only enable the hardware at the first prepare time.
>
> Errr, this shouldn't go in detect. How about putting this in
> bridge_enable instead?
Nope, if we put this in bridge_enable, then eDP would never be enabled.
Here're the calling flow.
--> analogix_dp_probe
--> analogix_dp_bind # we still haven't prepared the panel here,
that means panel have been powered up
--> analogix_dp_detect # Oops, losing panel valid hotplug signal,
cause panel have been powered up
--> ** Keep detecting **
>> + */
>> + if (dp->plat_data->panel)
>> + if (drm_panel_prepare(dp->plat_data->panel))
> Personally, I don't like doing work in a conditional since you're
> throwing the return code away. Could you break this out into:
>
> ret = drm_panel_prepare(dp->plat_data->panel);
> if (ret)
> DRM_ERROR("failed to setup the panel ret=%d\n", ret);
Okay
>
>> + DRM_ERROR("failed to setup the panel\n");
>> +
>> if (analogix_dp_detect_hpd(dp))
>> return connector_status_disconnected;
>>
>> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>> return;
>>
>> if (dp->plat_data->panel) {
>> - if (drm_panel_disable(dp->plat_data->panel)) {
>> + if (drm_panel_disable(dp->plat_data->panel) ||
>> + drm_panel_unprepare(dp->plat_data->panel)) {
> Same comment here, please break this out into separate statements for
> better readability/logging.
Okay,
Thanks,
- Yakir
>
>> DRM_ERROR("failed to disable the panel\n");
>> return;
>> }
>> @@ -1333,13 +1342,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,
>> --
>> 1.9.1
>>
>>
>
>
[-- Attachment #1.2: Type: text/html, Size: 5978 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31
2016-07-21 14:30 ` [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Sean Paul
@ 2016-07-22 1:00 ` Yakir Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-07-22 1:00 UTC (permalink / raw)
To: Sean Paul
Cc: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Emil Velikov, Linux Kernel Mailing List,
dri-devel, linux-samsung-soc, linux-rockchip
Sean,
On 07/21/2016 10:30 PM, Sean Paul wrote:
> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> According to page 16 of Sharp LQ123P1JX31 datasheet, we need to add the
>> missing delay timing. Panel prepare time should be t1 (0.5ms~10ms) plus
>> t3 (0ms~100ms), and panel enable time should equal to t7 (0ms~50ms), and
>> panel unprepare time should be t11 (1ms~50ms) plus t12 (500ms~).
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
Thanks :-D
- Yakir
>> ---
>> drivers/gpu/drm/panel/panel-simple.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> index 85143d1..f178998 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -1384,6 +1384,11 @@ static const struct panel_desc sharp_lq123p1jx31 = {
>> .width = 259,
>> .height = 173,
>> },
>> + .delay = {
>> + .prepare = 110,
>> + .enable = 50,
>> + .unprepare = 550,
>> + },
>> };
>>
>> static const struct drm_display_mode shelly_sca07010_bfn_lnn_mode = {
>> --
>> 1.9.1
>>
>>
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
2016-07-22 1:00 ` Yakir Yang
@ 2016-07-22 15:03 ` Sean Paul
2016-07-28 3:28 ` Yakir Yang
0 siblings, 1 reply; 26+ messages in thread
From: Sean Paul @ 2016-07-22 15:03 UTC (permalink / raw)
To: Yakir Yang
Cc: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Emil Velikov, Linux Kernel Mailing List,
dri-devel, linux-samsung-soc, linux-rockchip
On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> Sean,
>
> Thanks for your fast respond :-)
>
> But this patch is not the latest one, I have upgraded this to "v1.1" version
> to fix the eDP can't be disabled problem:
> [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP
> need to disable
> Changes in v1.1: - unprepare the panel at the end of
> bridge->disable() function
>
> In spite of this, I would take your comments with my "v1.1" patch.
>
> On 07/21/2016 10:28 PM, Sean Paul wrote:
>
> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>
> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
> controller stop to send valid video signal, otherwhise panel would
> go burn in, and keep flicker and flicker.
>
> So it's better to turn off the panel when eDP need to disable, and
> we need to turn on the panel in connector->detect() callback, so
> that driver would detect panel status rightly.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 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..ea059b3 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
> bool force)
> {
> struct analogix_dp_device *dp = to_dp(connector);
>
> + /*
> + * Panle would prepare for several times here, but don't worry it
>
> s/Panle/Panel/
>
>
> Done
>
> + * would only enable the hardware at the first prepare time.
>
> Errr, this shouldn't go in detect. How about putting this in
> bridge_enable instead?
>
>
> Nope, if we put this in bridge_enable, then eDP would never be enabled.
> Here're the calling flow.
>
> --> analogix_dp_probe
> --> analogix_dp_bind # we still haven't prepared the panel here, that
> means panel have been powered up
> --> analogix_dp_detect # Oops, losing panel valid hotplug signal, cause
> panel have been powered up
> --> ** Keep detecting **
Yeah, after playing around with the patch yesterday I discovered this
for myself. I still don't think detect() should be changing hardware
state. Perhaps you could add a matching unprepare() after you attempt
to detect the panel?
Sean
>
> + */
> + if (dp->plat_data->panel)
> + if (drm_panel_prepare(dp->plat_data->panel))
>
> Personally, I don't like doing work in a conditional since you're
> throwing the return code away. Could you break this out into:
>
> ret = drm_panel_prepare(dp->plat_data->panel);
> if (ret)
> DRM_ERROR("failed to setup the panel ret=%d\n", ret);
>
>
> Okay
>
>
> + DRM_ERROR("failed to setup the panel\n");
> +
> if (analogix_dp_detect_hpd(dp))
> return connector_status_disconnected;
>
> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
> drm_bridge *bridge)
> return;
>
> if (dp->plat_data->panel) {
> - if (drm_panel_disable(dp->plat_data->panel)) {
> + if (drm_panel_disable(dp->plat_data->panel) ||
> + drm_panel_unprepare(dp->plat_data->panel)) {
>
> Same comment here, please break this out into separate statements for
> better readability/logging.
>
>
> Okay,
>
>
> Thanks,
> - Yakir
>
>
> DRM_ERROR("failed to disable the panel\n");
> return;
> }
> @@ -1333,13 +1342,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,
> --
> 1.9.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
2016-07-22 15:03 ` Sean Paul
@ 2016-07-28 3:28 ` Yakir Yang
2016-07-28 14:06 ` Sean Paul
0 siblings, 1 reply; 26+ messages in thread
From: Yakir Yang @ 2016-07-28 3:28 UTC (permalink / raw)
To: Sean Paul
Cc: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Emil Velikov, Linux Kernel Mailing List,
dri-devel, linux-samsung-soc, linux-rockchip
Sean,
Oops, sorry about miss your suggest :(
On 07/22/2016 11:03 PM, Sean Paul wrote:
> On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Sean,
>>
>> Thanks for your fast respond :-)
>>
>> But this patch is not the latest one, I have upgraded this to "v1.1" version
>> to fix the eDP can't be disabled problem:
>> [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP
>> need to disable
>> Changes in v1.1: - unprepare the panel at the end of
>> bridge->disable() function
>>
>> In spite of this, I would take your comments with my "v1.1" patch.
>>
>> On 07/21/2016 10:28 PM, Sean Paul wrote:
>>
>> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>
>> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
>> controller stop to send valid video signal, otherwhise panel would
>> go burn in, and keep flicker and flicker.
>>
>> So it's better to turn off the panel when eDP need to disable, and
>> we need to turn on the panel in connector->detect() callback, so
>> that driver would detect panel status rightly.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
>> 1 file changed, 10 insertions(+), 8 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..ea059b3 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
>> bool force)
>> {
>> struct analogix_dp_device *dp = to_dp(connector);
>>
>> + /*
>> + * Panle would prepare for several times here, but don't worry it
>>
>> s/Panle/Panel/
>>
>>
>> Done
>>
>> + * would only enable the hardware at the first prepare time.
>>
>> Errr, this shouldn't go in detect. How about putting this in
>> bridge_enable instead?
>>
>>
>> Nope, if we put this in bridge_enable, then eDP would never be enabled.
>> Here're the calling flow.
>>
>> --> analogix_dp_probe
>> --> analogix_dp_bind # we still haven't prepared the panel here, that
>> means panel have been powered up
>> --> analogix_dp_detect # Oops, losing panel valid hotplug signal, cause
>> panel have been powered up
>> --> ** Keep detecting **
>
> Yeah, after playing around with the patch yesterday I discovered this
> for myself. I still don't think detect() should be changing hardware
> state. Perhaps you could add a matching unprepare() after you attempt
> to detect the panel?
Hmm, I don't understand the meaning of "add a matching unprepare()".
I didn't see there is an unprepare() callback in drm_crtc_helper.h, would
you like to share some simple code :-D
Thanks,
- Yakir
> Sean
>
>> + */
>> + if (dp->plat_data->panel)
>> + if (drm_panel_prepare(dp->plat_data->panel))
>>
>> Personally, I don't like doing work in a conditional since you're
>> throwing the return code away. Could you break this out into:
>>
>> ret = drm_panel_prepare(dp->plat_data->panel);
>> if (ret)
>> DRM_ERROR("failed to setup the panel ret=%d\n", ret);
>>
>>
>> Okay
>>
>>
>> + DRM_ERROR("failed to setup the panel\n");
>> +
>> if (analogix_dp_detect_hpd(dp))
>> return connector_status_disconnected;
>>
>> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
>> drm_bridge *bridge)
>> return;
>>
>> if (dp->plat_data->panel) {
>> - if (drm_panel_disable(dp->plat_data->panel)) {
>> + if (drm_panel_disable(dp->plat_data->panel) ||
>> + drm_panel_unprepare(dp->plat_data->panel)) {
>>
>> Same comment here, please break this out into separate statements for
>> better readability/logging.
>>
>>
>> Okay,
>>
>>
>> Thanks,
>> - Yakir
>>
>>
>> DRM_ERROR("failed to disable the panel\n");
>> return;
>> }
>> @@ -1333,13 +1342,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,
>> --
>> 1.9.1
>>
>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
2016-07-28 3:28 ` Yakir Yang
@ 2016-07-28 14:06 ` Sean Paul
0 siblings, 0 replies; 26+ messages in thread
From: Sean Paul @ 2016-07-28 14:06 UTC (permalink / raw)
To: Yakir Yang
Cc: Mark Yao, Inki Dae, Thierry Reding, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Emil Velikov, Linux Kernel Mailing List,
dri-devel, linux-samsung-soc, linux-rockchip
On Wed, Jul 27, 2016 at 11:28 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> Sean,
>
> Oops, sorry about miss your suggest :(
>
>
> On 07/22/2016 11:03 PM, Sean Paul wrote:
>>
>> On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>
>>> Sean,
>>>
>>> Thanks for your fast respond :-)
>>>
>>> But this patch is not the latest one, I have upgraded this to "v1.1"
>>> version
>>> to fix the eDP can't be disabled problem:
>>> [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when
>>> eDP
>>> need to disable
>>> Changes in v1.1: - unprepare the panel at the end of
>>> bridge->disable() function
>>>
>>> In spite of this, I would take your comments with my "v1.1" patch.
>>>
>>> On 07/21/2016 10:28 PM, Sean Paul wrote:
>>>
>>> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>
>>> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
>>> controller stop to send valid video signal, otherwhise panel would
>>> go burn in, and keep flicker and flicker.
>>>
>>> So it's better to turn off the panel when eDP need to disable, and
>>> we need to turn on the panel in connector->detect() callback, so
>>> that driver would detect panel status rightly.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> ---
>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18
>>> ++++++++++--------
>>> 1 file changed, 10 insertions(+), 8 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..ea059b3 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
>>> bool force)
>>> {
>>> struct analogix_dp_device *dp = to_dp(connector);
>>>
>>> + /*
>>> + * Panle would prepare for several times here, but don't worry it
>>>
>>> s/Panle/Panel/
>>>
>>>
>>> Done
>>>
>>> + * would only enable the hardware at the first prepare time.
>>>
>>> Errr, this shouldn't go in detect. How about putting this in
>>> bridge_enable instead?
>>>
>>>
>>> Nope, if we put this in bridge_enable, then eDP would never be enabled.
>>> Here're the calling flow.
>>>
>>> --> analogix_dp_probe
>>> --> analogix_dp_bind # we still haven't prepared the panel here, that
>>> means panel have been powered up
>>> --> analogix_dp_detect # Oops, losing panel valid hotplug signal,
>>> cause
>>> panel have been powered up
>>> --> ** Keep detecting **
>>
>>
>> Yeah, after playing around with the patch yesterday I discovered this
>> for myself. I still don't think detect() should be changing hardware
>> state. Perhaps you could add a matching unprepare() after you attempt
>> to detect the panel?
>
>
> Hmm, I don't understand the meaning of "add a matching unprepare()".
>
> I didn't see there is an unprepare() callback in drm_crtc_helper.h, would
> you like to share some simple code :-D
>
In other words, make sure the hardware is in the same state at the
beginning and end of detect(). You shouldn't have the panel in a
different state when you exit detect().
Sean
> Thanks,
> - Yakir
>
>
>> Sean
>>
>>> + */
>>> + if (dp->plat_data->panel)
>>> + if (drm_panel_prepare(dp->plat_data->panel))
>>>
>>> Personally, I don't like doing work in a conditional since you're
>>> throwing the return code away. Could you break this out into:
>>>
>>> ret = drm_panel_prepare(dp->plat_data->panel);
>>> if (ret)
>>> DRM_ERROR("failed to setup the panel ret=%d\n", ret);
>>>
>>>
>>> Okay
>>>
>>>
>>> + DRM_ERROR("failed to setup the panel\n");
>>> +
>>> if (analogix_dp_detect_hpd(dp))
>>> return connector_status_disconnected;
>>>
>>> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
>>> drm_bridge *bridge)
>>> return;
>>>
>>> if (dp->plat_data->panel) {
>>> - if (drm_panel_disable(dp->plat_data->panel)) {
>>> + if (drm_panel_disable(dp->plat_data->panel) ||
>>> + drm_panel_unprepare(dp->plat_data->panel)) {
>>>
>>> Same comment here, please break this out into separate statements for
>>> better readability/logging.
>>>
>>>
>>> Okay,
>>>
>>>
>>> Thanks,
>>> - Yakir
>>>
>>>
>>> DRM_ERROR("failed to disable the panel\n");
>>> return;
>>> }
>>> @@ -1333,13 +1342,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,
>>> --
>>> 1.9.1
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
> --
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
@ 2016-07-28 14:06 ` Sean Paul
0 siblings, 0 replies; 26+ messages in thread
From: Sean Paul @ 2016-07-28 14:06 UTC (permalink / raw)
To: Yakir Yang
Cc: Krzysztof Kozlowski, linux-samsung-soc, linux-rockchip, Mark Yao,
Jingoo Han, Emil Velikov, Douglas Anderson, dri-devel,
Tomasz Figa, Javier Martinez Canillas, Daniel Vetter,
Stéphane Marchesin, Thierry Reding,
Linux Kernel Mailing List
On Wed, Jul 27, 2016 at 11:28 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> Sean,
>
> Oops, sorry about miss your suggest :(
>
>
> On 07/22/2016 11:03 PM, Sean Paul wrote:
>>
>> On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>
>>> Sean,
>>>
>>> Thanks for your fast respond :-)
>>>
>>> But this patch is not the latest one, I have upgraded this to "v1.1"
>>> version
>>> to fix the eDP can't be disabled problem:
>>> [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when
>>> eDP
>>> need to disable
>>> Changes in v1.1: - unprepare the panel at the end of
>>> bridge->disable() function
>>>
>>> In spite of this, I would take your comments with my "v1.1" patch.
>>>
>>> On 07/21/2016 10:28 PM, Sean Paul wrote:
>>>
>>> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>
>>> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
>>> controller stop to send valid video signal, otherwhise panel would
>>> go burn in, and keep flicker and flicker.
>>>
>>> So it's better to turn off the panel when eDP need to disable, and
>>> we need to turn on the panel in connector->detect() callback, so
>>> that driver would detect panel status rightly.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> ---
>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18
>>> ++++++++++--------
>>> 1 file changed, 10 insertions(+), 8 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..ea059b3 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
>>> bool force)
>>> {
>>> struct analogix_dp_device *dp = to_dp(connector);
>>>
>>> + /*
>>> + * Panle would prepare for several times here, but don't worry it
>>>
>>> s/Panle/Panel/
>>>
>>>
>>> Done
>>>
>>> + * would only enable the hardware at the first prepare time.
>>>
>>> Errr, this shouldn't go in detect. How about putting this in
>>> bridge_enable instead?
>>>
>>>
>>> Nope, if we put this in bridge_enable, then eDP would never be enabled.
>>> Here're the calling flow.
>>>
>>> --> analogix_dp_probe
>>> --> analogix_dp_bind # we still haven't prepared the panel here, that
>>> means panel have been powered up
>>> --> analogix_dp_detect # Oops, losing panel valid hotplug signal,
>>> cause
>>> panel have been powered up
>>> --> ** Keep detecting **
>>
>>
>> Yeah, after playing around with the patch yesterday I discovered this
>> for myself. I still don't think detect() should be changing hardware
>> state. Perhaps you could add a matching unprepare() after you attempt
>> to detect the panel?
>
>
> Hmm, I don't understand the meaning of "add a matching unprepare()".
>
> I didn't see there is an unprepare() callback in drm_crtc_helper.h, would
> you like to share some simple code :-D
>
In other words, make sure the hardware is in the same state at the
beginning and end of detect(). You shouldn't have the panel in a
different state when you exit detect().
Sean
> Thanks,
> - Yakir
>
>
>> Sean
>>
>>> + */
>>> + if (dp->plat_data->panel)
>>> + if (drm_panel_prepare(dp->plat_data->panel))
>>>
>>> Personally, I don't like doing work in a conditional since you're
>>> throwing the return code away. Could you break this out into:
>>>
>>> ret = drm_panel_prepare(dp->plat_data->panel);
>>> if (ret)
>>> DRM_ERROR("failed to setup the panel ret=%d\n", ret);
>>>
>>>
>>> Okay
>>>
>>>
>>> + DRM_ERROR("failed to setup the panel\n");
>>> +
>>> if (analogix_dp_detect_hpd(dp))
>>> return connector_status_disconnected;
>>>
>>> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
>>> drm_bridge *bridge)
>>> return;
>>>
>>> if (dp->plat_data->panel) {
>>> - if (drm_panel_disable(dp->plat_data->panel)) {
>>> + if (drm_panel_disable(dp->plat_data->panel) ||
>>> + drm_panel_unprepare(dp->plat_data->panel)) {
>>>
>>> Same comment here, please break this out into separate statements for
>>> better readability/logging.
>>>
>>>
>>> Okay,
>>>
>>>
>>> Thanks,
>>> - Yakir
>>>
>>>
>>> DRM_ERROR("failed to disable the panel\n");
>>> return;
>>> }
>>> @@ -1333,13 +1342,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,
>>> --
>>> 1.9.1
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
> --
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
2016-07-28 14:06 ` Sean Paul
(?)
@ 2016-07-29 19:16 ` Sean Paul
2016-08-01 3:27 ` Yakir Yang
` (2 more replies)
-1 siblings, 3 replies; 26+ messages in thread
From: Sean Paul @ 2016-07-29 19:16 UTC (permalink / raw)
To: ykk
Cc: yzq, inki.dae, treding, heiko, jingoohan1, javier, marcheu,
tfiga, dianders, airlied, daniel.vetter, k.kozlowski,
emil.l.velikov, dri-devel, linux-kernel, linux-samsung-soc,
linux-rockchip, Sean Paul
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(-)
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,
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
2016-07-29 19:16 ` [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared Sean Paul
@ 2016-08-01 3:27 ` Yakir Yang
2016-08-01 3:28 ` Yakir Yang
2016-08-08 18:53 ` [PATCH v2] drm/bridge: analogix_dp: " Sean Paul
2 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-08-01 3:27 UTC (permalink / raw)
To: Sean Paul
Cc: yzq, inki.dae, treding, heiko, jingoohan1, javier, marcheu,
tfiga, dianders, airlied, daniel.vetter, k.kozlowski,
emil.l.velikov, dri-devel, linux-kernel, linux-samsung-soc,
linux-rockchip
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,
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
@ 2016-08-01 3:27 ` Yakir Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-08-01 3:27 UTC (permalink / raw)
To: Sean Paul
Cc: k.kozlowski, linux-samsung-soc, linux-rockchip, yzq, jingoohan1,
emil.l.velikov, dianders, dri-devel, tfiga, javier,
daniel.vetter, marcheu, treding, linux-kernel
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,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
2016-07-29 19:16 ` [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared Sean Paul
@ 2016-08-01 3:28 ` Yakir Yang
2016-08-01 3:28 ` Yakir Yang
2016-08-08 18:53 ` [PATCH v2] drm/bridge: analogix_dp: " Sean Paul
2 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-08-01 3:28 UTC (permalink / raw)
To: Sean Paul
Cc: yzq, inki.dae, treding, heiko, jingoohan1, javier, marcheu,
tfiga, dianders, airlied, daniel.vetter, k.kozlowski,
emil.l.velikov, dri-devel, linux-kernel, linux-samsung-soc,
linux-rockchip
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,
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
@ 2016-08-01 3:28 ` Yakir Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-08-01 3:28 UTC (permalink / raw)
To: Sean Paul
Cc: k.kozlowski, linux-samsung-soc, linux-rockchip, yzq, jingoohan1,
emil.l.velikov, dianders, dri-devel, tfiga, javier,
daniel.vetter, marcheu, treding, linux-kernel
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,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
2016-08-01 3:27 ` Yakir Yang
@ 2016-08-01 20:20 ` Sean Paul
-1 siblings, 0 replies; 26+ messages in thread
From: Sean Paul @ 2016-08-01 20:20 UTC (permalink / raw)
To: Yakir Yang
Cc: 姚智情,
InKi Dae, Thierry Reding, Heiko Stübner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, Dave Airlie, Daniel Vetter,
Krzysztof Kozłowski, Emil Velikov, dri-devel,
Linux Kernel Mailing List, linux-samsung-soc, linux-rockchip
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
@ 2016-08-01 20:20 ` Sean Paul
0 siblings, 0 replies; 26+ messages in thread
From: Sean Paul @ 2016-08-01 20:20 UTC (permalink / raw)
To: Yakir Yang
Cc: Krzysztof Kozłowski, linux-samsung-soc, linux-rockchip,
姚智情,
Jingoo Han, Emil Velikov, Douglas Anderson, dri-devel,
Tomasz Figa, Javier Martinez Canillas, Daniel Vetter,
Stéphane Marchesin, Thierry Reding,
Linux Kernel Mailing List
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
2016-08-01 20:20 ` Sean Paul
@ 2016-08-01 20:46 ` Sean Paul
-1 siblings, 0 replies; 26+ messages in thread
From: Sean Paul @ 2016-08-01 20:46 UTC (permalink / raw)
To: Yakir Yang
Cc: 姚智情,
InKi Dae, Thierry Reding, Heiko Stübner, Jingoo Han,
Javier Martinez Canillas, Stéphane Marchesin, Tomasz Figa,
Douglas Anderson, Dave Airlie, Daniel Vetter,
Krzysztof Kozłowski, Emil Velikov, dri-devel,
Linux Kernel Mailing List, linux-samsung-soc, linux-rockchip
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared
@ 2016-08-01 20:46 ` Sean Paul
0 siblings, 0 replies; 26+ messages in thread
From: Sean Paul @ 2016-08-01 20:46 UTC (permalink / raw)
To: Yakir Yang
Cc: Krzysztof Kozłowski, linux-samsung-soc, linux-rockchip,
姚智情,
Jingoo Han, Emil Velikov, Douglas Anderson, dri-devel,
Tomasz Figa, Javier Martinez Canillas, Daniel Vetter,
Stéphane Marchesin, Thierry Reding,
Linux Kernel Mailing List
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2] drm/bridge: analogix_dp: Ensure the panel is properly prepared/unprepared
2016-07-29 19:16 ` [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared Sean Paul
2016-08-01 3:27 ` Yakir Yang
2016-08-01 3:28 ` Yakir Yang
@ 2016-08-08 18:53 ` Sean Paul
2016-08-09 2:35 ` Yakir Yang
2 siblings, 1 reply; 26+ messages in thread
From: Sean Paul @ 2016-08-08 18:53 UTC (permalink / raw)
To: ykk
Cc: yzq, inki.dae, treding, heiko, jingoohan1, javier, marcheu,
tfiga, dianders, airlied, daniel.vetter, k.kozlowski,
emil.l.velikov, dri-devel, linux-kernel, linux-samsung-soc,
linux-rockchip, Sean Paul
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 and edid, so we need to keep state around 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>
---
Changes in v2:
- Added panel_is_modeset state/lock to avoid racing detect with modeset (marcheu)
- Added prepare/unprepare in .get_modes (yakir)
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 101 ++++++++++++++++++---
drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 3 +
2 files changed, 93 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..47c449a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -923,11 +923,63 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
analogix_dp_start_video(dp);
}
+/*
+ * This function is a bit of a catch-all for panel preparation, hopefully
+ * simplifying the logic of functions that need to prepare/unprepare the panel
+ * below.
+ *
+ * If @prepare is true, this function will prepare the panel. Conversely, if it
+ * is false, the panel will be unprepared.
+ *
+ * If @is_modeset_prepare is true, the function will disregard the current state
+ * of the panel and either prepare/unprepare the panel based on @prepare. Once
+ * it finishes, it will update dp->panel_is_modeset to reflect the current state
+ * of the panel.
+ */
+static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
+ bool prepare, bool is_modeset_prepare)
+{
+ int ret = 0;
+
+ if (!dp->plat_data->panel)
+ return 0;
+
+ mutex_lock(&dp->panel_lock);
+
+ /*
+ * Exit early if this is a temporary prepare/unprepare and we're already
+ * modeset (since we neither want to prepare twice or unprepare early).
+ */
+ if (dp->panel_is_modeset && !is_modeset_prepare)
+ goto out;
+
+ if (prepare)
+ ret = drm_panel_prepare(dp->plat_data->panel);
+ else
+ ret = drm_panel_unprepare(dp->plat_data->panel);
+
+ if (ret)
+ goto out;
+
+ if (is_modeset_prepare)
+ dp->panel_is_modeset = prepare;
+
+out:
+ mutex_unlock(&dp->panel_lock);
+ return ret;
+}
+
int analogix_dp_get_modes(struct drm_connector *connector)
{
struct analogix_dp_device *dp = to_dp(connector);
struct edid *edid = (struct edid *)dp->edid;
- int num_modes = 0;
+ int ret, num_modes = 0;
+
+ ret = analogix_dp_prepare_panel(dp, true, false);
+ if (ret) {
+ DRM_ERROR("Failed to prepare panel (%d)\n", ret);
+ return 0;
+ }
if (analogix_dp_handle_edid(dp) == 0) {
drm_mode_connector_update_edid_property(&dp->connector, edid);
@@ -940,6 +992,10 @@ int analogix_dp_get_modes(struct drm_connector *connector)
if (dp->plat_data->get_modes)
num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
+ ret = analogix_dp_prepare_panel(dp, false, false);
+ if (ret)
+ DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
+
return num_modes;
}
@@ -960,11 +1016,23 @@ 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))
+ ret = analogix_dp_prepare_panel(dp, true, false);
+ if (ret) {
+ DRM_ERROR("Failed to prepare panel (%d)\n", ret);
return connector_status_disconnected;
+ }
+
+ if (!analogix_dp_detect_hpd(dp))
+ status = connector_status_connected;
- return connector_status_connected;
+ ret = analogix_dp_prepare_panel(dp, false, false);
+ if (ret)
+ DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
+
+ return status;
}
static void analogix_dp_connector_destroy(struct drm_connector *connector)
@@ -1035,6 +1103,16 @@ 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;
+
+ ret = analogix_dp_prepare_panel(dp, true, true);
+ 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 +1136,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 +1156,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
pm_runtime_put_sync(dp->dev);
+ ret = analogix_dp_prepare_panel(dp, false, true);
+ if (ret)
+ DRM_ERROR("failed to setup the panel ret = %d\n", ret);
+
dp->dpms_mode = DRM_MODE_DPMS_OFF;
}
@@ -1165,9 +1248,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,
@@ -1254,6 +1337,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
dp->dev = &pdev->dev;
dp->dpms_mode = DRM_MODE_DPMS_OFF;
+ mutex_init(&dp->panel_lock);
+ dp->panel_is_modeset = false;
+
/*
* platform dp driver need containor_of the plat_data to get
* the driver private data, so we need to store the point of
@@ -1333,13 +1419,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,
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index b456380..43108d7 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -178,6 +178,9 @@ struct analogix_dp_device {
bool force_hpd;
unsigned char edid[EDID_BLOCK_LENGTH * 2];
+ struct mutex panel_lock;
+ bool panel_is_modeset;
+
struct analogix_dp_plat_data *plat_data;
};
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix_dp: Ensure the panel is properly prepared/unprepared
2016-08-08 18:53 ` [PATCH v2] drm/bridge: analogix_dp: " Sean Paul
@ 2016-08-09 2:35 ` Yakir Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-08-09 2:35 UTC (permalink / raw)
To: Sean Paul
Cc: yzq, inki.dae, treding, heiko, jingoohan1, javier, marcheu,
tfiga, dianders, airlied, daniel.vetter, k.kozlowski,
emil.l.velikov, dri-devel, linux-kernel, linux-samsung-soc,
linux-rockchip, Archit Taneja
+ Archit
On 08/09/2016 02:53 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 and edid, so we need to keep state around 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>
Reviewed-by: Yakir Yang <ykk@rock-chips.com>
And I also tested this patch on RK3399 Kevin board, panel works rightly, so:
Tested-by: Yakir Yang <ykk@rock-chips.com>
Also add Archit into CC list, guess this patch should go through his
drm_bridge's tree.
Thanks,
- Yakir
> ---
>
> Changes in v2:
> - Added panel_is_modeset state/lock to avoid racing detect with modeset (marcheu)
> - Added prepare/unprepare in .get_modes (yakir)
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 101 ++++++++++++++++++---
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 3 +
> 2 files changed, 93 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..47c449a 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -923,11 +923,63 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> analogix_dp_start_video(dp);
> }
>
> +/*
> + * This function is a bit of a catch-all for panel preparation, hopefully
> + * simplifying the logic of functions that need to prepare/unprepare the panel
> + * below.
> + *
> + * If @prepare is true, this function will prepare the panel. Conversely, if it
> + * is false, the panel will be unprepared.
> + *
> + * If @is_modeset_prepare is true, the function will disregard the current state
> + * of the panel and either prepare/unprepare the panel based on @prepare. Once
> + * it finishes, it will update dp->panel_is_modeset to reflect the current state
> + * of the panel.
> + */
> +static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
> + bool prepare, bool is_modeset_prepare)
> +{
> + int ret = 0;
> +
> + if (!dp->plat_data->panel)
> + return 0;
> +
> + mutex_lock(&dp->panel_lock);
> +
> + /*
> + * Exit early if this is a temporary prepare/unprepare and we're already
> + * modeset (since we neither want to prepare twice or unprepare early).
> + */
> + if (dp->panel_is_modeset && !is_modeset_prepare)
> + goto out;
> +
> + if (prepare)
> + ret = drm_panel_prepare(dp->plat_data->panel);
> + else
> + ret = drm_panel_unprepare(dp->plat_data->panel);
> +
> + if (ret)
> + goto out;
> +
> + if (is_modeset_prepare)
> + dp->panel_is_modeset = prepare;
> +
> +out:
> + mutex_unlock(&dp->panel_lock);
> + return ret;
> +}
> +
> int analogix_dp_get_modes(struct drm_connector *connector)
> {
> struct analogix_dp_device *dp = to_dp(connector);
> struct edid *edid = (struct edid *)dp->edid;
> - int num_modes = 0;
> + int ret, num_modes = 0;
> +
> + ret = analogix_dp_prepare_panel(dp, true, false);
> + if (ret) {
> + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> + return 0;
> + }
>
> if (analogix_dp_handle_edid(dp) == 0) {
> drm_mode_connector_update_edid_property(&dp->connector, edid);
> @@ -940,6 +992,10 @@ int analogix_dp_get_modes(struct drm_connector *connector)
> if (dp->plat_data->get_modes)
> num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
>
> + ret = analogix_dp_prepare_panel(dp, false, false);
> + if (ret)
> + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> +
> return num_modes;
> }
>
> @@ -960,11 +1016,23 @@ 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))
> + ret = analogix_dp_prepare_panel(dp, true, false);
> + if (ret) {
> + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> return connector_status_disconnected;
> + }
> +
> + if (!analogix_dp_detect_hpd(dp))
> + status = connector_status_connected;
>
> - return connector_status_connected;
> + ret = analogix_dp_prepare_panel(dp, false, false);
> + if (ret)
> + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> +
> + return status;
> }
>
> static void analogix_dp_connector_destroy(struct drm_connector *connector)
> @@ -1035,6 +1103,16 @@ 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;
> +
> + ret = analogix_dp_prepare_panel(dp, true, true);
> + 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 +1136,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 +1156,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>
> pm_runtime_put_sync(dp->dev);
>
> + ret = analogix_dp_prepare_panel(dp, false, true);
> + if (ret)
> + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +
> dp->dpms_mode = DRM_MODE_DPMS_OFF;
> }
>
> @@ -1165,9 +1248,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,
> @@ -1254,6 +1337,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> dp->dev = &pdev->dev;
> dp->dpms_mode = DRM_MODE_DPMS_OFF;
>
> + mutex_init(&dp->panel_lock);
> + dp->panel_is_modeset = false;
> +
> /*
> * platform dp driver need containor_of the plat_data to get
> * the driver private data, so we need to store the point of
> @@ -1333,13 +1419,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,
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index b456380..43108d7 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -178,6 +178,9 @@ struct analogix_dp_device {
> bool force_hpd;
> unsigned char edid[EDID_BLOCK_LENGTH * 2];
>
> + struct mutex panel_lock;
> + bool panel_is_modeset;
> +
> struct analogix_dp_plat_data *plat_data;
> };
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix_dp: Ensure the panel is properly prepared/unprepared
@ 2016-08-09 2:35 ` Yakir Yang
0 siblings, 0 replies; 26+ messages in thread
From: Yakir Yang @ 2016-08-09 2:35 UTC (permalink / raw)
To: Sean Paul
Cc: k.kozlowski, linux-samsung-soc, linux-rockchip, yzq, jingoohan1,
emil.l.velikov, dianders, dri-devel, tfiga, javier,
daniel.vetter, marcheu, treding, linux-kernel
+ Archit
On 08/09/2016 02:53 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 and edid, so we need to keep state around 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>
Reviewed-by: Yakir Yang <ykk@rock-chips.com>
And I also tested this patch on RK3399 Kevin board, panel works rightly, so:
Tested-by: Yakir Yang <ykk@rock-chips.com>
Also add Archit into CC list, guess this patch should go through his
drm_bridge's tree.
Thanks,
- Yakir
> ---
>
> Changes in v2:
> - Added panel_is_modeset state/lock to avoid racing detect with modeset (marcheu)
> - Added prepare/unprepare in .get_modes (yakir)
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 101 ++++++++++++++++++---
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 3 +
> 2 files changed, 93 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..47c449a 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -923,11 +923,63 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> analogix_dp_start_video(dp);
> }
>
> +/*
> + * This function is a bit of a catch-all for panel preparation, hopefully
> + * simplifying the logic of functions that need to prepare/unprepare the panel
> + * below.
> + *
> + * If @prepare is true, this function will prepare the panel. Conversely, if it
> + * is false, the panel will be unprepared.
> + *
> + * If @is_modeset_prepare is true, the function will disregard the current state
> + * of the panel and either prepare/unprepare the panel based on @prepare. Once
> + * it finishes, it will update dp->panel_is_modeset to reflect the current state
> + * of the panel.
> + */
> +static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
> + bool prepare, bool is_modeset_prepare)
> +{
> + int ret = 0;
> +
> + if (!dp->plat_data->panel)
> + return 0;
> +
> + mutex_lock(&dp->panel_lock);
> +
> + /*
> + * Exit early if this is a temporary prepare/unprepare and we're already
> + * modeset (since we neither want to prepare twice or unprepare early).
> + */
> + if (dp->panel_is_modeset && !is_modeset_prepare)
> + goto out;
> +
> + if (prepare)
> + ret = drm_panel_prepare(dp->plat_data->panel);
> + else
> + ret = drm_panel_unprepare(dp->plat_data->panel);
> +
> + if (ret)
> + goto out;
> +
> + if (is_modeset_prepare)
> + dp->panel_is_modeset = prepare;
> +
> +out:
> + mutex_unlock(&dp->panel_lock);
> + return ret;
> +}
> +
> int analogix_dp_get_modes(struct drm_connector *connector)
> {
> struct analogix_dp_device *dp = to_dp(connector);
> struct edid *edid = (struct edid *)dp->edid;
> - int num_modes = 0;
> + int ret, num_modes = 0;
> +
> + ret = analogix_dp_prepare_panel(dp, true, false);
> + if (ret) {
> + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> + return 0;
> + }
>
> if (analogix_dp_handle_edid(dp) == 0) {
> drm_mode_connector_update_edid_property(&dp->connector, edid);
> @@ -940,6 +992,10 @@ int analogix_dp_get_modes(struct drm_connector *connector)
> if (dp->plat_data->get_modes)
> num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
>
> + ret = analogix_dp_prepare_panel(dp, false, false);
> + if (ret)
> + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> +
> return num_modes;
> }
>
> @@ -960,11 +1016,23 @@ 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))
> + ret = analogix_dp_prepare_panel(dp, true, false);
> + if (ret) {
> + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> return connector_status_disconnected;
> + }
> +
> + if (!analogix_dp_detect_hpd(dp))
> + status = connector_status_connected;
>
> - return connector_status_connected;
> + ret = analogix_dp_prepare_panel(dp, false, false);
> + if (ret)
> + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> +
> + return status;
> }
>
> static void analogix_dp_connector_destroy(struct drm_connector *connector)
> @@ -1035,6 +1103,16 @@ 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;
> +
> + ret = analogix_dp_prepare_panel(dp, true, true);
> + 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 +1136,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 +1156,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>
> pm_runtime_put_sync(dp->dev);
>
> + ret = analogix_dp_prepare_panel(dp, false, true);
> + if (ret)
> + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +
> dp->dpms_mode = DRM_MODE_DPMS_OFF;
> }
>
> @@ -1165,9 +1248,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,
> @@ -1254,6 +1337,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> dp->dev = &pdev->dev;
> dp->dpms_mode = DRM_MODE_DPMS_OFF;
>
> + mutex_init(&dp->panel_lock);
> + dp->panel_is_modeset = false;
> +
> /*
> * platform dp driver need containor_of the plat_data to get
> * the driver private data, so we need to store the point of
> @@ -1333,13 +1419,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,
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index b456380..43108d7 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -178,6 +178,9 @@ struct analogix_dp_device {
> bool force_hpd;
> unsigned char edid[EDID_BLOCK_LENGTH * 2];
>
> + struct mutex panel_lock;
> + bool panel_is_modeset;
> +
> struct analogix_dp_plat_data *plat_data;
> };
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix_dp: Ensure the panel is properly prepared/unprepared
2016-08-09 2:35 ` Yakir Yang
@ 2016-08-09 6:05 ` Daniel Vetter
-1 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2016-08-09 6:05 UTC (permalink / raw)
To: Yakir Yang
Cc: Sean Paul, k.kozlowski, linux-samsung-soc, linux-rockchip, yzq,
jingoohan1, emil.l.velikov, dianders, dri-devel, tfiga, javier,
daniel.vetter, marcheu, treding, linux-kernel
On Tue, Aug 09, 2016 at 10:35:57AM +0800, Yakir Yang wrote:
> + Archit
>
>
> On 08/09/2016 02:53 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 and edid, so we need to keep state around 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>
>
> Reviewed-by: Yakir Yang <ykk@rock-chips.com>
>
> And I also tested this patch on RK3399 Kevin board, panel works rightly, so:
> Tested-by: Yakir Yang <ykk@rock-chips.com>
>
> Also add Archit into CC list, guess this patch should go through his
> drm_bridge's tree.
drm_bridge stuff moved to -misc ;-)
-Daniel
>
> Thanks,
> - Yakir
>
> > ---
> >
> > Changes in v2:
> > - Added panel_is_modeset state/lock to avoid racing detect with modeset (marcheu)
> > - Added prepare/unprepare in .get_modes (yakir)
> >
> > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 101 ++++++++++++++++++---
> > drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 3 +
> > 2 files changed, 93 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..47c449a 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -923,11 +923,63 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> > analogix_dp_start_video(dp);
> > }
> > +/*
> > + * This function is a bit of a catch-all for panel preparation, hopefully
> > + * simplifying the logic of functions that need to prepare/unprepare the panel
> > + * below.
> > + *
> > + * If @prepare is true, this function will prepare the panel. Conversely, if it
> > + * is false, the panel will be unprepared.
> > + *
> > + * If @is_modeset_prepare is true, the function will disregard the current state
> > + * of the panel and either prepare/unprepare the panel based on @prepare. Once
> > + * it finishes, it will update dp->panel_is_modeset to reflect the current state
> > + * of the panel.
> > + */
> > +static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
> > + bool prepare, bool is_modeset_prepare)
> > +{
> > + int ret = 0;
> > +
> > + if (!dp->plat_data->panel)
> > + return 0;
> > +
> > + mutex_lock(&dp->panel_lock);
> > +
> > + /*
> > + * Exit early if this is a temporary prepare/unprepare and we're already
> > + * modeset (since we neither want to prepare twice or unprepare early).
> > + */
> > + if (dp->panel_is_modeset && !is_modeset_prepare)
> > + goto out;
> > +
> > + if (prepare)
> > + ret = drm_panel_prepare(dp->plat_data->panel);
> > + else
> > + ret = drm_panel_unprepare(dp->plat_data->panel);
> > +
> > + if (ret)
> > + goto out;
> > +
> > + if (is_modeset_prepare)
> > + dp->panel_is_modeset = prepare;
> > +
> > +out:
> > + mutex_unlock(&dp->panel_lock);
> > + return ret;
> > +}
> > +
> > int analogix_dp_get_modes(struct drm_connector *connector)
> > {
> > struct analogix_dp_device *dp = to_dp(connector);
> > struct edid *edid = (struct edid *)dp->edid;
> > - int num_modes = 0;
> > + int ret, num_modes = 0;
> > +
> > + ret = analogix_dp_prepare_panel(dp, true, false);
> > + if (ret) {
> > + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> > + return 0;
> > + }
> > if (analogix_dp_handle_edid(dp) == 0) {
> > drm_mode_connector_update_edid_property(&dp->connector, edid);
> > @@ -940,6 +992,10 @@ int analogix_dp_get_modes(struct drm_connector *connector)
> > if (dp->plat_data->get_modes)
> > num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
> > + ret = analogix_dp_prepare_panel(dp, false, false);
> > + if (ret)
> > + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> > +
> > return num_modes;
> > }
> > @@ -960,11 +1016,23 @@ 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))
> > + ret = analogix_dp_prepare_panel(dp, true, false);
> > + if (ret) {
> > + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> > return connector_status_disconnected;
> > + }
> > +
> > + if (!analogix_dp_detect_hpd(dp))
> > + status = connector_status_connected;
> > - return connector_status_connected;
> > + ret = analogix_dp_prepare_panel(dp, false, false);
> > + if (ret)
> > + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> > +
> > + return status;
> > }
> > static void analogix_dp_connector_destroy(struct drm_connector *connector)
> > @@ -1035,6 +1103,16 @@ 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;
> > +
> > + ret = analogix_dp_prepare_panel(dp, true, true);
> > + 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 +1136,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 +1156,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> > pm_runtime_put_sync(dp->dev);
> > + ret = analogix_dp_prepare_panel(dp, false, true);
> > + if (ret)
> > + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> > +
> > dp->dpms_mode = DRM_MODE_DPMS_OFF;
> > }
> > @@ -1165,9 +1248,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,
> > @@ -1254,6 +1337,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> > dp->dev = &pdev->dev;
> > dp->dpms_mode = DRM_MODE_DPMS_OFF;
> > + mutex_init(&dp->panel_lock);
> > + dp->panel_is_modeset = false;
> > +
> > /*
> > * platform dp driver need containor_of the plat_data to get
> > * the driver private data, so we need to store the point of
> > @@ -1333,13 +1419,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,
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > index b456380..43108d7 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > @@ -178,6 +178,9 @@ struct analogix_dp_device {
> > bool force_hpd;
> > unsigned char edid[EDID_BLOCK_LENGTH * 2];
> > + struct mutex panel_lock;
> > + bool panel_is_modeset;
> > +
> > struct analogix_dp_plat_data *plat_data;
> > };
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix_dp: Ensure the panel is properly prepared/unprepared
@ 2016-08-09 6:05 ` Daniel Vetter
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2016-08-09 6:05 UTC (permalink / raw)
To: Yakir Yang
Cc: k.kozlowski, linux-samsung-soc, javier, yzq, jingoohan1,
emil.l.velikov, dianders, dri-devel, tfiga, linux-rockchip,
daniel.vetter, marcheu, treding, linux-kernel
On Tue, Aug 09, 2016 at 10:35:57AM +0800, Yakir Yang wrote:
> + Archit
>
>
> On 08/09/2016 02:53 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 and edid, so we need to keep state around 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>
>
> Reviewed-by: Yakir Yang <ykk@rock-chips.com>
>
> And I also tested this patch on RK3399 Kevin board, panel works rightly, so:
> Tested-by: Yakir Yang <ykk@rock-chips.com>
>
> Also add Archit into CC list, guess this patch should go through his
> drm_bridge's tree.
drm_bridge stuff moved to -misc ;-)
-Daniel
>
> Thanks,
> - Yakir
>
> > ---
> >
> > Changes in v2:
> > - Added panel_is_modeset state/lock to avoid racing detect with modeset (marcheu)
> > - Added prepare/unprepare in .get_modes (yakir)
> >
> > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 101 ++++++++++++++++++---
> > drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 3 +
> > 2 files changed, 93 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..47c449a 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -923,11 +923,63 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> > analogix_dp_start_video(dp);
> > }
> > +/*
> > + * This function is a bit of a catch-all for panel preparation, hopefully
> > + * simplifying the logic of functions that need to prepare/unprepare the panel
> > + * below.
> > + *
> > + * If @prepare is true, this function will prepare the panel. Conversely, if it
> > + * is false, the panel will be unprepared.
> > + *
> > + * If @is_modeset_prepare is true, the function will disregard the current state
> > + * of the panel and either prepare/unprepare the panel based on @prepare. Once
> > + * it finishes, it will update dp->panel_is_modeset to reflect the current state
> > + * of the panel.
> > + */
> > +static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
> > + bool prepare, bool is_modeset_prepare)
> > +{
> > + int ret = 0;
> > +
> > + if (!dp->plat_data->panel)
> > + return 0;
> > +
> > + mutex_lock(&dp->panel_lock);
> > +
> > + /*
> > + * Exit early if this is a temporary prepare/unprepare and we're already
> > + * modeset (since we neither want to prepare twice or unprepare early).
> > + */
> > + if (dp->panel_is_modeset && !is_modeset_prepare)
> > + goto out;
> > +
> > + if (prepare)
> > + ret = drm_panel_prepare(dp->plat_data->panel);
> > + else
> > + ret = drm_panel_unprepare(dp->plat_data->panel);
> > +
> > + if (ret)
> > + goto out;
> > +
> > + if (is_modeset_prepare)
> > + dp->panel_is_modeset = prepare;
> > +
> > +out:
> > + mutex_unlock(&dp->panel_lock);
> > + return ret;
> > +}
> > +
> > int analogix_dp_get_modes(struct drm_connector *connector)
> > {
> > struct analogix_dp_device *dp = to_dp(connector);
> > struct edid *edid = (struct edid *)dp->edid;
> > - int num_modes = 0;
> > + int ret, num_modes = 0;
> > +
> > + ret = analogix_dp_prepare_panel(dp, true, false);
> > + if (ret) {
> > + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> > + return 0;
> > + }
> > if (analogix_dp_handle_edid(dp) == 0) {
> > drm_mode_connector_update_edid_property(&dp->connector, edid);
> > @@ -940,6 +992,10 @@ int analogix_dp_get_modes(struct drm_connector *connector)
> > if (dp->plat_data->get_modes)
> > num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
> > + ret = analogix_dp_prepare_panel(dp, false, false);
> > + if (ret)
> > + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> > +
> > return num_modes;
> > }
> > @@ -960,11 +1016,23 @@ 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))
> > + ret = analogix_dp_prepare_panel(dp, true, false);
> > + if (ret) {
> > + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> > return connector_status_disconnected;
> > + }
> > +
> > + if (!analogix_dp_detect_hpd(dp))
> > + status = connector_status_connected;
> > - return connector_status_connected;
> > + ret = analogix_dp_prepare_panel(dp, false, false);
> > + if (ret)
> > + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> > +
> > + return status;
> > }
> > static void analogix_dp_connector_destroy(struct drm_connector *connector)
> > @@ -1035,6 +1103,16 @@ 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;
> > +
> > + ret = analogix_dp_prepare_panel(dp, true, true);
> > + 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 +1136,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 +1156,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> > pm_runtime_put_sync(dp->dev);
> > + ret = analogix_dp_prepare_panel(dp, false, true);
> > + if (ret)
> > + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> > +
> > dp->dpms_mode = DRM_MODE_DPMS_OFF;
> > }
> > @@ -1165,9 +1248,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,
> > @@ -1254,6 +1337,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> > dp->dev = &pdev->dev;
> > dp->dpms_mode = DRM_MODE_DPMS_OFF;
> > + mutex_init(&dp->panel_lock);
> > + dp->panel_is_modeset = false;
> > +
> > /*
> > * platform dp driver need containor_of the plat_data to get
> > * the driver private data, so we need to store the point of
> > @@ -1333,13 +1419,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,
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > index b456380..43108d7 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > @@ -178,6 +178,9 @@ struct analogix_dp_device {
> > bool force_hpd;
> > unsigned char edid[EDID_BLOCK_LENGTH * 2];
> > + struct mutex panel_lock;
> > + bool panel_is_modeset;
> > +
> > struct analogix_dp_plat_data *plat_data;
> > };
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix_dp: Ensure the panel is properly prepared/unprepared
2016-08-09 2:35 ` Yakir Yang
(?)
(?)
@ 2016-08-17 5:08 ` Archit Taneja
-1 siblings, 0 replies; 26+ messages in thread
From: Archit Taneja @ 2016-08-17 5:08 UTC (permalink / raw)
To: Yakir Yang, Sean Paul
Cc: yzq, inki.dae, treding, heiko, jingoohan1, javier, marcheu,
tfiga, dianders, airlied, daniel.vetter, k.kozlowski,
emil.l.velikov, dri-devel, linux-kernel, linux-samsung-soc,
linux-rockchip
On 08/09/2016 08:05 AM, Yakir Yang wrote:
> + Archit
>
>
> On 08/09/2016 02:53 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 and edid, so we need to keep state around 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>
>
> Reviewed-by: Yakir Yang <ykk@rock-chips.com>
>
> And I also tested this patch on RK3399 Kevin board, panel works rightly,
> so:
> Tested-by: Yakir Yang <ykk@rock-chips.com>
>
> Also add Archit into CC list, guess this patch should go through his
> drm_bridge's tree.
Reviewed-by: Archit Taneja <architt@codeaurora.org>
>
> Thanks,
> - Yakir
>
>> ---
>>
>> Changes in v2:
>> - Added panel_is_modeset state/lock to avoid racing detect with
>> modeset (marcheu)
>> - Added prepare/unprepare in .get_modes (yakir)
>>
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 101
>> ++++++++++++++++++---
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 3 +
>> 2 files changed, 93 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..47c449a 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -923,11 +923,63 @@ static void analogix_dp_commit(struct
>> analogix_dp_device *dp)
>> analogix_dp_start_video(dp);
>> }
>> +/*
>> + * This function is a bit of a catch-all for panel preparation,
>> hopefully
>> + * simplifying the logic of functions that need to prepare/unprepare
>> the panel
>> + * below.
>> + *
>> + * If @prepare is true, this function will prepare the panel.
>> Conversely, if it
>> + * is false, the panel will be unprepared.
>> + *
>> + * If @is_modeset_prepare is true, the function will disregard the
>> current state
>> + * of the panel and either prepare/unprepare the panel based on
>> @prepare. Once
>> + * it finishes, it will update dp->panel_is_modeset to reflect the
>> current state
>> + * of the panel.
>> + */
>> +static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
>> + bool prepare, bool is_modeset_prepare)
>> +{
>> + int ret = 0;
>> +
>> + if (!dp->plat_data->panel)
>> + return 0;
>> +
>> + mutex_lock(&dp->panel_lock);
>> +
>> + /*
>> + * Exit early if this is a temporary prepare/unprepare and we're
>> already
>> + * modeset (since we neither want to prepare twice or unprepare
>> early).
>> + */
>> + if (dp->panel_is_modeset && !is_modeset_prepare)
>> + goto out;
>> +
>> + if (prepare)
>> + ret = drm_panel_prepare(dp->plat_data->panel);
>> + else
>> + ret = drm_panel_unprepare(dp->plat_data->panel);
>> +
>> + if (ret)
>> + goto out;
>> +
>> + if (is_modeset_prepare)
>> + dp->panel_is_modeset = prepare;
>> +
>> +out:
>> + mutex_unlock(&dp->panel_lock);
>> + return ret;
>> +}
>> +
>> int analogix_dp_get_modes(struct drm_connector *connector)
>> {
>> struct analogix_dp_device *dp = to_dp(connector);
>> struct edid *edid = (struct edid *)dp->edid;
>> - int num_modes = 0;
>> + int ret, num_modes = 0;
>> +
>> + ret = analogix_dp_prepare_panel(dp, true, false);
>> + if (ret) {
>> + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
>> + return 0;
>> + }
>> if (analogix_dp_handle_edid(dp) == 0) {
>> drm_mode_connector_update_edid_property(&dp->connector, edid);
>> @@ -940,6 +992,10 @@ int analogix_dp_get_modes(struct drm_connector
>> *connector)
>> if (dp->plat_data->get_modes)
>> num_modes += dp->plat_data->get_modes(dp->plat_data,
>> connector);
>> + ret = analogix_dp_prepare_panel(dp, false, false);
>> + if (ret)
>> + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
>> +
>> return num_modes;
>> }
>> @@ -960,11 +1016,23 @@ 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))
>> + ret = analogix_dp_prepare_panel(dp, true, false);
>> + if (ret) {
>> + DRM_ERROR("Failed to prepare panel (%d)\n", ret);
>> return connector_status_disconnected;
>> + }
>> +
>> + if (!analogix_dp_detect_hpd(dp))
>> + status = connector_status_connected;
>> - return connector_status_connected;
>> + ret = analogix_dp_prepare_panel(dp, false, false);
>> + if (ret)
>> + DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
>> +
>> + return status;
>> }
>> static void analogix_dp_connector_destroy(struct drm_connector
>> *connector)
>> @@ -1035,6 +1103,16 @@ 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;
>> +
>> + ret = analogix_dp_prepare_panel(dp, true, true);
>> + 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 +1136,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 +1156,10 @@ static void analogix_dp_bridge_disable(struct
>> drm_bridge *bridge)
>> pm_runtime_put_sync(dp->dev);
>> + ret = analogix_dp_prepare_panel(dp, false, true);
>> + if (ret)
>> + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>> +
>> dp->dpms_mode = DRM_MODE_DPMS_OFF;
>> }
>> @@ -1165,9 +1248,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,
>> @@ -1254,6 +1337,9 @@ int analogix_dp_bind(struct device *dev, struct
>> drm_device *drm_dev,
>> dp->dev = &pdev->dev;
>> dp->dpms_mode = DRM_MODE_DPMS_OFF;
>> + mutex_init(&dp->panel_lock);
>> + dp->panel_is_modeset = false;
>> +
>> /*
>> * platform dp driver need containor_of the plat_data to get
>> * the driver private data, so we need to store the point of
>> @@ -1333,13 +1419,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,
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> index b456380..43108d7 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> @@ -178,6 +178,9 @@ struct analogix_dp_device {
>> bool force_hpd;
>> unsigned char edid[EDID_BLOCK_LENGTH * 2];
>> + struct mutex panel_lock;
>> + bool panel_is_modeset;
>> +
>> struct analogix_dp_plat_data *plat_data;
>> };
>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2016-08-17 5:08 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-21 13:14 [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Yakir Yang
2016-07-21 13:14 ` [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
2016-07-21 14:28 ` Sean Paul
2016-07-22 1:00 ` Yakir Yang
2016-07-22 15:03 ` Sean Paul
2016-07-28 3:28 ` Yakir Yang
2016-07-28 14:06 ` Sean Paul
2016-07-28 14:06 ` Sean Paul
2016-07-29 19:16 ` [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared Sean Paul
2016-08-01 3:27 ` Yakir Yang
2016-08-01 3:27 ` Yakir Yang
2016-08-01 20:20 ` Sean Paul
2016-08-01 20:20 ` Sean Paul
2016-08-01 20:46 ` Sean Paul
2016-08-01 20:46 ` Sean Paul
2016-08-01 3:28 ` Yakir Yang
2016-08-01 3:28 ` Yakir Yang
2016-08-08 18:53 ` [PATCH v2] drm/bridge: analogix_dp: " Sean Paul
2016-08-09 2:35 ` Yakir Yang
2016-08-09 2:35 ` Yakir Yang
2016-08-09 6:05 ` Daniel Vetter
2016-08-09 6:05 ` Daniel Vetter
2016-08-17 5:08 ` Archit Taneja
2016-07-21 14:14 ` [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
2016-07-21 14:30 ` [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Sean Paul
2016-07-22 1:00 ` Yakir Yang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.