Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
On 2020-07-20 19:57, Rob Clark wrote: On Mon, Jul 20, 2020 at 4:32 PM Stephen Boyd wrote: Quoting khs...@codeaurora.org (2020-07-20 15:48:13) > On 2020-07-20 13:18, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2020-07-07 11:41:25) > >> drivers/gpu/drm/msm/dp/dp_power.c | 32 +- > >> drivers/gpu/drm/msm/dp/dp_power.h | 1 + > >> drivers/gpu/drm/msm/dp/dp_reg.h | 1 + > >> 17 files changed, 861 insertions(+), 424 deletions(-) > > > > It seems to spread various changes throughout the DP bits and only has > > a > > short description about what's changing. Given that the series above > > isn't merged it would be better to get rid of this change and make the > > changes in the patches that introduce these files. > > > > Yes, the base DP driver is not yet merged as its still in reviews and > has been for a while. > While it is being reviewed, different developers are working on > different aspects of DP such as base DP driver, DP compliance, audio etc > to keep things going in parallel. > To maintain the authorship of the different developers, we prefer having > them as separate changes and not merge them. > We can make all these changes as part of the same series if that shall > help to keep things together but would prefer the changes themselves to > be separate. > Please consider this and let us know if that works. > I'm not the maintainer here so it's not really up to me, but this is why we have the Co-developed-by tag, to show that multiple people worked on some patch. The patch is supposed to logically stand on its own regardless of how many people worked on it. Authorship is a single person but the Co-developed-by tag helps express that more than one person is the actual author of the patch. Can you use that tag instead and then squash this into the other DP patches? The dpu mega-patches are hard enough to review already.. I'd really appreciated it if the dpu dev's sort out some way to squash later fixups into earlier patches BR, -R as per discussion on IRC, I have separated the parts of this change which are unrelated to compliance and we have merged it to the base DP driver and added the Co-developed-by tag there. Since this change adds supports for DP compliance on MSM chipsets which is a new feature and not fixes to the base driver, we will prefer to have this as a separate change as it will make it easier for you to review it instead of continuing to expand the base DP driver ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
Quoting Kuogee Hsieh (2020-07-07 11:41:25)
> add event thread to execute events serially from event queue. Also
> timeout mode is supported which allow an event be deferred to be
> executed at later time. Both link and phy compliant tests had been
> done successfully.
>
> This change depends-on following series:
>
> https://lore.kernel.org/dri-devel/20200630184507.15589-1-tan...@codeaurora.org/
>
Can this be sent along with that series?
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +-
> drivers/gpu/drm/msm/dp/dp_aux.c | 4 +
> drivers/gpu/drm/msm/dp/dp_aux.h | 1 +
> drivers/gpu/drm/msm/dp/dp_catalog.c | 78 ++-
> drivers/gpu/drm/msm/dp/dp_ctrl.c| 361 +++
> drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 +-
> drivers/gpu/drm/msm/dp/dp_display.c | 654 +---
> drivers/gpu/drm/msm/dp/dp_hpd.c | 2 +-
> drivers/gpu/drm/msm/dp/dp_hpd.h | 1 +
> drivers/gpu/drm/msm/dp/dp_link.c| 22 +-
> drivers/gpu/drm/msm/dp/dp_panel.c | 56 +-
> drivers/gpu/drm/msm/dp/dp_panel.h | 10 +-
> drivers/gpu/drm/msm/dp/dp_parser.c | 45 +-
> drivers/gpu/drm/msm/dp/dp_parser.h | 2 +
> drivers/gpu/drm/msm/dp/dp_power.c | 32 +-
> drivers/gpu/drm/msm/dp/dp_power.h | 1 +
> drivers/gpu/drm/msm/dp/dp_reg.h | 1 +
> 17 files changed, 861 insertions(+), 424 deletions(-)
It seems to spread various changes throughout the DP bits and only has a
short description about what's changing. Given that the series above
isn't merged it would be better to get rid of this change and make the
changes in the patches that introduce these files.
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index b439e482fc80..87b291b8d7b7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1183,13 +1183,6 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
> dpu_kms = to_dpu_kms(priv->kms);
> global_state = dpu_kms_get_existing_global_state(dpu_kms);
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> - if (msm_dp_display_disable(priv->dp, drm_enc)) {
> - DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
> - return;
> - }
> - }
> -
> trace_dpu_enc_disable(DRMID(drm_enc));
>
> /* wait for idle */
> @@ -1220,6 +1213,11 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>
> dpu_rm_release(global_state, drm_enc);
>
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> + if (msm_dp_display_disable(priv->dp, drm_enc))
> + DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
> + }
> +
> mutex_unlock(_enc->enc_lock);
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 696dc8741f1e..c0e8ad031895 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -189,6 +189,8 @@ static void dp_aux_native_handler(struct dp_aux_private
> *aux)
> aux->aux_error_num = DP_AUX_ERR_TOUT;
> if (isr & DP_INTR_NACK_DEFER)
> aux->aux_error_num = DP_AUX_ERR_NACK;
> + if (isr & DP_INTR_AUX_ERROR)
> + aux->aux_error_num = DP_AUX_ERR_DPPHY_AUX;
>
> complete(>comp);
> }
> @@ -359,6 +361,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> PHY_AUX_CFG1);
> dp_catalog_aux_reset(aux->catalog);
> }
> + if (aux->aux_error_num == DP_AUX_ERR_DPPHY_AUX)
> + usleep_range(400, 400); /* need 400us before next try
> */
Typically usleep_range() should be a range, and not the same number
for both ends of the range.
> goto unlock_exit;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index ab69ae3e2dbd..367eb54c9a68 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -452,7 +452,6 @@ void dp_catalog_aux_setup(struct dp_catalog *dp_catalog)
> dp_write_phy(catalog, REG_DP_PHY_PD_CTL, DP_PHY_PD_CTL_PSR_PWRDN);
>
> /* Make sure that hardware is done with PSR power down */
> - wmb();
Is that comment above now not needed?
> dp_write_phy(catalog, REG_DP_PHY_PD_CTL, DP_PHY_PD_CTL_PWRDN |
> DP_PHY_PD_CTL_AUX_PWRDN | DP_PHY_PD_CTL_LANE_0_1_PWRDN
> | DP_PHY_PD_CTL_LANE_2_3_PWRDN | DP_PHY_PD_CTL_PLL_PWRDN
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index
Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
On 2020-07-20 13:18, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2020-07-07 11:41:25)
add event thread to execute events serially from event queue. Also
timeout mode is supported which allow an event be deferred to be
executed at later time. Both link and phy compliant tests had been
done successfully.
This change depends-on following series:
https://lore.kernel.org/dri-devel/20200630184507.15589-1-tan...@codeaurora.org/
Can this be sent along with that series?
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +-
drivers/gpu/drm/msm/dp/dp_aux.c | 4 +
drivers/gpu/drm/msm/dp/dp_aux.h | 1 +
drivers/gpu/drm/msm/dp/dp_catalog.c | 78 ++-
drivers/gpu/drm/msm/dp/dp_ctrl.c| 361 +++
drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 +-
drivers/gpu/drm/msm/dp/dp_display.c | 654
+---
drivers/gpu/drm/msm/dp/dp_hpd.c | 2 +-
drivers/gpu/drm/msm/dp/dp_hpd.h | 1 +
drivers/gpu/drm/msm/dp/dp_link.c| 22 +-
drivers/gpu/drm/msm/dp/dp_panel.c | 56 +-
drivers/gpu/drm/msm/dp/dp_panel.h | 10 +-
drivers/gpu/drm/msm/dp/dp_parser.c | 45 +-
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +
drivers/gpu/drm/msm/dp/dp_power.c | 32 +-
drivers/gpu/drm/msm/dp/dp_power.h | 1 +
drivers/gpu/drm/msm/dp/dp_reg.h | 1 +
17 files changed, 861 insertions(+), 424 deletions(-)
It seems to spread various changes throughout the DP bits and only has
a
short description about what's changing. Given that the series above
isn't merged it would be better to get rid of this change and make the
changes in the patches that introduce these files.
Yes, the base DP driver is not yet merged as its still in reviews and
has been for a while.
While it is being reviewed, different developers are working on
different aspects of DP such as base DP driver, DP compliance, audio etc
to keep things going in parallel.
To maintain the authorship of the different developers, we prefer having
them as separate changes and not merge them.
We can make all these changes as part of the same series if that shall
help to keep things together but would prefer the changes themselves to
be separate.
Please consider this and let us know if that works.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b439e482fc80..87b291b8d7b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1183,13 +1183,6 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
dpu_kms = to_dpu_kms(priv->kms);
global_state = dpu_kms_get_existing_global_state(dpu_kms);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
priv->dp) {
- if (msm_dp_display_disable(priv->dp, drm_enc)) {
- DPU_ERROR_ENC(dpu_enc, "dp display disable
failed\n");
- return;
- }
- }
-
trace_dpu_enc_disable(DRMID(drm_enc));
/* wait for idle */
@@ -1220,6 +1213,11 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
dpu_rm_release(global_state, drm_enc);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
priv->dp) {
+ if (msm_dp_display_disable(priv->dp, drm_enc))
+ DPU_ERROR_ENC(dpu_enc, "dp display disable
failed\n");
+ }
+
mutex_unlock(_enc->enc_lock);
}
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
b/drivers/gpu/drm/msm/dp/dp_aux.c
index 696dc8741f1e..c0e8ad031895 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -189,6 +189,8 @@ static void dp_aux_native_handler(struct
dp_aux_private *aux)
aux->aux_error_num = DP_AUX_ERR_TOUT;
if (isr & DP_INTR_NACK_DEFER)
aux->aux_error_num = DP_AUX_ERR_NACK;
+ if (isr & DP_INTR_AUX_ERROR)
+ aux->aux_error_num = DP_AUX_ERR_DPPHY_AUX;
complete(>comp);
}
@@ -359,6 +361,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
*dp_aux,
PHY_AUX_CFG1);
dp_catalog_aux_reset(aux->catalog);
}
+ if (aux->aux_error_num == DP_AUX_ERR_DPPHY_AUX)
+ usleep_range(400, 400); /* need 400us before
next try */
Typically usleep_range() should be a range, and not the same number
for both ends of the range.
goto unlock_exit;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index ab69ae3e2dbd..367eb54c9a68 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -452,7 +452,6 @@ void dp_catalog_aux_setup(struct dp_catalog
*dp_catalog)
dp_write_phy(catalog, REG_DP_PHY_PD_CTL,
Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
Quoting khs...@codeaurora.org (2020-07-20 15:48:13) > On 2020-07-20 13:18, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2020-07-07 11:41:25) > >> drivers/gpu/drm/msm/dp/dp_power.c | 32 +- > >> drivers/gpu/drm/msm/dp/dp_power.h | 1 + > >> drivers/gpu/drm/msm/dp/dp_reg.h | 1 + > >> 17 files changed, 861 insertions(+), 424 deletions(-) > > > > It seems to spread various changes throughout the DP bits and only has > > a > > short description about what's changing. Given that the series above > > isn't merged it would be better to get rid of this change and make the > > changes in the patches that introduce these files. > > > > Yes, the base DP driver is not yet merged as its still in reviews and > has been for a while. > While it is being reviewed, different developers are working on > different aspects of DP such as base DP driver, DP compliance, audio etc > to keep things going in parallel. > To maintain the authorship of the different developers, we prefer having > them as separate changes and not merge them. > We can make all these changes as part of the same series if that shall > help to keep things together but would prefer the changes themselves to > be separate. > Please consider this and let us know if that works. > I'm not the maintainer here so it's not really up to me, but this is why we have the Co-developed-by tag, to show that multiple people worked on some patch. The patch is supposed to logically stand on its own regardless of how many people worked on it. Authorship is a single person but the Co-developed-by tag helps express that more than one person is the actual author of the patch. Can you use that tag instead and then squash this into the other DP patches? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
On Mon, Jul 20, 2020 at 4:32 PM Stephen Boyd wrote: > > Quoting khs...@codeaurora.org (2020-07-20 15:48:13) > > On 2020-07-20 13:18, Stephen Boyd wrote: > > > Quoting Kuogee Hsieh (2020-07-07 11:41:25) > > >> drivers/gpu/drm/msm/dp/dp_power.c | 32 +- > > >> drivers/gpu/drm/msm/dp/dp_power.h | 1 + > > >> drivers/gpu/drm/msm/dp/dp_reg.h | 1 + > > >> 17 files changed, 861 insertions(+), 424 deletions(-) > > > > > > It seems to spread various changes throughout the DP bits and only has > > > a > > > short description about what's changing. Given that the series above > > > isn't merged it would be better to get rid of this change and make the > > > changes in the patches that introduce these files. > > > > > > > Yes, the base DP driver is not yet merged as its still in reviews and > > has been for a while. > > While it is being reviewed, different developers are working on > > different aspects of DP such as base DP driver, DP compliance, audio etc > > to keep things going in parallel. > > To maintain the authorship of the different developers, we prefer having > > them as separate changes and not merge them. > > We can make all these changes as part of the same series if that shall > > help to keep things together but would prefer the changes themselves to > > be separate. > > Please consider this and let us know if that works. > > > > I'm not the maintainer here so it's not really up to me, but this is why > we have the Co-developed-by tag, to show that multiple people worked on > some patch. The patch is supposed to logically stand on its own > regardless of how many people worked on it. Authorship is a single > person but the Co-developed-by tag helps express that more than one > person is the actual author of the patch. Can you use that tag instead > and then squash this into the other DP patches? The dpu mega-patches are hard enough to review already.. I'd really appreciated it if the dpu dev's sort out some way to squash later fixups into earlier patches BR, -R ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
add event thread to execute events serially from event queue. Also
timeout mode is supported which allow an event be deferred to be
executed at later time. Both link and phy compliant tests had been
done successfully.
This change depends-on following series:
https://lore.kernel.org/dri-devel/20200630184507.15589-1-tan...@codeaurora.org/
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +-
drivers/gpu/drm/msm/dp/dp_aux.c | 4 +
drivers/gpu/drm/msm/dp/dp_aux.h | 1 +
drivers/gpu/drm/msm/dp/dp_catalog.c | 78 ++-
drivers/gpu/drm/msm/dp/dp_ctrl.c| 361 +++
drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 +-
drivers/gpu/drm/msm/dp/dp_display.c | 654 +---
drivers/gpu/drm/msm/dp/dp_hpd.c | 2 +-
drivers/gpu/drm/msm/dp/dp_hpd.h | 1 +
drivers/gpu/drm/msm/dp/dp_link.c| 22 +-
drivers/gpu/drm/msm/dp/dp_panel.c | 56 +-
drivers/gpu/drm/msm/dp/dp_panel.h | 10 +-
drivers/gpu/drm/msm/dp/dp_parser.c | 45 +-
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +
drivers/gpu/drm/msm/dp/dp_power.c | 32 +-
drivers/gpu/drm/msm/dp/dp_power.h | 1 +
drivers/gpu/drm/msm/dp/dp_reg.h | 1 +
17 files changed, 861 insertions(+), 424 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b439e482fc80..87b291b8d7b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1183,13 +1183,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
dpu_kms = to_dpu_kms(priv->kms);
global_state = dpu_kms_get_existing_global_state(dpu_kms);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_disable(priv->dp, drm_enc)) {
- DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
- return;
- }
- }
-
trace_dpu_enc_disable(DRMID(drm_enc));
/* wait for idle */
@@ -1220,6 +1213,11 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
dpu_rm_release(global_state, drm_enc);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
+ if (msm_dp_display_disable(priv->dp, drm_enc))
+ DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
+ }
+
mutex_unlock(_enc->enc_lock);
}
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 696dc8741f1e..c0e8ad031895 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -189,6 +189,8 @@ static void dp_aux_native_handler(struct dp_aux_private
*aux)
aux->aux_error_num = DP_AUX_ERR_TOUT;
if (isr & DP_INTR_NACK_DEFER)
aux->aux_error_num = DP_AUX_ERR_NACK;
+ if (isr & DP_INTR_AUX_ERROR)
+ aux->aux_error_num = DP_AUX_ERR_DPPHY_AUX;
complete(>comp);
}
@@ -359,6 +361,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
PHY_AUX_CFG1);
dp_catalog_aux_reset(aux->catalog);
}
+ if (aux->aux_error_num == DP_AUX_ERR_DPPHY_AUX)
+ usleep_range(400, 400); /* need 400us before next try */
goto unlock_exit;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 2de11021d84a..1fbdfc9aa62e 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -15,6 +15,7 @@
#define DP_AUX_ERR_NACK-3
#define DP_AUX_ERR_DEFER -4
#define DP_AUX_ERR_NACK_DEFER -5
+#define DP_AUX_ERR_DPPHY_AUX -6
int dp_aux_register(struct drm_dp_aux *dp_aux);
void dp_aux_unregister(struct drm_dp_aux *dp_aux);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index ab69ae3e2dbd..367eb54c9a68 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -452,7 +452,6 @@ void dp_catalog_aux_setup(struct dp_catalog *dp_catalog)
dp_write_phy(catalog, REG_DP_PHY_PD_CTL, DP_PHY_PD_CTL_PSR_PWRDN);
/* Make sure that hardware is done with PSR power down */
- wmb();
dp_write_phy(catalog, REG_DP_PHY_PD_CTL, DP_PHY_PD_CTL_PWRDN |
DP_PHY_PD_CTL_AUX_PWRDN | DP_PHY_PD_CTL_LANE_0_1_PWRDN
| DP_PHY_PD_CTL_LANE_2_3_PWRDN | DP_PHY_PD_CTL_PLL_PWRDN
@@ -554,16 +553,21 @@ void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog
*dp_catalog,
* To make sure link reg writes happens before other operation,
* dp_write_link() function uses writel()
*/
- dp_write_link(catalog, REG_DP_MAINLINK_CTRL,
-
