* [PATCH] drm/msm/dp: force link training for display resolution change
@ 2022-05-26 17:26 Kuogee Hsieh
2022-05-27 0:07 ` Stephen Boyd
0 siblings, 1 reply; 3+ messages in thread
From: Kuogee Hsieh @ 2022-05-26 17:26 UTC (permalink / raw)
To: robdclark, sean, swboyd, dianders, vkoul, daniel, airlied,
agross, dmitry.baryshkov, bjorn.andersson
Cc: quic_abhinavk, quic_aravindh, quic_khsieh, quic_sbillaka,
freedreno, dri-devel, linux-arm-msm, linux-kernel
During display resolution changes display have to be disabled first
followed by display enable with new resolution. This patch force
main link always be retrained during display enable procedure to
simplify implementation instead of manually kicking of irq_hpd
handle.
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 6 +++---
drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +-
drivers/gpu/drm/msm/dp/dp_display.c | 13 ++++++-------
3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 5ddb4e8..8b71013 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1545,7 +1545,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
if (!ret)
- ret = dp_ctrl_on_stream(&ctrl->dp_ctrl);
+ ret = dp_ctrl_on_stream(&ctrl->dp_ctrl, false);
else
DRM_ERROR("failed to enable DP link controller\n");
@@ -1802,7 +1802,7 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
return dp_ctrl_setup_main_link(ctrl, &training_step);
}
-int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
+int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
{
int ret = 0;
bool mainlink_ready = false;
@@ -1827,7 +1827,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
}
}
- if (!dp_ctrl_channel_eq_ok(ctrl))
+ if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl))
dp_ctrl_link_retrain(ctrl);
/* stop txing train pattern to end link training */
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..dcc7af21 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -20,7 +20,7 @@ struct dp_ctrl {
};
int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
-int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train);
int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bfc6581..9246421 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -892,7 +892,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
return 0;
}
- rc = dp_ctrl_on_stream(dp->ctrl);
+ rc = dp_ctrl_on_stream(dp->ctrl, data);
if (!rc)
dp_display->power_on = true;
@@ -1594,6 +1594,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
int rc = 0;
struct dp_display_private *dp_display;
u32 state;
+ bool force_link_train = false;
dp_display = container_of(dp, struct dp_display_private, dp_display);
if (!dp_display->dp_mode.drm_mode.clock) {
@@ -1622,10 +1623,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
state = dp_display->hpd_state;
- if (state == ST_DISPLAY_OFF)
+ if (state == ST_DISPLAY_OFF) {
dp_display_host_phy_init(dp_display);
+ force_link_train = 1;
+ }
- dp_display_enable(dp_display, 0);
+ dp_display_enable(dp_display, force_link_train);
rc = dp_display_post_enable(dp);
if (rc) {
@@ -1634,10 +1637,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
dp_display_unprepare(dp);
}
- /* manual kick off plug event to train link */
- if (state == ST_DISPLAY_OFF)
- dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0);
-
/* completed connection */
dp_display->hpd_state = ST_CONNECTED;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/msm/dp: force link training for display resolution change
2022-05-26 17:26 [PATCH] drm/msm/dp: force link training for display resolution change Kuogee Hsieh
@ 2022-05-27 0:07 ` Stephen Boyd
2022-05-27 15:25 ` Kuogee Hsieh
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2022-05-27 0:07 UTC (permalink / raw)
To: Kuogee Hsieh, agross, airlied, bjorn.andersson, daniel, dianders,
dmitry.baryshkov, robdclark, sean, vkoul
Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
dri-devel, linux-arm-msm, linux-kernel
Quoting Kuogee Hsieh (2022-05-26 10:26:18)
> During display resolution changes display have to be disabled first
> followed by display enable with new resolution. This patch force
> main link always be retrained during display enable procedure to
> simplify implementation instead of manually kicking of irq_hpd
> handle.
How is that better? Can you please describe how it improves things? I
suppose it avoids some round trips through the event queue just to turn
on the display?
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Any Fixes tag? Or it's not fixing anything, just making things simpler?
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 5ddb4e8..8b71013 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 2433edb..dcc7af21 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -20,7 +20,7 @@ struct dp_ctrl {
> };
>
> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
> -int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
> +int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train);
Can we have another API like dp_ctrl_on_stream_retrain()? Or name
'force_link_train' to 'retrain'?
> int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
> int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
> void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bfc6581..9246421 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -892,7 +892,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
Any reason why dp_display_enable() is forward declared and why it takes
a u32 data argument? It's not part of the eventqueue calling code from
what I can tell so we should be able to rename 'data' to 'retrain_link'?
> return 0;
> }
>
> - rc = dp_ctrl_on_stream(dp->ctrl);
> + rc = dp_ctrl_on_stream(dp->ctrl, data);
> if (!rc)
> dp_display->power_on = true;
>
> @@ -1594,6 +1594,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> int rc = 0;
> struct dp_display_private *dp_display;
> u32 state;
> + bool force_link_train = false;
>
> dp_display = container_of(dp, struct dp_display_private, dp_display);
> if (!dp_display->dp_mode.drm_mode.clock) {
> @@ -1622,10 +1623,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>
> state = dp_display->hpd_state;
>
> - if (state == ST_DISPLAY_OFF)
> + if (state == ST_DISPLAY_OFF) {
> dp_display_host_phy_init(dp_display);
> + force_link_train = 1;
Use true instead of 1 if the type is a bool please.
> + }
>
> - dp_display_enable(dp_display, 0);
> + dp_display_enable(dp_display, force_link_train);
>
> rc = dp_display_post_enable(dp);
> if (rc) {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/msm/dp: force link training for display resolution change
2022-05-27 0:07 ` Stephen Boyd
@ 2022-05-27 15:25 ` Kuogee Hsieh
0 siblings, 0 replies; 3+ messages in thread
From: Kuogee Hsieh @ 2022-05-27 15:25 UTC (permalink / raw)
To: Stephen Boyd, agross, airlied, bjorn.andersson, daniel, dianders,
dmitry.baryshkov, robdclark, sean, vkoul
Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
dri-devel, linux-arm-msm, linux-kernel
On 5/26/2022 5:07 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-05-26 10:26:18)
>> During display resolution changes display have to be disabled first
>> followed by display enable with new resolution. This patch force
>> main link always be retrained during display enable procedure to
>> simplify implementation instead of manually kicking of irq_hpd
>> handle.
> How is that better? Can you please describe how it improves things? I
> suppose it avoids some round trips through the event queue just to turn
> on the display?
This try to fix customer issues which only happen on one particular
panel which fails on display resolution changes sometimes.
I will add more description.
>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Any Fixes tag? Or it's not fixing anything, just making things simpler?
will add.
>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 5ddb4e8..8b71013 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index 2433edb..dcc7af21 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -20,7 +20,7 @@ struct dp_ctrl {
>> };
>>
>> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>> -int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
>> +int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train);
> Can we have another API like dp_ctrl_on_stream_retrain()? Or name
> 'force_link_train' to 'retrain'?
>
>> int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
>> int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
>> void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index bfc6581..9246421 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -892,7 +892,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
> Any reason why dp_display_enable() is forward declared and why it takes
> a u32 data argument? It's not part of the eventqueue calling code from
> what I can tell so we should be able to rename 'data' to 'retrain_link'?
>
>> return 0;
>> }
>>
>> - rc = dp_ctrl_on_stream(dp->ctrl);
>> + rc = dp_ctrl_on_stream(dp->ctrl, data);
>> if (!rc)
>> dp_display->power_on = true;
>>
>> @@ -1594,6 +1594,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>> int rc = 0;
>> struct dp_display_private *dp_display;
>> u32 state;
>> + bool force_link_train = false;
>>
>> dp_display = container_of(dp, struct dp_display_private, dp_display);
>> if (!dp_display->dp_mode.drm_mode.clock) {
>> @@ -1622,10 +1623,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>>
>> state = dp_display->hpd_state;
>>
>> - if (state == ST_DISPLAY_OFF)
>> + if (state == ST_DISPLAY_OFF) {
>> dp_display_host_phy_init(dp_display);
>> + force_link_train = 1;
> Use true instead of 1 if the type is a bool please.
>
>> + }
>>
>> - dp_display_enable(dp_display, 0);
>> + dp_display_enable(dp_display, force_link_train);
>>
>> rc = dp_display_post_enable(dp);
>> if (rc) {
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-27 15:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-26 17:26 [PATCH] drm/msm/dp: force link training for display resolution change Kuogee Hsieh
2022-05-27 0:07 ` Stephen Boyd
2022-05-27 15:25 ` Kuogee Hsieh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).