Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler
On 2021-01-13 12:23, Stephen Boyd wrote:
Quoting khs...@codeaurora.org (2021-01-13 09:48:25)
On 2021-01-11 11:54, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-01-07 12:30:25)
>> There is HPD unplug interrupts missed at scenario of an irq_hpd
>> followed by unplug interrupts with around 10 ms in between.
>> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
>> irq_hpd handler should not issues either aux or sw reset to avoid
>> following unplug interrupt be cleared accidentally.
>
> So the problem is that we're resetting the DP aux phy in the middle of
> the HPD state machine transitioning states?
>
yes, after reset aux, hw clear pending hpd interrupts
>>
>> Signed-off-by: Kuogee Hsieh
>> ---
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 44f0c57..9c0ce98 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct
>> dp_catalog *dp_catalog)
>> return 0;
>> }
>>
>> +/**
>> + * dp_catalog_aux_reset() - reset AUX controller
>> + *
>> + * @aux: DP catalog structure
>> + *
>> + * return: void
>> + *
>> + * This function reset AUX controller
>> + *
>> + * NOTE: reset AUX controller will also clear any pending HPD related
>> interrupts
>> + *
>> + */
>> void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
>> {
>> u32 aux_ctrl;
>> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
>> *dp_catalog,
>> return 0;
>> }
>>
>> +/**
>> + * dp_catalog_ctrl_reset() - reset DP controller
>> + *
>> + * @aux: DP catalog structure
>
> It's called dp_catalog though.
registers access are through dp_catalog_
Agreed. The variable is not called 'aux' though, it's called
'dp_catalog'.
>
>> + *
>> + * return: void
>> + *
>> + * This function reset DP controller
>
> resets the
>
>> + *
>> + * NOTE: reset DP controller will also clear any pending HPD related
>> interrupts
>> + *
>> + */
>> void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
Right here.
fixed at v2
>> {
>> u32 sw_reset;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index e3462f5..f96c415 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct
>> dp_ctrl_private *ctrl,
>> * transitioned to PUSH_IDLE. In order to start transmitting
>> * a link training pattern, we have to first do soft reset.
>> */
>> - dp_catalog_ctrl_reset(ctrl->catalog);
>> + if (*training_step != DP_TRAINING_NONE)
>
> Can we check for the positive value instead? i.e.
> DP_TRAINING_1/DP_TRAINING_2
>
Any answer?
fixed at v2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler
Quoting khs...@codeaurora.org (2021-01-13 09:48:25)
> On 2021-01-11 11:54, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2021-01-07 12:30:25)
> >> There is HPD unplug interrupts missed at scenario of an irq_hpd
> >> followed by unplug interrupts with around 10 ms in between.
> >> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
> >> irq_hpd handler should not issues either aux or sw reset to avoid
> >> following unplug interrupt be cleared accidentally.
> >
> > So the problem is that we're resetting the DP aux phy in the middle of
> > the HPD state machine transitioning states?
> >
> yes, after reset aux, hw clear pending hpd interrupts
> >>
> >> Signed-off-by: Kuogee Hsieh
> >> ---
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index 44f0c57..9c0ce98 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct
> >> dp_catalog *dp_catalog)
> >> return 0;
> >> }
> >>
> >> +/**
> >> + * dp_catalog_aux_reset() - reset AUX controller
> >> + *
> >> + * @aux: DP catalog structure
> >> + *
> >> + * return: void
> >> + *
> >> + * This function reset AUX controller
> >> + *
> >> + * NOTE: reset AUX controller will also clear any pending HPD related
> >> interrupts
> >> + *
> >> + */
> >> void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
> >> {
> >> u32 aux_ctrl;
> >> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
> >> *dp_catalog,
> >> return 0;
> >> }
> >>
> >> +/**
> >> + * dp_catalog_ctrl_reset() - reset DP controller
> >> + *
> >> + * @aux: DP catalog structure
> >
> > It's called dp_catalog though.
> registers access are through dp_catalog_
Agreed. The variable is not called 'aux' though, it's called
'dp_catalog'.
> >
> >> + *
> >> + * return: void
> >> + *
> >> + * This function reset DP controller
> >
> > resets the
> >
> >> + *
> >> + * NOTE: reset DP controller will also clear any pending HPD related
> >> interrupts
> >> + *
> >> + */
> >> void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
Right here.
> >> {
> >> u32 sw_reset;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> index e3462f5..f96c415 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct
> >> dp_ctrl_private *ctrl,
> >> * transitioned to PUSH_IDLE. In order to start transmitting
> >> * a link training pattern, we have to first do soft reset.
> >> */
> >> - dp_catalog_ctrl_reset(ctrl->catalog);
> >> + if (*training_step != DP_TRAINING_NONE)
> >
> > Can we check for the positive value instead? i.e.
> > DP_TRAINING_1/DP_TRAINING_2
> >
Any answer?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler
On 2021-01-11 11:54, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2021-01-07 12:30:25)
There is HPD unplug interrupts missed at scenario of an irq_hpd
followed by unplug interrupts with around 10 ms in between.
Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
irq_hpd handler should not issues either aux or sw reset to avoid
following unplug interrupt be cleared accidentally.
So the problem is that we're resetting the DP aux phy in the middle of
the HPD state machine transitioning states?
yes, after reset aux, hw clear pending hpd interrupts
Signed-off-by: Kuogee Hsieh
---
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 44f0c57..9c0ce98 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct
dp_catalog *dp_catalog)
return 0;
}
+/**
+ * dp_catalog_aux_reset() - reset AUX controller
+ *
+ * @aux: DP catalog structure
+ *
+ * return: void
+ *
+ * This function reset AUX controller
+ *
+ * NOTE: reset AUX controller will also clear any pending HPD related
interrupts
+ *
+ */
void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
{
u32 aux_ctrl;
@@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
*dp_catalog,
return 0;
}
+/**
+ * dp_catalog_ctrl_reset() - reset DP controller
+ *
+ * @aux: DP catalog structure
It's called dp_catalog though.
registers access are through dp_catalog_
+ *
+ * return: void
+ *
+ * This function reset DP controller
resets the
+ *
+ * NOTE: reset DP controller will also clear any pending HPD related
interrupts
+ *
+ */
void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
{
u32 sw_reset;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index e3462f5..f96c415 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct
dp_ctrl_private *ctrl,
* transitioned to PUSH_IDLE. In order to start transmitting
* a link training pattern, we have to first do soft reset.
*/
- dp_catalog_ctrl_reset(ctrl->catalog);
+ if (*training_step != DP_TRAINING_NONE)
Can we check for the positive value instead? i.e.
DP_TRAINING_1/DP_TRAINING_2
+ dp_catalog_ctrl_reset(ctrl->catalog);
ret = dp_ctrl_link_train(ctrl, cr, training_step);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler
Quoting Kuogee Hsieh (2021-01-07 12:30:25)
> There is HPD unplug interrupts missed at scenario of an irq_hpd
> followed by unplug interrupts with around 10 ms in between.
> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
> irq_hpd handler should not issues either aux or sw reset to avoid
> following unplug interrupt be cleared accidentally.
So the problem is that we're resetting the DP aux phy in the middle of
the HPD state machine transitioning states?
>
> Signed-off-by: Kuogee Hsieh
> ---
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 44f0c57..9c0ce98 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog
> *dp_catalog)
> return 0;
> }
>
> +/**
> + * dp_catalog_aux_reset() - reset AUX controller
> + *
> + * @aux: DP catalog structure
> + *
> + * return: void
> + *
> + * This function reset AUX controller
> + *
> + * NOTE: reset AUX controller will also clear any pending HPD related
> interrupts
> + *
> + */
> void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
> {
> u32 aux_ctrl;
> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
> *dp_catalog,
> return 0;
> }
>
> +/**
> + * dp_catalog_ctrl_reset() - reset DP controller
> + *
> + * @aux: DP catalog structure
It's called dp_catalog though.
> + *
> + * return: void
> + *
> + * This function reset DP controller
resets the
> + *
> + * NOTE: reset DP controller will also clear any pending HPD related
> interrupts
> + *
> + */
> void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
> {
> u32 sw_reset;
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index e3462f5..f96c415 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct
> dp_ctrl_private *ctrl,
> * transitioned to PUSH_IDLE. In order to start transmitting
> * a link training pattern, we have to first do soft reset.
> */
> - dp_catalog_ctrl_reset(ctrl->catalog);
> + if (*training_step != DP_TRAINING_NONE)
Can we check for the positive value instead? i.e.
DP_TRAINING_1/DP_TRAINING_2
> + dp_catalog_ctrl_reset(ctrl->catalog);
>
> ret = dp_ctrl_link_train(ctrl, cr, training_step);
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler
There is HPD unplug interrupts missed at scenario of an irq_hpd
followed by unplug interrupts with around 10 ms in between.
Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
irq_hpd handler should not issues either aux or sw reset to avoid
following unplug interrupt be cleared accidentally.
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_aux.c | 7 ---
drivers/gpu/drm/msm/dp/dp_catalog.c | 24
drivers/gpu/drm/msm/dp/dp_ctrl.c| 15 ++-
3 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 19b35ae..1c6e1d2 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -336,7 +336,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
ssize_t ret;
int const aux_cmd_native_max = 16;
int const aux_cmd_i2c_max = 128;
- int const retry_count = 5;
struct dp_aux_private *aux = container_of(dp_aux,
struct dp_aux_private, dp_aux);
@@ -378,12 +377,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
ret = dp_aux_cmd_fifo_tx(aux, msg);
if (ret < 0) {
- if (aux->native) {
- aux->retry_cnt++;
- if (!(aux->retry_cnt % retry_count))
- dp_catalog_aux_update_cfg(aux->catalog);
- dp_catalog_aux_reset(aux->catalog);
- }
usleep_range(400, 500); /* at least 400us to next try */
goto unlock_exit;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 44f0c57..9c0ce98 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog
*dp_catalog)
return 0;
}
+/**
+ * dp_catalog_aux_reset() - reset AUX controller
+ *
+ * @aux: DP catalog structure
+ *
+ * return: void
+ *
+ * This function reset AUX controller
+ *
+ * NOTE: reset AUX controller will also clear any pending HPD related
interrupts
+ *
+ */
void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
{
u32 aux_ctrl;
@@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
*dp_catalog,
return 0;
}
+/**
+ * dp_catalog_ctrl_reset() - reset DP controller
+ *
+ * @aux: DP catalog structure
+ *
+ * return: void
+ *
+ * This function reset DP controller
+ *
+ * NOTE: reset DP controller will also clear any pending HPD related interrupts
+ *
+ */
void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
{
u32 sw_reset;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index e3462f5..f96c415 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct dp_ctrl_private
*ctrl,
* transitioned to PUSH_IDLE. In order to start transmitting
* a link training pattern, we have to first do soft reset.
*/
- dp_catalog_ctrl_reset(ctrl->catalog);
+ if (*training_step != DP_TRAINING_NONE)
+ dp_catalog_ctrl_reset(ctrl->catalog);
ret = dp_ctrl_link_train(ctrl, cr, training_step);
@@ -1491,15 +1492,18 @@ static int dp_ctrl_deinitialize_mainlink(struct
dp_ctrl_private *ctrl)
return 0;
}
+static void dp_ctrl_link_idle_reset(struct dp_ctrl_private *ctrl)
+{
+ dp_ctrl_push_idle(>dp_ctrl);
+ dp_catalog_ctrl_reset(ctrl->catalog);
+}
+
static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
{
int ret = 0;
struct dp_cr_status cr;
int training_step = DP_TRAINING_NONE;
- dp_ctrl_push_idle(>dp_ctrl);
- dp_catalog_ctrl_reset(ctrl->catalog);
-
ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
ret = dp_ctrl_setup_main_link(ctrl, , _step);
@@ -1626,6 +1630,7 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
if (sink_request & DP_TEST_LINK_TRAINING) {
dp_link_send_test_response(ctrl->link);
+ dp_ctrl_link_idle_reset(ctrl);
if (dp_ctrl_link_maintenance(ctrl)) {
DRM_ERROR("LM failed: TEST_LINK_TRAINING\n");
return;
@@ -1679,7 +1684,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
break;
}
- training_step = DP_TRAINING_NONE;
+ training_step = DP_TRAINING_1;
rc = dp_ctrl_setup_main_link(ctrl, , _step);
if (rc == 0) {
/* training completed successfully */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
