date:20240405

Re: [PATCH v2] drm/msm/dp: Remove now unused connector_type from desc

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 08:14:11PM -0700, Bjorn Andersson wrote:
> Now that the connector_type is dynamically determined, the
> connector_type of the struct msm_dp_desc is unused. Clean it up.
> 
> Remaining duplicate entries are squashed.
> 
> Signed-off-by: Bjorn Andersson 
> ---
> This cleans up after, and hence depends on,
> https://lore.kernel.org/all/20240324-x1e80100-display-refactor-connector-v4-1-e0ebaea66...@linaro.org/
> ---
> Changes in v2:
> - Squashed now duplicate entries
> - Link to v1: 
> https://lore.kernel.org/r/20240328-dp-connector-type-cleanup-v1-1-9bf84c5a6...@quicinc.com
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 48 
> +
>  1 file changed, 17 insertions(+), 31 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov 

-- 
With best wishes
Dmitry

Re: [PATCH 0/6] Add SMEM-based speedbin matching

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 10:41:28AM +0200, Konrad Dybcio wrote:
> Newer (SM8550+) SoCs don't seem to have a nice speedbin fuse anymore,
> but instead rely on a set of combinations of "feature code" (FC) and
> "product code" (PC) identifiers to match the bins. This series adds
> support for that.
> 
> I suppose a qcom/for-soc immutable branch would be in order if we want
> to land this in the upcoming cycle.
> 
> FWIW I preferred the fuses myself..
> 
> Signed-off-by: Konrad Dybcio 
> ---
> Konrad Dybcio (5):
>   soc: qcom: Move some socinfo defines to the header, expand them
>   soc: qcom: smem: Add pcode/fcode getters
>   drm/msm/adreno: Implement SMEM-based speed bin
>   drm/msm/adreno: Add speedbin data for SM8550 / A740
>   arm64: dts: qcom: sm8550: Wire up GPU speed bin & more OPPs
> 
> Neil Armstrong (1):
>   drm/msm/adreno: Allow specifying default speedbin value

Generic comment: as you are reworking speed bins implementaiton, could
you please take a broader look. A5xx just reads nvmem manually. A6xx
uses adreno_read_speedbin(). And then we call adreno_read_speedbin
second time from from adreno_gpu_init(). Can we get to the point where
the function is called only once for all the platforms which implements
speed binning?

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] drm/msm/adreno: Add speedbin data for SM8550 / A740

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 10:41:33AM +0200, Konrad Dybcio wrote:
> Add speebin data for A740, as found on SM8550 and derivative SoCs.
> 
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 901ef767e491..c976a485aef2 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -570,6 +570,20 @@ static const struct adreno_info gpulist[] = {
>   .zapfw = "a740_zap.mdt",
>   .hwcg = a740_hwcg,
>   .address_space_size = SZ_16G,
> + .speedbins = ADRENO_SPEEDBINS(

I think this deserves either a comment or some info in the commit
message.

> + { ADRENO_SKU_ID(SOCINFO_PC_UNKNOWN, SOCINFO_FC_AC), 0 },
> + { ADRENO_SKU_ID(SOCINFO_PC_UNKNOWN, SOCINFO_FC_AF), 0 },
> + { ADRENO_SKU_ID(SOCINFO_PCn(1), SOCINFO_FC_UNKNOWN), 1 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(2), SOCINFO_FC_Yn(0x0)), 0 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(2), SOCINFO_FC_Yn(0x2)), 0 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(4), SOCINFO_FC_Yn(0x0)), 0 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(4), SOCINFO_FC_Yn(0x2)), 0 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0x0)), 0 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0x1)), 0 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0xd)), 0 
> },
> + { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0xe)), 0 
> },
> + ),
> + .default_speedbin = 1,
>   }, {
>   .chip_ids = ADRENO_CHIP_IDS(0x43051401), /* "C520v2" */
>   .family = ADRENO_7XX_GEN3,
> 
> -- 
> 2.40.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 4/6] drm/msm/adreno: Implement SMEM-based speed bin

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 10:41:32AM +0200, Konrad Dybcio wrote:
> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
> abstracted through SMEM, instead of being directly available in a fuse.
> 
> Add support for SMEM-based speed binning, which includes getting
> "feature code" and "product code" from said source and parsing them
> to form something that lets us match OPPs against.
> 
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c  |  8 +++---
>  drivers/gpu/drm/msm/adreno/adreno_device.c |  2 ++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c| 39 
> +++---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h| 12 ++---
>  4 files changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 4cbdfabbcee5..6776fd80f7a6 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2890,13 +2890,15 @@ static u32 fuse_to_supp_hw(const struct adreno_info 
> *info, u32 fuse)
>   return UINT_MAX;
>  }
>  
> -static int a6xx_set_supported_hw(struct device *dev, const struct 
> adreno_info *info)
> +static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
> +  struct device *dev,
> +  const struct adreno_info *info)
>  {
>   u32 supp_hw;
>   u32 speedbin;
>   int ret;
>  
> - ret = adreno_read_speedbin(dev, );
> + ret = adreno_read_speedbin(adreno_gpu, dev, );
>   /*
>* -ENOENT means that the platform doesn't support speedbin which is
>* fine
> @@ -3056,7 +3058,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>  
>   a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
>  
> - ret = a6xx_set_supported_hw(>dev, config->info);
> + ret = a6xx_set_supported_hw(adreno_gpu, >dev, config->info);
>   if (ret) {
>   a6xx_destroy(&(a6xx_gpu->base.base));
>   return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index c3703a51287b..901ef767e491 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -6,6 +6,8 @@
>   * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved.
>   */
>  
> +#include 
> +
>  #include "adreno_gpu.h"
>  
>  bool hang_debug = false;
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 074fb498706f..0e4ff532ac3c 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -21,6 +21,9 @@
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
>  
> +#include 
> +#include 
> +
>  static u64 address_space_size = 0;
>  MODULE_PARM_DESC(address_space_size, "Override for size of processes private 
> GPU address space");
>  module_param(address_space_size, ullong, 0600);
> @@ -1057,9 +1060,37 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem 
> *adreno_ocmem)
>  adreno_ocmem->hdl);
>  }
>  
> -int adreno_read_speedbin(struct device *dev, u32 *speedbin)
> +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
> +  struct device *dev, u32 *speedbin)
>  {
> - return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
> + u32 fcode, pcode;
> + int ret;
> +
> + /* Try reading the speedbin via a nvmem cell first */
> + ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
> + if (!ret && ret != -EINVAL)

This is always false.

> + return ret;
> +
> + ret = qcom_smem_get_feature_code();
> + if (ret) {
> + dev_err(dev, "Couldn't get feature code from SMEM!\n");
> + return ret;

This brings in QCOM_SMEM dependency (which is not mentioned in the
Kconfig). Please keep iMX5 hardware in mind, so the dependency should be
optional. Respective functions should be stubbed in the header.

> + }
> +
> + ret = qcom_smem_get_product_code();
> + if (ret) {
> + dev_err(dev, "Couldn't get product code from SMEM!\n");
> + return ret;
> + }
> +
> + /* Don't consider fcode for external feature codes */
> + if (fcode <= SOCINFO_FC_EXT_RESERVE)
> + fcode = SOCINFO_FC_UNKNOWN;
> +
> + *speedbin = FIELD_PREP(ADRENO_SKU_ID_PCODE, pcode) |
> + FIELD_PREP(ADRENO_SKU_ID_FCODE, fcode);

What about just asking the qcom_smem for the 'gpu_bin' and hiding gory
details there? It almost feels that handling raw PCODE / FCODE here is
too low-level and a subject to change depending on the socinfo format.

> +
> + return ret;
>  }
>  
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> @@ -1098,9 +1129,9 @@ int adreno_gpu_init(struct drm_device *drm, struct 
> platform_device *pdev,
>

[PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

2024-04-05 Thread Abhinav Kumar

From: Kuogee Hsieh 

For HPD events coming from external modules using drm_bridge_hpd_notify(),
the sequence of calls leading to dp_bridge_hpd_notify() is like below:

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_connector_hotplug_event+0x30/0x3c [drm_kms_helper]
drm_bridge_connector_handle_hpd+0x84/0x94 [drm_kms_helper]
drm_bridge_connector_hpd_cb+0xc/0x14 [drm_kms_helper]
drm_bridge_hpd_notify+0x38/0x50 [drm]
drm_aux_hpd_bridge_notify+0x14/0x20 [aux_hpd_bridge]
pmic_glink_altmode_worker+0xec/0x27c [pmic_glink_altmode]
process_scheduled_works+0x17c/0x2cc
worker_thread+0x2ac/0x2d0
kthread+0xfc/0x120

There are three notifications delivered to DP driver for each notification 
event.

1) From the drm_aux_hpd_bridge_notify() itself as shown above

2) From output_poll_execute() thread which arises due to
drm_helper_probe_single_connector_modes() call of the above stacktrace
as shown in more detail here.

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_hotplug_event+0x30/0x3c [drm_kms_helper]
output_poll_execute+0xe0/0x210 [drm_kms_helper]

3) From the DP driver as the dp_bridge_hpd_notify() callback today triggers
the hpd_event_thread for connect and disconnect events respectively via below 
stack

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect_ctx+0x98/0x110 [drm_kms_helper]
check_connector_changed+0x4c/0x20c [drm_kms_helper]
drm_helper_hpd_irq_event+0x98/0x120 [drm_kms_helper]
hpd_event_thread+0x478/0x5bc [msm]

dp_bridge_hpd_notify() delivered from output_poll_execute() thread
returns the incorrect HPD status as the MSM DP driver returns the value
of link_ready and not the HPD status currently in the .detect() callback.

And because the HPD event thread has not run yet, this results in two 
complementary
events.

To address this, fix dp_bridge_hpd_notify() to call 
dp_hpd_plug_handle/unplug_handle()
directly to return consistent values for the above scenarios.

changes in v3:
- Fix the commit message as per submitting guidelines.
- remove extra line added

changes in v2:
- Fix the commit message to explain the scenario
- Fix the subject a little as well

Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Signed-off-by: Kuogee Hsieh 
Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
 
if (!dp_display->link_ready && status == connector_status_connected)
-   dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+   dp_hpd_plug_handle(dp, 0);
else if (dp_display->link_ready && status == 
connector_status_disconnected)
-   dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+   dp_hpd_unplug_handle(dp, 0);
 }
-- 
2.43.2

[PATCH v2] drm/msm/dp: Remove now unused connector_type from desc

2024-04-05 Thread Bjorn Andersson

Now that the connector_type is dynamically determined, the
connector_type of the struct msm_dp_desc is unused. Clean it up.

Remaining duplicate entries are squashed.

Signed-off-by: Bjorn Andersson 
---
This cleans up after, and hence depends on,
https://lore.kernel.org/all/20240324-x1e80100-display-refactor-connector-v4-1-e0ebaea66...@linaro.org/
---
Changes in v2:
- Squashed now duplicate entries
- Link to v1: 
https://lore.kernel.org/r/20240328-dp-connector-type-cleanup-v1-1-9bf84c5a6...@quicinc.com
---
 drivers/gpu/drm/msm/dp/dp_display.c | 48 +
 1 file changed, 17 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 521cba76d2a0..12c01625c551 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -119,55 +119,41 @@ struct dp_display_private {
 struct msm_dp_desc {
phys_addr_t io_start;
unsigned int id;
-   unsigned int connector_type;
bool wide_bus_supported;
 };
 
 static const struct msm_dp_desc sc7180_dp_descs[] = {
-   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort },
+   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0 },
{}
 };
 
 static const struct msm_dp_desc sc7280_dp_descs[] = {
-   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x0aea, .id = MSM_DP_CONTROLLER_1, .connector_type = 
DRM_MODE_CONNECTOR_eDP, .wide_bus_supported = true },
+   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0, 
.wide_bus_supported = true },
+   { .io_start = 0x0aea, .id = MSM_DP_CONTROLLER_1, 
.wide_bus_supported = true },
{}
 };
 
 static const struct msm_dp_desc sc8180x_dp_descs[] = {
-   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort },
-   { .io_start = 0x0ae98000, .id = MSM_DP_CONTROLLER_1, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort },
-   { .io_start = 0x0ae9a000, .id = MSM_DP_CONTROLLER_2, .connector_type = 
DRM_MODE_CONNECTOR_eDP },
+   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0 },
+   { .io_start = 0x0ae98000, .id = MSM_DP_CONTROLLER_1 },
+   { .io_start = 0x0ae9a000, .id = MSM_DP_CONTROLLER_2 },
{}
 };
 
 static const struct msm_dp_desc sc8280xp_dp_descs[] = {
-   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x0ae98000, .id = MSM_DP_CONTROLLER_1, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x0ae9a000, .id = MSM_DP_CONTROLLER_2, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x0aea, .id = MSM_DP_CONTROLLER_3, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x2209, .id = MSM_DP_CONTROLLER_0, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x22098000, .id = MSM_DP_CONTROLLER_1, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x2209a000, .id = MSM_DP_CONTROLLER_2, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   { .io_start = 0x220a, .id = MSM_DP_CONTROLLER_3, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_supported = true },
-   {}
-};
-
-static const struct msm_dp_desc sc8280xp_edp_descs[] = {
-   { .io_start = 0x0ae9a000, .id = MSM_DP_CONTROLLER_2, .connector_type = 
DRM_MODE_CONNECTOR_eDP, .wide_bus_supported = true },
-   { .io_start = 0x0aea, .id = MSM_DP_CONTROLLER_3, .connector_type = 
DRM_MODE_CONNECTOR_eDP, .wide_bus_supported = true },
-   { .io_start = 0x2209a000, .id = MSM_DP_CONTROLLER_2, .connector_type = 
DRM_MODE_CONNECTOR_eDP, .wide_bus_supported = true },
-   { .io_start = 0x220a, .id = MSM_DP_CONTROLLER_3, .connector_type = 
DRM_MODE_CONNECTOR_eDP, .wide_bus_supported = true },
-   {}
-};
-
-static const struct msm_dp_desc sm8350_dp_descs[] = {
-   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0, .connector_type = 
DRM_MODE_CONNECTOR_DisplayPort },
+   { .io_start = 0x0ae9, .id = MSM_DP_CONTROLLER_0, 
.wide_bus_supported = true },
+   { .io_start = 0x0ae98000, .id = MSM_DP_CONTROLLER_1, 
.wide_bus_supported = true },
+   { .io_start = 0x0ae9a000, .id = MSM_DP_CONTROLLER_2, 
.wide_bus_supported = true },
+   { .io_start = 0x0aea, .id = MSM_DP_CONTROLLER_3, 
.wide_bus_supported = true },
+   { .io_start = 0x2209, .id = MSM_DP_CONTROLLER_0, 
.wide_bus_supported = true },
+   { .io_start = 0x22098000, .id = MSM_DP_CONTROLLER_1, 
.wide_bus_supported = true },
+   { .io_start

Re: [PATCH 3/6] drm/msm/adreno: Allow specifying default speedbin value

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote:
> From: Neil Armstrong 
> 
> Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock
> the highest. Falling back to it when things go wrong is largely
> suboptimal, as more often than not, the top frequencies are not
> supposed to work on other bins.

Isn't it better to just return an error here instead of trying to guess
which speedbin to use?

If that's not the case, I think the commit should be expanded with
actually setting default_speedbin for the existing GPUs.

> 
> Let the developer specify the intended "lowest common denominator" bin
> in struct adreno_info. If not specified, partial struct initialization
> will ensure it's set to zero, retaining previous behavior.
> 
> Signed-off-by: Neil Armstrong 
> [Konrad: clean up, add commit message]
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 2 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 0674aca0f8a3..4cbdfabbcee5 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2915,7 +2915,7 @@ static int a6xx_set_supported_hw(struct device *dev, 
> const struct adreno_info *i
>   DRM_DEV_ERROR(dev,
>   "missing support for speed-bin: %u. Some OPPs may not 
> be supported by hardware\n",
>   speedbin);
> - supp_hw = BIT(0); /* Default */
> + supp_hw = BIT(info->default_speedbin); /* Default */
>   }
>  
>   ret = devm_pm_opp_set_supported_hw(dev, _hw, 1);
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 77526892eb8c..460b399be37b 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -110,6 +110,7 @@ struct adreno_info {
>* {SHRT_MAX, 0} sentinal.
>*/
>   struct adreno_speedbin *speedbins;
> + unsigned int default_speedbin;
>  };
>  
>  #define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }
> 
> -- 
> 2.40.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
> In preparation for parsing the chip "feature code" (FC) and "product
> code" (PC) (essentially the parameters that let us conclusively
> characterize the sillicon we're running on, including various speed
> bins), move the socinfo version defines to the public header and
> include some more FC/PC defines.
> 
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/soc/qcom/socinfo.c   |  8 
>  include/linux/soc/qcom/socinfo.h | 36 
>  2 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 277c07a6603d..cf4616a468f2 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -21,14 +21,6 @@
>  
>  #include 
>  
> -/*
> - * SoC version type with major number in the upper 16 bits and minor
> - * number in the lower 16 bits.
> - */
> -#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0x)
> -#define SOCINFO_MINOR(ver) ((ver) & 0x)
> -#define SOCINFO_VERSION(maj, min)  maj) & 0x) << 16)|((min) & 
> 0x))
> -
>  /* Helper macros to create soc_id table */
>  #define qcom_board_id(id) QCOM_ID_ ## id, __stringify(id)
>  #define qcom_board_id_named(id, name) QCOM_ID_ ## id, (name)
> diff --git a/include/linux/soc/qcom/socinfo.h 
> b/include/linux/soc/qcom/socinfo.h
> index e78777bb0f4a..ba7f683bd32c 100644
> --- a/include/linux/soc/qcom/socinfo.h
> +++ b/include/linux/soc/qcom/socinfo.h
> @@ -3,6 +3,8 @@
>  #ifndef __QCOM_SOCINFO_H__
>  #define __QCOM_SOCINFO_H__
>  
> +#include 
> +
>  /*
>   * SMEM item id, used to acquire handles to respective
>   * SMEM region.
> @@ -12,6 +14,14 @@
>  #define SMEM_SOCINFO_BUILD_ID_LENGTH 32
>  #define SMEM_SOCINFO_CHIP_ID_LENGTH  32
>  
> +/*
> + * SoC version type with major number in the upper 16 bits and minor
> + * number in the lower 16 bits.
> + */
> +#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0x)
> +#define SOCINFO_MINOR(ver) ((ver) & 0x)
> +#define SOCINFO_VERSION(maj, min)  maj) & 0x) << 16)|((min) & 
> 0x))
> +
>  /* Socinfo SMEM item structure */
>  struct socinfo {
>   __le32 fmt;
> @@ -74,4 +84,30 @@ struct socinfo {
>   __le32 boot_core;
>  };
>  
> +/* Internal feature codes */
> +enum feature_code {
> + /* External feature codes */
> + SOCINFO_FC_UNKNOWN = 0x0,
> + SOCINFO_FC_AA,
> + SOCINFO_FC_AB,
> + SOCINFO_FC_AC,
> + SOCINFO_FC_AD,
> + SOCINFO_FC_AE,
> + SOCINFO_FC_AF,
> + SOCINFO_FC_AG,
> + SOCINFO_FC_AH,
> + SOCINFO_FC_EXT_RESERVE,
> +};
> +
> +/* Internal feature codes */
> +/* Valid values: 0 <= n <= 0xf */
> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
> +#define SOCINFO_FC_INT_RESERVE   SOCINFO_FC_Yn(0x10)
> +
> +/* Product codes */
> +#define SOCINFO_PC_UNKNOWN   0
> +/* Valid values: 0 <= n <= 8, the rest is reserved */
> +#define SOCINFO_PCn(n)   (n + 1)
> +#define SOCINFO_PC_RESERVE   (BIT(31) - 1)

Please move these defines into the next patch.

> +
>  #endif
> 
> -- 
> 2.40.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/6] soc: qcom: smem: Add pcode/fcode getters

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 10:41:30AM +0200, Konrad Dybcio wrote:
> Introduce getters for SoC product and feature codes and export them.
> 
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/soc/qcom/smem.c   | 66 
> +++
>  include/linux/soc/qcom/smem.h |  2 ++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 7191fa0c087f..e89b4d26877a 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -795,6 +795,72 @@ int qcom_smem_get_soc_id(u32 *id)
>  }
>  EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
>  
> +/**
> + * qcom_smem_get_feature_code() - return the feature code
> + * @id:  On success, we return the feature code here.
> + *
> + * Look up the feature code identifier from SMEM and return it.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qcom_smem_get_feature_code(u32 *code)
> +{
> + struct socinfo *info;
> + u32 raw_code;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* This only makes sense for socinfo >= 16 */
> + if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
> + return -EINVAL;
> +
> + raw_code = __le32_to_cpu(info->feature_code);
> +
> + /* Ensure the value makes sense */
> + if (raw_code >= SOCINFO_FC_INT_RESERVE)
> + raw_code = SOCINFO_FC_UNKNOWN;
> +
> + *code = raw_code;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_get_feature_code);
> +
> +/**
> + * qcom_smem_get_product_code() - return the product code
> + * @id:  On success, we return the product code here.
> + *
> + * Look up feature code identifier from SMEM and return it.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qcom_smem_get_product_code(u32 *code)
> +{
> + struct socinfo *info;
> + u32 raw_code;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* This only makes sense for socinfo >= 16 */
> + if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
> + return -EINVAL;
> +
> + raw_code = __le32_to_cpu(info->pcode);
> +
> + /* Ensure the value makes sense */
> + if (raw_code >= SOCINFO_FC_INT_RESERVE)
> + raw_code = SOCINFO_FC_UNKNOWN;

This looks like a c from the previous function. Should we be comparing
the raw_code with a SOCINFO_PC_ constant?

> +
> + *code = raw_code;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_get_product_code);
> +
>  static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
>  {
>   struct smem_header *header;
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index a36a3b9d4929..aef8c9fc6c08 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -13,5 +13,7 @@ int qcom_smem_get_free_space(unsigned host);
>  phys_addr_t qcom_smem_virt_to_phys(void *p);
>  
>  int qcom_smem_get_soc_id(u32 *id);
> +int qcom_smem_get_feature_code(u32 *code);
> +int qcom_smem_get_product_code(u32 *code);
>  
>  #endif
> 
> -- 
> 2.40.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 05:17:14PM -0700, Abhinav Kumar wrote:
> From: Kuogee Hsieh 
> 
> In the external HPD case, hotplug event is delivered by pmic glink to DP 
> driver
> using drm_aux_hpd_bridge_notify().

There can be other drivers in front of the DP chain. For example,
altmode driver uses drm_connector_oob_hotplug_event() to deliver HPD
events.

> 
> The stacktrace showing the sequence of events is below:
> 
> dp_bridge_hpd_notify+0x18/0x70 [msm]
> drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
> drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
> drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
> drm_client_modeset_probe+0x240/0x1114 [drm]
> drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
> drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
> msm_fbdev_client_hotplug+0x24/0xd4 [msm]
> drm_client_dev_hotplug+0xd8/0x148 [drm]
> drm_kms_helper_connector_hotplug_event+0x30/0x3c [drm_kms_helper]
> drm_bridge_connector_handle_hpd+0x84/0x94 [drm_kms_helper]
> drm_bridge_connector_hpd_cb+0xc/0x14 [drm_kms_helper]
> drm_bridge_hpd_notify+0x38/0x50 [drm]
> drm_aux_hpd_bridge_notify+0x14/0x20 [aux_hpd_bridge]
> pmic_glink_altmode_worker+0xec/0x27c [pmic_glink_altmode]
> process_scheduled_works+0x17c/0x2cc
> worker_thread+0x2ac/0x2d0
> kthread+0xfc/0x120
> 
> There are three notifications delivered to DP driver for each notification 
> event.
> 
> 1) From the drm_aux_hpd_bridge_notify() itself as shown above
> 
> 2) From output_poll_execute() thread which arises due to
> drm_helper_probe_single_connector_modes() call of the above stacktrace
> as shown in more detail here.
> 
> dp_bridge_hpd_notify+0x18/0x70 [msm]
> drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
> drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
> drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
> drm_client_modeset_probe+0x240/0x1114 [drm]
> drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
> drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
> msm_fbdev_client_hotplug+0x24/0xd4 [msm]
> drm_client_dev_hotplug+0xd8/0x148 [drm]
> drm_kms_helper_hotplug_event+0x30/0x3c [drm_kms_helper]
> output_poll_execute+0xe0/0x210 [drm_kms_helper]
> 
> 3) From the DP driver as the dp_bridge_hpd_notify() callback today triggers
> the hpd_event_thread for connect and disconnect events respectively via below 
> stack
> 
> dp_bridge_hpd_notify+0x18/0x70 [msm]
> drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
> drm_helper_probe_detect_ctx+0x98/0x110 [drm_kms_helper]
> check_connector_changed+0x4c/0x20c [drm_kms_helper]
> drm_helper_hpd_irq_event+0x98/0x120 [drm_kms_helper]
> hpd_event_thread+0x478/0x5bc [msm]
> 
> We have to address why we end up with 3 events for every single event so 
> something
> is broken with how we work with the drm_bridge_connector.
> 
> But, the dp_bridge_hpd_notify() delivered from output_poll_execute() thread 
> will
> return the incorrect HPD status DP driver because the .detect() returns the 
> value
> of link_ready and not the HPD status currently.
> 
> And because the HPD event thread has not run yet and this results in the two 
> complementary
> events.
> 
> To fix this problem lets have dp_bridge_hpd_notify() call
> dp_hpd_plug_handle/unplug_handle() directly instead of going through the
> event thread.
> 
> Then the following .detect() called by 
> drm_kms_helper_connector_hotplug_event()
> will return correct value of HPD status since it uses the correct link_ready 
> value.
> 
> With this change, the HPD status delivered by both 
> drm_bridge_connector_hpd_notify()
> and drm_kms_helper_connector_hotplug_event() will match each other 
> consistently.

Please take a look at Documentation/process/submitting-patches.rst

With the commit message fixed, the change LGTM. Thanks a lot for
describing the call chains leading to this issue.

I must admit, initially I thought that the change should be rejected on
a basis of being a band-aid, but after studying the call graphs and the
locking within the DP driver, the change looks correct to me.

> 
> changes in v2:
>   - Fix the commit message to explain the scenario
>   - Fix the subject a little as well
> 
> Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
> Signed-off-by: Kuogee Hsieh 
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index d80f89581760..dfb10584ff97 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1665,7 +1665,8 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>   return;
>  
>   if (!dp_display->link_ready && status == connector_status_connected)
> - dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> + dp_hpd_plug_handle(dp, 0);
>   else if (dp_display->link_ready && status

[PATCH v2] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD

2024-04-05 Thread Abhinav Kumar

From: Kuogee Hsieh 

In the external HPD case, hotplug event is delivered by pmic glink to DP driver
using drm_aux_hpd_bridge_notify().

The stacktrace showing the sequence of events is below:

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_connector_hotplug_event+0x30/0x3c [drm_kms_helper]
drm_bridge_connector_handle_hpd+0x84/0x94 [drm_kms_helper]
drm_bridge_connector_hpd_cb+0xc/0x14 [drm_kms_helper]
drm_bridge_hpd_notify+0x38/0x50 [drm]
drm_aux_hpd_bridge_notify+0x14/0x20 [aux_hpd_bridge]
pmic_glink_altmode_worker+0xec/0x27c [pmic_glink_altmode]
process_scheduled_works+0x17c/0x2cc
worker_thread+0x2ac/0x2d0
kthread+0xfc/0x120

There are three notifications delivered to DP driver for each notification 
event.

1) From the drm_aux_hpd_bridge_notify() itself as shown above

2) From output_poll_execute() thread which arises due to
drm_helper_probe_single_connector_modes() call of the above stacktrace
as shown in more detail here.

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_hotplug_event+0x30/0x3c [drm_kms_helper]
output_poll_execute+0xe0/0x210 [drm_kms_helper]

3) From the DP driver as the dp_bridge_hpd_notify() callback today triggers
the hpd_event_thread for connect and disconnect events respectively via below 
stack

dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect_ctx+0x98/0x110 [drm_kms_helper]
check_connector_changed+0x4c/0x20c [drm_kms_helper]
drm_helper_hpd_irq_event+0x98/0x120 [drm_kms_helper]
hpd_event_thread+0x478/0x5bc [msm]

We have to address why we end up with 3 events for every single event so 
something
is broken with how we work with the drm_bridge_connector.

But, the dp_bridge_hpd_notify() delivered from output_poll_execute() thread will
return the incorrect HPD status DP driver because the .detect() returns the 
value
of link_ready and not the HPD status currently.

And because the HPD event thread has not run yet and this results in the two 
complementary
events.

To fix this problem lets have dp_bridge_hpd_notify() call
dp_hpd_plug_handle/unplug_handle() directly instead of going through the
event thread.

Then the following .detect() called by drm_kms_helper_connector_hotplug_event()
will return correct value of HPD status since it uses the correct link_ready 
value.

With this change, the HPD status delivered by both 
drm_bridge_connector_hpd_notify()
and drm_kms_helper_connector_hotplug_event() will match each other consistently.

changes in v2:
- Fix the commit message to explain the scenario
- Fix the subject a little as well

Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Signed-off-by: Kuogee Hsieh 
Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..dfb10584ff97 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,8 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
 
if (!dp_display->link_ready && status == connector_status_connected)
-   dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+   dp_hpd_plug_handle(dp, 0);
else if (dp_display->link_ready && status == 
connector_status_disconnected)
-   dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+   dp_hpd_unplug_handle(dp, 0);
+
 }
-- 
2.43.2

Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly

2024-04-05 Thread Abhinav Kumar





On 3/28/2024 10:47 PM, Abhinav Kumar wrote:



On 3/28/2024 8:23 PM, Dmitry Baryshkov wrote:
On Fri, 29 Mar 2024 at 04:16, Abhinav Kumar 
 wrote:




On 3/28/2024 5:10 PM, Dmitry Baryshkov wrote:
On Fri, 29 Mar 2024 at 01:42, Abhinav Kumar 
 wrote:




On 3/28/2024 3:50 PM, Dmitry Baryshkov wrote:
On Thu, 28 Mar 2024 at 23:21, Abhinav Kumar 
 wrote:




On 3/28/2024 1:58 PM, Stephen Boyd wrote:

Quoting Abhinav Kumar (2024-03-28 13:24:34)

+ Johan and Bjorn for FYI

On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:

For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under 
hpd_event_thread

context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are 
executed
under thread context already, there is no reason to hand over 
those

events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at 
dp_bridge_hpd_notify().


Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)



Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")


Is this a bug fix or an optimization? The commit text doesn't 
tell me.




I would say both.

optimization as it avoids the need to go through the hpd_event 
thread

processing.

bug fix because once you go through the hpd event thread 
processing it
exposes and often breaks the already fragile hpd handling state 
machine

which can be avoided in this case.


Please add a description for the particular issue that was observed
and how it is fixed by the patch.

Otherwise consider there to be an implicit NAK for all HPD-related
patches unless it is a series that moves link training to the enable
path and drops the HPD state machine completely.

I really mean it. We should stop beating a dead horse unless there is
a grave bug that must be fixed.



I think the commit message is explaining the issue well enough.

This was not fixing any issue we saw to explain you the exact scenario
of things which happened but this is just from code walkthrough.

Like kuogee wrote, hpd event thread was there so handle events coming
out of the hpd_isr for internal hpd cases. For the hpd_notify coming
from pmic_glink or any other extnernal hpd cases, there is no need to
put this through the hpd event thread because this will only make 
things

worse of exposing the race conditions of the state machine.

Moving link training to enable and removal of hpd event thread will be
worked on but delaying obvious things we can fix does not make sense.


  From the commit message this feels like an optimisation rather than a
fix. And granted the fragility of the HPD state machine, I'd prefer to
stay away from optimisations. As far as I understood from the history
of the last revert, we'd better make sure that HPD handling goes only
through the HPD event thread.



I think you are mixing the two. We tried to send the events through
DRM's hpd_notify which ended up in a bad way and btw, thats still not
resolved even though I have seen reports that things are fine with the
revert, we are consistently able to see us ending up in a disconnected
state with all the reverts and fixes in our x1e80100 DP setup.

I plan to investigate that issue properly in the next week and try to
make some sense of it all.

In fact, this patch is removing one more user of the hpd event thread
which is the direction in which we all want to head towards.


As I stated earlier, from my point of view it doesn't make sense to
rework the HPD thread in small steps.


On whether this is an optimization or a bug fix. I think by avoiding hpd
event thread (which should have never been used for hpd_notify updates,
hence a bug) we are avoiding the possibility of more race conditions.


I think that the HPD event thread serializes handling of events, so
avoiding it increases the possibility of a race condition.



So, this has my R-b and it holds. Upto you.


I'd wait for a proper description of the issue that was observed and
how it is solved by this patch.



This was a code walkthrough fix as I wrote a few times. If there no 
merit in pushing this, lets ignore it and stop discussing.




Ok, so after we debugged the HPD issue on we have found the issue and 
why actually this change will help. I am going to post a V2 with more 
details on the commit text. We can discuss after that.

Re: [PATCH 2/6] soc: qcom: smem: Add pcode/fcode getters

2024-04-05 Thread kernel test robot

Hi Konrad,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2b3d5988ae2cb5cd945ddbc653f0a71706231fdd]

url:
https://github.com/intel-lab-lkp/linux/commits/Konrad-Dybcio/soc-qcom-Move-some-socinfo-defines-to-the-header-expand-them/20240405-164231
base:   2b3d5988ae2cb5cd945ddbc653f0a71706231fdd
patch link:
https://lore.kernel.org/r/20240405-topic-smem_speedbin-v1-2-ce2b864251b1%40linaro.org
patch subject: [PATCH 2/6] soc: qcom: smem: Add pcode/fcode getters
config: arm-defconfig 
(https://download.01.org/0day-ci/archive/20240406/202404060648.dojoyusf-...@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git 
f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): 
(https://download.01.org/0day-ci/archive/20240406/202404060648.dojoyusf-...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Closes: 
https://lore.kernel.org/oe-kbuild-all/202404060648.dojoyusf-...@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/soc/qcom/smem.c:807: warning: Function parameter or struct member 
>> 'code' not described in 'qcom_smem_get_feature_code'
>> drivers/soc/qcom/smem.c:807: warning: Excess function parameter 'id' 
>> description in 'qcom_smem_get_feature_code'
>> drivers/soc/qcom/smem.c:840: warning: Function parameter or struct member 
>> 'code' not described in 'qcom_smem_get_product_code'
>> drivers/soc/qcom/smem.c:840: warning: Excess function parameter 'id' 
>> description in 'qcom_smem_get_product_code'


vim +807 drivers/soc/qcom/smem.c

   797  
   798  /**
   799   * qcom_smem_get_feature_code() - return the feature code
   800   * @id: On success, we return the feature code here.
   801   *
   802   * Look up the feature code identifier from SMEM and return it.
   803   *
   804   * Return: 0 on success, negative errno on failure.
   805   */
   806  int qcom_smem_get_feature_code(u32 *code)
 > 807  {
   808  struct socinfo *info;
   809  u32 raw_code;
   810  
   811  info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, 
NULL);
   812  if (IS_ERR(info))
   813  return PTR_ERR(info);
   814  
   815  /* This only makes sense for socinfo >= 16 */
   816  if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
   817  return -EINVAL;
   818  
   819  raw_code = __le32_to_cpu(info->feature_code);
   820  
   821  /* Ensure the value makes sense */
   822  if (raw_code >= SOCINFO_FC_INT_RESERVE)
   823  raw_code = SOCINFO_FC_UNKNOWN;
   824  
   825  *code = raw_code;
   826  
   827  return 0;
   828  }
   829  EXPORT_SYMBOL_GPL(qcom_smem_get_feature_code);
   830  
   831  /**
   832   * qcom_smem_get_product_code() - return the product code
   833   * @id: On success, we return the product code here.
   834   *
   835   * Look up feature code identifier from SMEM and return it.
   836   *
   837   * Return: 0 on success, negative errno on failure.
   838   */
   839  int qcom_smem_get_product_code(u32 *code)
 > 840  {
   841  struct socinfo *info;
   842  u32 raw_code;
   843  
   844  info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, 
NULL);
   845  if (IS_ERR(info))
   846  return PTR_ERR(info);
   847  
   848  /* This only makes sense for socinfo >= 16 */
   849  if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
   850  return -EINVAL;
   851  
   852  raw_code = __le32_to_cpu(info->pcode);
   853  
   854  /* Ensure the value makes sense */
   855  if (raw_code >= SOCINFO_FC_INT_RESERVE)
   856  raw_code = SOCINFO_FC_UNKNOWN;
   857  
   858  *code = raw_code;
   859  
   860  return 0;
   861  }
   862  EXPORT_SYMBOL_GPL(qcom_smem_get_product_code);
   863  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: (subset) [PATCH v3 0/4] arm64: dts: fix several display-related schema warnings

2024-04-05 Thread Abhinav Kumar



On Tue, 02 Apr 2024 05:57:14 +0300, Dmitry Baryshkov wrote:
> Fix several warnings produced by the display nodes.
> 
> Please excuse me for the spam for sending v3 soon after v2.
> 
> 

Applied, thanks!

[1/4] dt-bindings: display/msm: sm8150-mdss: add DP node
  https://gitlab.freedesktop.org/drm/msm/-/commit/be1b7acb9291

Best regards,
-- 
Abhinav Kumar

Re: [PATCH] drm/msm/dp: fix typo in dp_display_handle_port_status_changed()

2024-04-05 Thread Abhinav Kumar



On Wed, 06 Mar 2024 11:35:15 -0800, Abhinav Kumar wrote:
> Fix the typo in the name of dp_display_handle_port_status_changed().
> 
> 

Applied, thanks!

[1/1] drm/msm/dp: fix typo in dp_display_handle_port_status_changed()
  (no commit info)
  https://gitlab.freedesktop.org/drm/msm/-/commit/cd49cca222bc
Best regards,
-- 
Abhinav Kumar

Re: [PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible

2024-04-05 Thread Abhinav Kumar



On Sat, 30 Mar 2024 05:53:22 +0200, Dmitry Baryshkov wrote:
> There is little point in using %ps to print a value known to be NULL. On
> the other hand it makes sense to print the callback symbol in the
> 'invalid IRQ' message. Correct those two error messages to make more
> sense.
> 
> 

Applied, thanks!

[1/1] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more 
sensible
  https://gitlab.freedesktop.org/drm/msm/-/commit/8844f467d6a5

Best regards,
-- 
Abhinav Kumar

Re: [PATCH v3] drm/msm/dp: assign correct DP controller ID to x1e80100 interface table

2024-04-05 Thread Abhinav Kumar



On Fri, 29 Mar 2024 12:46:26 -0700, Kuogee Hsieh wrote:
> At current x1e80100 interface table, interface #3 is wrongly
> connected to DP controller #0 and interface #4 wrongly connected
> to DP controller #2. Fix this problem by connect Interface #3 to
> DP controller #0 and interface #4 connect to DP controller #1.
> Also add interface #6, #7 and #8 connections to DP controller to
> complete x1e80100 interface table.
> 
> [...]

Applied, thanks!

[1/1] drm/msm/dp: assign correct DP controller ID to x1e80100 interface table
  https://gitlab.freedesktop.org/drm/msm/-/commit/ee15c8bf5d77

Best regards,
-- 
Abhinav Kumar

Re: (subset) [PATCH v3 0/5] drm/msm/dpu: rework debugfs interface of dpu_core_perf

2024-04-05 Thread Abhinav Kumar



On Thu, 14 Mar 2024 03:10:40 +0200, Dmitry Baryshkov wrote:
> Bring back a set of patches extracted from [1] per Abhinav's suggestion.
> 
> Rework debugging overrides for the bandwidth and clock settings. Instead
> of specifying the 'mode' and some values, allow one to set the affected
> value directly.
> 
> [1] https://patchwork.freedesktop.org/series/119552/#rev2
> 
> [...]

Applied, thanks!

[1/5] drm/msm/dpu: don't allow overriding data from catalog
  https://gitlab.freedesktop.org/drm/msm/-/commit/4f3b77ae5ff5

Best regards,
-- 
Abhinav Kumar

Re: [PATCH] drm/msm: Add newlines to some debug prints

2024-04-05 Thread Abhinav Kumar



On Mon, 25 Mar 2024 14:08:09 -0700, Stephen Boyd wrote:
> These debug prints are missing newlines, leading to multiple messages
> being printed on one line and hard to read logs. Add newlines to have
> the debug prints on separate lines. The DBG macro used to add a newline,
> but I missed that while migrating to drm_dbg wrappers.
> 
> 

Applied, thanks!

[1/1] drm/msm: Add newlines to some debug prints
  https://gitlab.freedesktop.org/drm/msm/-/commit/c588f7d67044

Best regards,
-- 
Abhinav Kumar

Re: [PATCH 0/2] drm/msm/dp: fix runtime PM leaks on hotplug

2024-04-05 Thread Abhinav Kumar



On Wed, 13 Mar 2024 17:43:04 +0100, Johan Hovold wrote:
> As I've reported elsewhere, I've been hitting runtime PM usage count
> leaks when investigated a DisplayPort hotplug regression on the Lenovo
> ThinkPad X13s. [1]
> 
> This series addresses two obvious leaks on disconnect and on connect
> failures, respectively.
> 
> [...]

Applied, thanks!

[1/2] drm/msm/dp: fix runtime PM leak on disconnect
  https://gitlab.freedesktop.org/drm/msm/-/commit/0640f47b7426
[2/2] drm/msm/dp: fix runtime PM leak on connect failure
  https://gitlab.freedesktop.org/drm/msm/-/commit/e86750b01a15  

Best regards,
-- 
Abhinav Kumar

Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback

2024-04-05 Thread Abhinav Kumar





On 4/5/2024 11:19 AM, Dmitry Baryshkov wrote:

On Fri, 5 Apr 2024 at 21:17, Abhinav Kumar  wrote:




On 4/5/2024 11:16 AM, Dmitry Baryshkov wrote:

On Fri, 5 Apr 2024 at 20:20, Abhinav Kumar  wrote:




On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:

All the bridges that are being used with the MSM DSI hosts have been
converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback
code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the
downstream bridges.

Signed-off-by: Dmitry Baryshkov 
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 36 
+++
1 file changed, 11 insertions(+), 25 deletions(-)



There are the bridges I checked by looking at the dts:

1) lontium,lt9611
2) lontium,lt9611uxc
3) adi,adv7533
4) ti,sn65dsi86

Are there any more?

If not, this LGTM

Reviewed-by: Abhinav Kumar 


  From your message it looks more like Tested-by rather than just Reviewed-by



No, I only cross-checked the dts.

So, its only Reviewed-by :)

But I wanted to list down all the bridges


Then I'd also nominate the panel bridge to the list of bridges for
cross-checking. It is created automatically when we request a bridge,
but DT has only a panel.



Yes, that one is fine too

58 static int panel_bridge_attach(struct drm_bridge *bridge,
59 enum drm_bridge_attach_flags flags)
60 {
61  struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
62  struct drm_connector *connector = _bridge->connector;
63  int ret;
64
65  if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
66  return 0;
67

Re: [PATCH] drm/msm: remove an unused-but-set variable

2024-04-05 Thread Dmitry Baryshkov

On Fri, 5 Apr 2024 at 18:59, Arnd Bergmann  wrote:
>
> From: Arnd Bergmann 
>
> The modification to a6xx_get_shader_block() had no effect other
> than causing a warning:
>
> drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable 'out' set 
> but not used [-Werror,-Wunused-but-set-variable]
> u64 out = dumper->iova + A6XX_CD_DATA_OFFSET;
>
> Revert this part of the previous patch.
>
> Fixes: 64d6255650d4 ("drm/msm: More fully implement devcoredump for a7xx")
> Signed-off-by: Arnd Bergmann 

Unfortunately this fix is not correct. The proper patch is present at
https://patchwork.freedesktop.org/patch/584955/?series=131663=1

> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index 1f5245fc2cdc..d4e1ebfcb021 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -840,7 +840,6 @@ static void a6xx_get_shader_block(struct msm_gpu *gpu,
> struct a6xx_crashdumper *dumper)
>  {
> u64 *in = dumper->ptr;
> -   u64 out = dumper->iova + A6XX_CD_DATA_OFFSET;
> size_t datasize = block->size * A6XX_NUM_SHADER_BANKS * sizeof(u32);
> int i;
>
> @@ -853,8 +852,6 @@ static void a6xx_get_shader_block(struct msm_gpu *gpu,
>
> in += CRASHDUMP_READ(in, REG_A6XX_HLSQ_DBG_AHB_READ_APERTURE,
> block->size, dumper->iova + A6XX_CD_DATA_OFFSET);
> -
> -   out += block->size * sizeof(u32);
> }
>
> CRASHDUMP_FINI(in);
> --
> 2.39.2
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback

2024-04-05 Thread Dmitry Baryshkov

On Fri, 5 Apr 2024 at 21:17, Abhinav Kumar  wrote:
>
>
>
> On 4/5/2024 11:16 AM, Dmitry Baryshkov wrote:
> > On Fri, 5 Apr 2024 at 20:20, Abhinav Kumar  
> > wrote:
> >>
> >>
> >>
> >> On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:
> >>> All the bridges that are being used with the MSM DSI hosts have been
> >>> converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback
> >>> code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the
> >>> downstream bridges.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov 
> >>> ---
> >>>drivers/gpu/drm/msm/dsi/dsi_manager.c | 36 
> >>> +++
> >>>1 file changed, 11 insertions(+), 25 deletions(-)
> >>>
> >>
> >> There are the bridges I checked by looking at the dts:
> >>
> >> 1) lontium,lt9611
> >> 2) lontium,lt9611uxc
> >> 3) adi,adv7533
> >> 4) ti,sn65dsi86
> >>
> >> Are there any more?
> >>
> >> If not, this LGTM
> >>
> >> Reviewed-by: Abhinav Kumar 
> >
> >  From your message it looks more like Tested-by rather than just Reviewed-by
> >
>
> No, I only cross-checked the dts.
>
> So, its only Reviewed-by :)
>
> But I wanted to list down all the bridges

Then I'd also nominate the panel bridge to the list of bridges for
cross-checking. It is created automatically when we request a bridge,
but DT has only a panel.

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback

2024-04-05 Thread Abhinav Kumar





On 4/5/2024 11:16 AM, Dmitry Baryshkov wrote:

On Fri, 5 Apr 2024 at 20:20, Abhinav Kumar  wrote:




On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:

All the bridges that are being used with the MSM DSI hosts have been
converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback
code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the
downstream bridges.

Signed-off-by: Dmitry Baryshkov 
---
   drivers/gpu/drm/msm/dsi/dsi_manager.c | 36 
+++
   1 file changed, 11 insertions(+), 25 deletions(-)



There are the bridges I checked by looking at the dts:

1) lontium,lt9611
2) lontium,lt9611uxc
3) adi,adv7533
4) ti,sn65dsi86

Are there any more?

If not, this LGTM

Reviewed-by: Abhinav Kumar 


 From your message it looks more like Tested-by rather than just Reviewed-by



No, I only cross-checked the dts.

So, its only Reviewed-by :)

But I wanted to list down all the bridges

Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback

2024-04-05 Thread Dmitry Baryshkov

On Fri, 5 Apr 2024 at 20:20, Abhinav Kumar  wrote:
>
>
>
> On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:
> > All the bridges that are being used with the MSM DSI hosts have been
> > converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback
> > code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the
> > downstream bridges.
> >
> > Signed-off-by: Dmitry Baryshkov 
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 36 
> > +++
> >   1 file changed, 11 insertions(+), 25 deletions(-)
> >
>
> There are the bridges I checked by looking at the dts:
>
> 1) lontium,lt9611
> 2) lontium,lt9611uxc
> 3) adi,adv7533
> 4) ti,sn65dsi86
>
> Are there any more?
>
> If not, this LGTM
>
> Reviewed-by: Abhinav Kumar 

>From your message it looks more like Tested-by rather than just Reviewed-by

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dsi: move next bridge acquisition to dsi_bind

2024-04-05 Thread Dmitry Baryshkov

On Fri, 5 Apr 2024 at 20:35, Abhinav Kumar  wrote:
>
>
>
> On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:
> > Currently the MSM DSI driver looks for the next bridge during
> > msm_dsi_modeset_init(). If the bridge is not registered at that point,
> > this might result in -EPROBE_DEFER, which can be problematic that late
> > during the device probe process. Move next bridge acquisition to the
> > dsi_bind state so that probe deferral is returned as early as possible.
> >
>
> But msm_dsi_modeset)init() is also called during msm_drm_bind() only.
>
> What issue are you suggesting will be fixed by moving this from
> msm_drm_bind() to dsi_bind()?

The goal is to return as early as possible as not not cause
probe-deferral loops. See commit fbc35b45f9f6 ("Add documentation on
meaning of -EPROBE_DEFER"). It discusses returning from probe() but
the same logic applies to bind().

> > Signed-off-by: Dmitry Baryshkov 
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi.c | 16 
> >   drivers/gpu/drm/msm/dsi/dsi.h |  2 ++
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c |  8 +---
> >   3 files changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> > index 37c4c07005fe..38f10f7a10d3 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> > @@ -120,6 +120,22 @@ static int dsi_bind(struct device *dev, struct device 
> > *master, void *data)
> >   struct msm_drm_private *priv = dev_get_drvdata(master);
> >   struct msm_dsi *msm_dsi = dev_get_drvdata(dev);
> >
> > + /*
> > +  * Next bridge doesn't exist for the secondary DSI host in a bonded
> > +  * pair.
> > +  */
> > + if (!msm_dsi_is_bonded_dsi(msm_dsi) ||
> > + msm_dsi_is_master_dsi(msm_dsi)) {
> > + struct drm_bridge *ext_bridge;
> > +
> > + ext_bridge = devm_drm_of_get_bridge(_dsi->pdev->dev,
> > + 
> > msm_dsi->pdev->dev.of_node, 1, 0);
> > + if (IS_ERR(ext_bridge))
> > + return PTR_ERR(ext_bridge);
> > +
> > + msm_dsi->next_bridge = ext_bridge;
> > + }
> > +
> >   priv->dsi[msm_dsi->id] = msm_dsi;
> >
> >   return 0;
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> > index 2ad9a842c678..0adef65be1de 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> > @@ -38,6 +38,8 @@ struct msm_dsi {
> >   struct mipi_dsi_host *host;
> >   struct msm_dsi_phy *phy;
> >
> > + struct drm_bridge *next_bridge;
> > +
> >   struct device *phy_dev;
> >   bool phy_enabled;
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> > b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index a7c7f85b73e4..b7c52b14c790 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -464,18 +464,12 @@ int msm_dsi_manager_ext_bridge_init(u8 id, struct 
> > drm_bridge *int_bridge)
> >   struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >   struct drm_device *dev = msm_dsi->dev;
> >   struct drm_encoder *encoder;
> > - struct drm_bridge *ext_bridge;
> >   struct drm_connector *connector;
> >   int ret;
> >
> > - ext_bridge = devm_drm_of_get_bridge(_dsi->pdev->dev,
> > - msm_dsi->pdev->dev.of_node, 1, 0);
> > - if (IS_ERR(ext_bridge))
> > - return PTR_ERR(ext_bridge);
> > -
> >   encoder = int_bridge->encoder;
> >
> > - ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
> > + ret = drm_bridge_attach(encoder, msm_dsi->next_bridge, int_bridge,
> >   DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >   if (ret)
> >   return ret;
> >



-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dsi: move next bridge acquisition to dsi_bind

2024-04-05 Thread Abhinav Kumar





On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:

Currently the MSM DSI driver looks for the next bridge during
msm_dsi_modeset_init(). If the bridge is not registered at that point,
this might result in -EPROBE_DEFER, which can be problematic that late
during the device probe process. Move next bridge acquisition to the
dsi_bind state so that probe deferral is returned as early as possible.



But msm_dsi_modeset)init() is also called during msm_drm_bind() only.

What issue are you suggesting will be fixed by moving this from 
msm_drm_bind() to dsi_bind()?



Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/dsi/dsi.c | 16 
  drivers/gpu/drm/msm/dsi/dsi.h |  2 ++
  drivers/gpu/drm/msm/dsi/dsi_manager.c |  8 +---
  3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 37c4c07005fe..38f10f7a10d3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -120,6 +120,22 @@ static int dsi_bind(struct device *dev, struct device 
*master, void *data)
struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_dsi *msm_dsi = dev_get_drvdata(dev);
  
+	/*

+* Next bridge doesn't exist for the secondary DSI host in a bonded
+* pair.
+*/
+   if (!msm_dsi_is_bonded_dsi(msm_dsi) ||
+   msm_dsi_is_master_dsi(msm_dsi)) {
+   struct drm_bridge *ext_bridge;
+
+   ext_bridge = devm_drm_of_get_bridge(_dsi->pdev->dev,
+   msm_dsi->pdev->dev.of_node, 
1, 0);
+   if (IS_ERR(ext_bridge))
+   return PTR_ERR(ext_bridge);
+
+   msm_dsi->next_bridge = ext_bridge;
+   }
+
priv->dsi[msm_dsi->id] = msm_dsi;
  
  	return 0;

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 2ad9a842c678..0adef65be1de 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -38,6 +38,8 @@ struct msm_dsi {
struct mipi_dsi_host *host;
struct msm_dsi_phy *phy;
  
+	struct drm_bridge *next_bridge;

+
struct device *phy_dev;
bool phy_enabled;
  
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c

index a7c7f85b73e4..b7c52b14c790 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -464,18 +464,12 @@ int msm_dsi_manager_ext_bridge_init(u8 id, struct 
drm_bridge *int_bridge)
struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
struct drm_device *dev = msm_dsi->dev;
struct drm_encoder *encoder;
-   struct drm_bridge *ext_bridge;
struct drm_connector *connector;
int ret;
  
-	ext_bridge = devm_drm_of_get_bridge(_dsi->pdev->dev,

-   msm_dsi->pdev->dev.of_node, 1, 0);
-   if (IS_ERR(ext_bridge))
-   return PTR_ERR(ext_bridge);
-
encoder = int_bridge->encoder;
  
-	ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,

+   ret = drm_bridge_attach(encoder, msm_dsi->next_bridge, int_bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (ret)
return ret;

Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback

2024-04-05 Thread Abhinav Kumar





On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:

All the bridges that are being used with the MSM DSI hosts have been
converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback
code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the
downstream bridges.

Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/dsi/dsi_manager.c | 36 +++
  1 file changed, 11 insertions(+), 25 deletions(-)



There are the bridges I checked by looking at the dts:

1) lontium,lt9611
2) lontium,lt9611uxc
3) adi,adv7533
4) ti,sn65dsi86

Are there any more?

If not, this LGTM

Reviewed-by: Abhinav Kumar

Re: [PATCH] drm: ci: fix the xfails for apq8016

2024-04-05 Thread Dmitry Baryshkov

On Mon, Apr 01, 2024 at 01:48:58PM -0700, Abhinav Kumar wrote:
> After IGT migrating to dynamic sub-tests, the pipe prefixes
> in the expected fails list are incorrect. Lets drop those
> to accurately match the expected fails.
> 
> In addition, update the xfails list to match the current passing
> list. This should have ideally failed in the CI run because some
> tests were marked as fail even though they passed but due to the
> mismatch in test names, the matching didn't correctly work and was
> resulting in those failures not being seen.
> 
> Here is the passing pipeline for apq8016 with this change:
> 
> https://gitlab.freedesktop.org/drm/msm/-/jobs/57050562
> 
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt | 13 +
>  1 file changed, 1 insertion(+), 12 deletions(-)

Reviewed-by: Dmitry Baryshkov 

-- 
With best wishes
Dmitry

Re: [PATCH] drm/msm: convert all pixel format logging to use %p4cc

2024-04-05 Thread Dmitry Baryshkov

On Fri, Apr 05, 2024 at 12:29:07PM +0300, Jani Nikula wrote:
> Logging u32 pixel formats using %4.4s format string with a pointer to
> the u32 is somewhat questionable, as well as dependent on byte
> order. There's a kernel extension format specifier %p4cc to format 4cc
> codes. Use it across the board in msm for pixel format logging.
> 
> This should also fix the reported build warning:
> 
>   include/drm/drm_print.h:536:35: warning: '%4.4s' directive argument is
>   null [-Wformat-overflow=]
> 
> Reported-by: Aishwarya TCV 
> Closes: https://lore.kernel.org/r/2ac758ce-a196-4e89-a397-488ba3101...@arm.com
> Signed-off-by: Jani Nikula 
> 
> ---
> 
> Tip: 'git show --color-words -w' might be the easiest way to review.
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  8 +++
>  .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c   |  4 ++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 +--
>  drivers/gpu/drm/msm/msm_fb.c  | 10 
>  5 files changed, 24 insertions(+), 24 deletions(-)

Reviewed-by: Dmitry Baryshkov 
-- 
With best wishes
Dmitry

[PATCH] drm/msm: remove an unused-but-set variable

2024-04-05 Thread Arnd Bergmann

From: Arnd Bergmann 

The modification to a6xx_get_shader_block() had no effect other
than causing a warning:

drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable 'out' set 
but not used [-Werror,-Wunused-but-set-variable]
u64 out = dumper->iova + A6XX_CD_DATA_OFFSET;

Revert this part of the previous patch.

Fixes: 64d6255650d4 ("drm/msm: More fully implement devcoredump for a7xx")
Signed-off-by: Arnd Bergmann 
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 1f5245fc2cdc..d4e1ebfcb021 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -840,7 +840,6 @@ static void a6xx_get_shader_block(struct msm_gpu *gpu,
struct a6xx_crashdumper *dumper)
 {
u64 *in = dumper->ptr;
-   u64 out = dumper->iova + A6XX_CD_DATA_OFFSET;
size_t datasize = block->size * A6XX_NUM_SHADER_BANKS * sizeof(u32);
int i;
 
@@ -853,8 +852,6 @@ static void a6xx_get_shader_block(struct msm_gpu *gpu,
 
in += CRASHDUMP_READ(in, REG_A6XX_HLSQ_DBG_AHB_READ_APERTURE,
block->size, dumper->iova + A6XX_CD_DATA_OFFSET);
-
-   out += block->size * sizeof(u32);
}
 
CRASHDUMP_FINI(in);
-- 
2.39.2

Re: [linux-next:master] BUILD REGRESSION 8568bb2ccc278f344e6ac44af6ed010a90aa88dc

2024-04-05 Thread Alexei Starovoitov

On Fri, Apr 5, 2024 at 8:37 AM kernel test robot  wrote:
>
> tree/branch: 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> branch HEAD: 8568bb2ccc278f344e6ac44af6ed010a90aa88dc  Add linux-next 
> specific files for 20240405
>
> Error/Warning reports:
>
> https://lore.kernel.org/oe-kbuild-all/202404051333.7und7ppw-...@intel.com
> https://lore.kernel.org/oe-kbuild-all/202404051423.eiaxlwhx-...@intel.com
> https://lore.kernel.org/oe-kbuild-all/202404051659.aawukguq-...@intel.com
> https://lore.kernel.org/oe-kbuild-all/202404052022.cwf2ilbp-...@intel.com
>
> Error/Warning: (recently discovered and may have been fixed)
>
> aarch64-linux-ld: kernel/bpf/verifier.c:20223:(.text+0xdbb4): undefined 
> reference to `__SCK__perf_snapshot_branch_stack'
> aarch64-linux-ld: verifier.c:(.text+0x17c3c): undefined reference to 
> `__SCK__perf_snapshot_branch_stack'
> drivers/i2c/busses/i2c-i801.c:1407:(.text+0x1d2ef4a): undefined reference to 
> `i2c_root_adapter'
> kernel/bpf/verifier.c:20223:(.text+0xdba4): dangerous relocation: unsupported 
> relocation
> loongarch64-linux-ld: kernel/bpf/verifier.c:20223:(.text+0xa818): undefined 
> reference to `__SCK__perf_snapshot_branch_stack'
> loongarch64-linux-ld: verifier.c:(.text+0xa964): undefined reference to 
> `__SCK__perf_snapshot_branch_stack'
> mips64el-linux-ld: verifier.c:(.text.do_misc_fixups+0xd9c): undefined 
> reference to `__SCK__perf_snapshot_branch_stack'
> riscv32-linux-ld: section .data LMA [00369000,00907967] overlaps section 
> .text LMA [0007899c,01a6a6af]
> s390-linux-ld: verifier.c:(.text+0x13038): undefined reference to 
> `__SCK__perf_snapshot_branch_stack'
> verifier.c:(.text+0x17c14): relocation truncated to fit: 
> R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol 
> `__SCK__perf_snapshot_branch_stack'
> verifier.c:(.text+0xa960): undefined reference to 
> `__SCK__perf_snapshot_branch_stack'
> verifier.c:(.text+0xadd0): dangerous relocation: unsupported relocation
> verifier.c:(.text.do_misc_fixups+0xd98): undefined reference to 
> `__SCK__perf_snapshot_branch_stack'

Fixed in bpf-next with commit:
https://lore.kernel.org/all/20240405142637.577046-1-a...@kernel.org/

[linux-next:master] BUILD REGRESSION 8568bb2ccc278f344e6ac44af6ed010a90aa88dc

2024-04-05 Thread kernel test robot

tree/branch: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
branch HEAD: 8568bb2ccc278f344e6ac44af6ed010a90aa88dc  Add linux-next specific 
files for 20240405

Error/Warning reports:

https://lore.kernel.org/oe-kbuild-all/202404051333.7und7ppw-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202404051423.eiaxlwhx-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202404051659.aawukguq-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202404052022.cwf2ilbp-...@intel.com

Error/Warning: (recently discovered and may have been fixed)

aarch64-linux-ld: kernel/bpf/verifier.c:20223:(.text+0xdbb4): undefined 
reference to `__SCK__perf_snapshot_branch_stack'
aarch64-linux-ld: verifier.c:(.text+0x17c3c): undefined reference to 
`__SCK__perf_snapshot_branch_stack'
drivers/i2c/busses/i2c-i801.c:1407:(.text+0x1d2ef4a): undefined reference to 
`i2c_root_adapter'
kernel/bpf/verifier.c:20223:(.text+0xdba4): dangerous relocation: unsupported 
relocation
loongarch64-linux-ld: kernel/bpf/verifier.c:20223:(.text+0xa818): undefined 
reference to `__SCK__perf_snapshot_branch_stack'
loongarch64-linux-ld: verifier.c:(.text+0xa964): undefined reference to 
`__SCK__perf_snapshot_branch_stack'
mips64el-linux-ld: verifier.c:(.text.do_misc_fixups+0xd9c): undefined reference 
to `__SCK__perf_snapshot_branch_stack'
riscv32-linux-ld: section .data LMA [00369000,00907967] overlaps section .text 
LMA [0007899c,01a6a6af]
s390-linux-ld: verifier.c:(.text+0x13038): undefined reference to 
`__SCK__perf_snapshot_branch_stack'
verifier.c:(.text+0x17c14): relocation truncated to fit: 
R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol 
`__SCK__perf_snapshot_branch_stack'
verifier.c:(.text+0xa960): undefined reference to 
`__SCK__perf_snapshot_branch_stack'
verifier.c:(.text+0xadd0): dangerous relocation: unsupported relocation
verifier.c:(.text.do_misc_fixups+0xd98): undefined reference to 
`__SCK__perf_snapshot_branch_stack'

Unverified Error/Warning (likely false positive, please contact us if 
interested):

lib/alloc_tag.c:142 alloc_tag_init() warn: passing zero to 'PTR_ERR'

Error/Warning ids grouped by kconfigs:

gcc_recent_errors
|-- alpha-allyesconfig
|   |-- 
drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and
|   `-- 
drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size
|-- alpha-randconfig-r123-20240405
|   `-- 
kernel-bpf-verifier.c:sparse:sparse:cast-truncates-bits-from-constant-value-(fffc-becomes-)
|-- arm-allmodconfig
|   |-- 
drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and
|   `-- 
drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size
|-- arm-allyesconfig
|   |-- 
drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and
|   `-- 
drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size
|-- arm-hisi_defconfig
|   |-- 
drm_dp_mst_topology.c:(.text):undefined-reference-to-__drm_atomic_helper_private_obj_duplicate_state
|   `-- 
drm_dp_mst_topology.c:(.text):undefined-reference-to-drm_kms_helper_hotplug_event
|-- arm64-randconfig-c041-20221104
|   |-- 
aarch64-linux-ld:kernel-bpf-verifier.c:(.text):undefined-reference-to-__SCK__perf_snapshot_branch_stack
|   `-- 
kernel-bpf-verifier.c:(.text):dangerous-relocation:unsupported-relocation
|-- arm64-randconfig-r013-20230703
|   |-- 
aarch64-linux-ld:verifier.c:(.text):undefined-reference-to-__SCK__perf_snapshot_branch_stack
|   `-- 
verifier.c:(.text):relocation-truncated-to-fit:R_AARCH64_ADR_PREL_PG_HI21-against-undefined-symbol-__SCK__perf_snapshot_branch_stack
|-- arm64-randconfig-r032-20220702
|   `-- verifier.c:(.text):dangerous-relocation:unsupported-relocation
|-- csky-allmodconfig
|   |-- 
drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and
|   `-- 
drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size
|-- csky-allyesconfig
|   |-- 
drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and
|   `-- 
drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size
|-- loongarch-allmodconfig
|   |-- 
drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and
|   `-- 
drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size
|-- loongarch

[PATCH] drm/msm: convert all pixel format logging to use %p4cc

2024-04-05 Thread Jani Nikula

Logging u32 pixel formats using %4.4s format string with a pointer to
the u32 is somewhat questionable, as well as dependent on byte
order. There's a kernel extension format specifier %p4cc to format 4cc
codes. Use it across the board in msm for pixel format logging.

This should also fix the reported build warning:

  include/drm/drm_print.h:536:35: warning: '%4.4s' directive argument is
  null [-Wformat-overflow=]

Reported-by: Aishwarya TCV 
Closes: https://lore.kernel.org/r/2ac758ce-a196-4e89-a397-488ba3101...@arm.com
Signed-off-by: Jani Nikula 

---

Tip: 'git show --color-words -w' might be the easiest way to review.
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  8 +++
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c   |  4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 +--
 drivers/gpu/drm/msm/msm_fb.c  | 10 
 5 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9a14d2232e4a..aa1e68379d9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2203,8 +2203,8 @@ void dpu_encoder_helper_phys_setup_cdm(struct 
dpu_encoder_phys *phys_enc,
return;
 
if (!DPU_FORMAT_IS_YUV(dpu_fmt)) {
-   DPU_DEBUG("[enc:%d] cdm_disable fmt:%x\n", 
DRMID(phys_enc->parent),
- dpu_fmt->base.pixel_format);
+   DPU_DEBUG("[enc:%d] cdm_disable fmt:%p4cc\n", 
DRMID(phys_enc->parent),
+ _fmt->base.pixel_format);
if (hw_cdm->ops.bind_pingpong_blk)
hw_cdm->ops.bind_pingpong_blk(hw_cdm, PINGPONG_NONE);
 
@@ -2244,9 +2244,9 @@ void dpu_encoder_helper_phys_setup_cdm(struct 
dpu_encoder_phys *phys_enc,
break;
}
 
-   DPU_DEBUG("[enc:%d] cdm_enable:%d,%d,%X,%d,%d,%d,%d]\n",
+   DPU_DEBUG("[enc:%d] cdm_enable:%d,%d,%p4cc,%d,%d,%d,%d]\n",
  DRMID(phys_enc->parent), cdm_cfg->output_width,
- cdm_cfg->output_height, 
cdm_cfg->output_fmt->base.pixel_format,
+ cdm_cfg->output_height, 
_cfg->output_fmt->base.pixel_format,
  cdm_cfg->output_type, cdm_cfg->output_bit_depth,
  cdm_cfg->h_cdwn_type, cdm_cfg->v_cdwn_type);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 1924a2b28e53..9dbb8ddcddec 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -580,7 +580,7 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct 
dpu_encoder_phys *phys_enc
format->pixel_format, job->fb->modifier);
if (!wb_cfg->dest.format) {
/* this error should be detected during atomic_check */
-   DPU_ERROR("failed to get format %x\n", format->pixel_format);
+   DPU_ERROR("failed to get format %p4cc\n", 
>pixel_format);
return;
}
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index e366ab134249..95e6e58b1a21 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -647,8 +647,8 @@ static int _dpu_format_get_plane_sizes_ubwc(
 
color = _dpu_format_get_media_color_ubwc(fmt);
if (color < 0) {
-   DRM_ERROR("UBWC format not supported for fmt: %4.4s\n",
-   (char *)>base.pixel_format);
+   DRM_ERROR("UBWC format not supported for fmt: %p4cc\n",
+ >base.pixel_format);
return -EINVAL;
}
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ff975ad51145..ff4ac4daaeca 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -234,9 +234,9 @@ static int _dpu_plane_calc_fill_level(struct drm_plane 
*plane,
}
}
 
-   DPU_DEBUG_PLANE(pdpu, "pnum:%d fmt: %4.4s w:%u fl:%u\n",
+   DPU_DEBUG_PLANE(pdpu, "pnum:%d fmt: %p4cc w:%u fl:%u\n",
pipe->sspp->idx - SSPP_VIG0,
-   (char *)>base.pixel_format,
+   >base.pixel_format,
src_width, total_fl);
 
return total_fl;
@@ -287,9 +287,9 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
(fmt) ? fmt->base.pixel_format : 0,
pdpu->is_rt_pipe, total_fl, cfg.creq_lut, lut_usage);
 
-   DPU_DEBUG_PLANE(pdpu, "pnum:%d fmt: %4.4s rt:%d fl:%u lut:0x%llx\n",
+   DPU_DEBUG_PLANE(pdpu, "pnum:%d fmt: %p4cc rt:%d fl:%u lut:0x%llx\n",
pdpu->pipe - SSPP_VIG0,
-

[PATCH 5/6] drm/msm/adreno: Add speedbin data for SM8550 / A740

2024-04-05 Thread Konrad Dybcio

Add speebin data for A740, as found on SM8550 and derivative SoCs.

Signed-off-by: Konrad Dybcio 
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 901ef767e491..c976a485aef2 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -570,6 +570,20 @@ static const struct adreno_info gpulist[] = {
.zapfw = "a740_zap.mdt",
.hwcg = a740_hwcg,
.address_space_size = SZ_16G,
+   .speedbins = ADRENO_SPEEDBINS(
+   { ADRENO_SKU_ID(SOCINFO_PC_UNKNOWN, SOCINFO_FC_AC), 0 },
+   { ADRENO_SKU_ID(SOCINFO_PC_UNKNOWN, SOCINFO_FC_AF), 0 },
+   { ADRENO_SKU_ID(SOCINFO_PCn(1), SOCINFO_FC_UNKNOWN), 1 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(2), SOCINFO_FC_Yn(0x0)), 0 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(2), SOCINFO_FC_Yn(0x2)), 0 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(4), SOCINFO_FC_Yn(0x0)), 0 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(4), SOCINFO_FC_Yn(0x2)), 0 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0x0)), 0 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0x1)), 0 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0xd)), 0 
},
+   { ADRENO_SKU_ID(SOCINFO_PCn(6), SOCINFO_FC_Yn(0xe)), 0 
},
+   ),
+   .default_speedbin = 1,
}, {
.chip_ids = ADRENO_CHIP_IDS(0x43051401), /* "C520v2" */
.family = ADRENO_7XX_GEN3,

-- 
2.40.1

[PATCH 4/6] drm/msm/adreno: Implement SMEM-based speed bin

2024-04-05 Thread Konrad Dybcio

On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
abstracted through SMEM, instead of being directly available in a fuse.

Add support for SMEM-based speed binning, which includes getting
"feature code" and "product code" from said source and parsing them
to form something that lets us match OPPs against.

Signed-off-by: Konrad Dybcio 
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c  |  8 +++---
 drivers/gpu/drm/msm/adreno/adreno_device.c |  2 ++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c| 39 +++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h| 12 ++---
 4 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 4cbdfabbcee5..6776fd80f7a6 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2890,13 +2890,15 @@ static u32 fuse_to_supp_hw(const struct adreno_info 
*info, u32 fuse)
return UINT_MAX;
 }
 
-static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info 
*info)
+static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
+struct device *dev,
+const struct adreno_info *info)
 {
u32 supp_hw;
u32 speedbin;
int ret;
 
-   ret = adreno_read_speedbin(dev, );
+   ret = adreno_read_speedbin(adreno_gpu, dev, );
/*
 * -ENOENT means that the platform doesn't support speedbin which is
 * fine
@@ -3056,7 +3058,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 
a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
 
-   ret = a6xx_set_supported_hw(>dev, config->info);
+   ret = a6xx_set_supported_hw(adreno_gpu, >dev, config->info);
if (ret) {
a6xx_destroy(&(a6xx_gpu->base.base));
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index c3703a51287b..901ef767e491 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -6,6 +6,8 @@
  * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved.
  */
 
+#include 
+
 #include "adreno_gpu.h"
 
 bool hang_debug = false;
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 074fb498706f..0e4ff532ac3c 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -21,6 +21,9 @@
 #include "msm_gem.h"
 #include "msm_mmu.h"
 
+#include 
+#include 
+
 static u64 address_space_size = 0;
 MODULE_PARM_DESC(address_space_size, "Override for size of processes private 
GPU address space");
 module_param(address_space_size, ullong, 0600);
@@ -1057,9 +1060,37 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem 
*adreno_ocmem)
   adreno_ocmem->hdl);
 }
 
-int adreno_read_speedbin(struct device *dev, u32 *speedbin)
+int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
+struct device *dev, u32 *speedbin)
 {
-   return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
+   u32 fcode, pcode;
+   int ret;
+
+   /* Try reading the speedbin via a nvmem cell first */
+   ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
+   if (!ret && ret != -EINVAL)
+   return ret;
+
+   ret = qcom_smem_get_feature_code();
+   if (ret) {
+   dev_err(dev, "Couldn't get feature code from SMEM!\n");
+   return ret;
+   }
+
+   ret = qcom_smem_get_product_code();
+   if (ret) {
+   dev_err(dev, "Couldn't get product code from SMEM!\n");
+   return ret;
+   }
+
+   /* Don't consider fcode for external feature codes */
+   if (fcode <= SOCINFO_FC_EXT_RESERVE)
+   fcode = SOCINFO_FC_UNKNOWN;
+
+   *speedbin = FIELD_PREP(ADRENO_SKU_ID_PCODE, pcode) |
+   FIELD_PREP(ADRENO_SKU_ID_FCODE, fcode);
+
+   return ret;
 }
 
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
@@ -1098,9 +1129,9 @@ int adreno_gpu_init(struct drm_device *drm, struct 
platform_device *pdev,
devm_pm_opp_set_clkname(dev, "core");
}
 
-   if (adreno_read_speedbin(dev, ) || !speedbin)
+   if (adreno_read_speedbin(adreno_gpu, dev, ) || !speedbin)
speedbin = 0x;
-   adreno_gpu->speedbin = (uint16_t) (0x & speedbin);
+   adreno_gpu->speedbin = speedbin;
 
gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
ADRENO_CHIPID_ARGS(config->chip_id));
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 460b399be37b..1770a9e20484 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -81,7 +81,12 @@

[PATCH 2/6] soc: qcom: smem: Add pcode/fcode getters

2024-04-05 Thread Konrad Dybcio

Introduce getters for SoC product and feature codes and export them.

Signed-off-by: Konrad Dybcio 
---
 drivers/soc/qcom/smem.c   | 66 +++
 include/linux/soc/qcom/smem.h |  2 ++
 2 files changed, 68 insertions(+)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 7191fa0c087f..e89b4d26877a 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -795,6 +795,72 @@ int qcom_smem_get_soc_id(u32 *id)
 }
 EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
 
+/**
+ * qcom_smem_get_feature_code() - return the feature code
+ * @id:On success, we return the feature code here.
+ *
+ * Look up the feature code identifier from SMEM and return it.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int qcom_smem_get_feature_code(u32 *code)
+{
+   struct socinfo *info;
+   u32 raw_code;
+
+   info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
+   if (IS_ERR(info))
+   return PTR_ERR(info);
+
+   /* This only makes sense for socinfo >= 16 */
+   if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
+   return -EINVAL;
+
+   raw_code = __le32_to_cpu(info->feature_code);
+
+   /* Ensure the value makes sense */
+   if (raw_code >= SOCINFO_FC_INT_RESERVE)
+   raw_code = SOCINFO_FC_UNKNOWN;
+
+   *code = raw_code;
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_get_feature_code);
+
+/**
+ * qcom_smem_get_product_code() - return the product code
+ * @id:On success, we return the product code here.
+ *
+ * Look up feature code identifier from SMEM and return it.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int qcom_smem_get_product_code(u32 *code)
+{
+   struct socinfo *info;
+   u32 raw_code;
+
+   info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
+   if (IS_ERR(info))
+   return PTR_ERR(info);
+
+   /* This only makes sense for socinfo >= 16 */
+   if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
+   return -EINVAL;
+
+   raw_code = __le32_to_cpu(info->pcode);
+
+   /* Ensure the value makes sense */
+   if (raw_code >= SOCINFO_FC_INT_RESERVE)
+   raw_code = SOCINFO_FC_UNKNOWN;
+
+   *code = raw_code;
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_get_product_code);
+
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
struct smem_header *header;
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index a36a3b9d4929..aef8c9fc6c08 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -13,5 +13,7 @@ int qcom_smem_get_free_space(unsigned host);
 phys_addr_t qcom_smem_virt_to_phys(void *p);
 
 int qcom_smem_get_soc_id(u32 *id);
+int qcom_smem_get_feature_code(u32 *code);
+int qcom_smem_get_product_code(u32 *code);
 
 #endif

-- 
2.40.1

[PATCH 6/6] arm64: dts: qcom: sm8550: Wire up GPU speed bin & more OPPs

2024-04-05 Thread Konrad Dybcio

Add the speedbin masks to ensure only the desired OPPs are available on
chips of a given bin.

Using this, add the binned 719 MHz OPP and the non-binned 124.8 MHz.

Signed-off-by: Konrad Dybcio 
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi 
b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index 5cae8d773cec..2f6842f6a5b7 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -2087,48 +2087,67 @@ zap-shader {
memory-region = <_micro_code_mem>;
};
 
-   /* Speedbin needs more work on A740+, keep only lower 
freqs */
gpu_opp_table: opp-table {
compatible = "operating-points-v2";
 
+   opp-71900 {
+   opp-hz = /bits/ 64 <71900>;
+   opp-level = 
;
+   opp-supported-hw = <0x1>;
+   };
+
opp-68000 {
opp-hz = /bits/ 64 <68000>;
opp-level = 
;
+   opp-supported-hw = <0x3>;
};
 
opp-61500 {
opp-hz = /bits/ 64 <61500>;
opp-level = 
;
+   opp-supported-hw = <0x3>;
};
 
opp-55000 {
opp-hz = /bits/ 64 <55000>;
opp-level = ;
+   opp-supported-hw = <0x3>;
};
 
opp-47500 {
opp-hz = /bits/ 64 <47500>;
opp-level = 
;
+   opp-supported-hw = <0x3>;
};
 
opp-40100 {
opp-hz = /bits/ 64 <40100>;
opp-level = 
;
+   opp-supported-hw = <0x3>;
};
 
opp-34800 {
opp-hz = /bits/ 64 <34800>;
opp-level = 
;
+   opp-supported-hw = <0x3>;
};
 
opp-29500 {
opp-hz = /bits/ 64 <29500>;
opp-level = 
;
+   opp-supported-hw = <0x3>;
};
 
opp-22000 {
opp-hz = /bits/ 64 <22000>;
opp-level = 
;
+   opp-supported-hw = <0x3>;
+   };
+
+   opp-12480 {
+   opp-hz = /bits/ 64 <12480>;
+   opp-level = 
;
+   opp-supported-hw = <0x3>;
};
};
};

-- 
2.40.1

[PATCH 3/6] drm/msm/adreno: Allow specifying default speedbin value

2024-04-05 Thread Konrad Dybcio

From: Neil Armstrong 

Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock
the highest. Falling back to it when things go wrong is largely
suboptimal, as more often than not, the top frequencies are not
supposed to work on other bins.

Let the developer specify the intended "lowest common denominator" bin
in struct adreno_info. If not specified, partial struct initialization
will ensure it's set to zero, retaining previous behavior.

Signed-off-by: Neil Armstrong 
[Konrad: clean up, add commit message]
Signed-off-by: Konrad Dybcio 
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 2 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 0674aca0f8a3..4cbdfabbcee5 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2915,7 +2915,7 @@ static int a6xx_set_supported_hw(struct device *dev, 
const struct adreno_info *i
DRM_DEV_ERROR(dev,
"missing support for speed-bin: %u. Some OPPs may not 
be supported by hardware\n",
speedbin);
-   supp_hw = BIT(0); /* Default */
+   supp_hw = BIT(info->default_speedbin); /* Default */
}
 
ret = devm_pm_opp_set_supported_hw(dev, _hw, 1);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 77526892eb8c..460b399be37b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -110,6 +110,7 @@ struct adreno_info {
 * {SHRT_MAX, 0} sentinal.
 */
struct adreno_speedbin *speedbins;
+   unsigned int default_speedbin;
 };
 
 #define ADRENO_CHIP_IDS(tbl...) (uint32_t[]) { tbl, 0 }

-- 
2.40.1

[PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

2024-04-05 Thread Konrad Dybcio

In preparation for parsing the chip "feature code" (FC) and "product
code" (PC) (essentially the parameters that let us conclusively
characterize the sillicon we're running on, including various speed
bins), move the socinfo version defines to the public header and
include some more FC/PC defines.

Signed-off-by: Konrad Dybcio 
---
 drivers/soc/qcom/socinfo.c   |  8 
 include/linux/soc/qcom/socinfo.h | 36 
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 277c07a6603d..cf4616a468f2 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -21,14 +21,6 @@
 
 #include 
 
-/*
- * SoC version type with major number in the upper 16 bits and minor
- * number in the lower 16 bits.
- */
-#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0x)
-#define SOCINFO_MINOR(ver) ((ver) & 0x)
-#define SOCINFO_VERSION(maj, min)  maj) & 0x) << 16)|((min) & 0x))
-
 /* Helper macros to create soc_id table */
 #define qcom_board_id(id) QCOM_ID_ ## id, __stringify(id)
 #define qcom_board_id_named(id, name) QCOM_ID_ ## id, (name)
diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
index e78777bb0f4a..ba7f683bd32c 100644
--- a/include/linux/soc/qcom/socinfo.h
+++ b/include/linux/soc/qcom/socinfo.h
@@ -3,6 +3,8 @@
 #ifndef __QCOM_SOCINFO_H__
 #define __QCOM_SOCINFO_H__
 
+#include 
+
 /*
  * SMEM item id, used to acquire handles to respective
  * SMEM region.
@@ -12,6 +14,14 @@
 #define SMEM_SOCINFO_BUILD_ID_LENGTH   32
 #define SMEM_SOCINFO_CHIP_ID_LENGTH32
 
+/*
+ * SoC version type with major number in the upper 16 bits and minor
+ * number in the lower 16 bits.
+ */
+#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0x)
+#define SOCINFO_MINOR(ver) ((ver) & 0x)
+#define SOCINFO_VERSION(maj, min)  maj) & 0x) << 16)|((min) & 0x))
+
 /* Socinfo SMEM item structure */
 struct socinfo {
__le32 fmt;
@@ -74,4 +84,30 @@ struct socinfo {
__le32 boot_core;
 };
 
+/* Internal feature codes */
+enum feature_code {
+   /* External feature codes */
+   SOCINFO_FC_UNKNOWN = 0x0,
+   SOCINFO_FC_AA,
+   SOCINFO_FC_AB,
+   SOCINFO_FC_AC,
+   SOCINFO_FC_AD,
+   SOCINFO_FC_AE,
+   SOCINFO_FC_AF,
+   SOCINFO_FC_AG,
+   SOCINFO_FC_AH,
+   SOCINFO_FC_EXT_RESERVE,
+};
+
+/* Internal feature codes */
+/* Valid values: 0 <= n <= 0xf */
+#define SOCINFO_FC_Yn(n)   (0xf1 + n)
+#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10)
+
+/* Product codes */
+#define SOCINFO_PC_UNKNOWN 0
+/* Valid values: 0 <= n <= 8, the rest is reserved */
+#define SOCINFO_PCn(n) (n + 1)
+#define SOCINFO_PC_RESERVE (BIT(31) - 1)
+
 #endif

-- 
2.40.1

[PATCH 0/6] Add SMEM-based speedbin matching

2024-04-05 Thread Konrad Dybcio

Newer (SM8550+) SoCs don't seem to have a nice speedbin fuse anymore,
but instead rely on a set of combinations of "feature code" (FC) and
"product code" (PC) identifiers to match the bins. This series adds
support for that.

I suppose a qcom/for-soc immutable branch would be in order if we want
to land this in the upcoming cycle.

FWIW I preferred the fuses myself..

Signed-off-by: Konrad Dybcio 
---
Konrad Dybcio (5):
  soc: qcom: Move some socinfo defines to the header, expand them
  soc: qcom: smem: Add pcode/fcode getters
  drm/msm/adreno: Implement SMEM-based speed bin
  drm/msm/adreno: Add speedbin data for SM8550 / A740
  arm64: dts: qcom: sm8550: Wire up GPU speed bin & more OPPs

Neil Armstrong (1):
  drm/msm/adreno: Allow specifying default speedbin value

 arch/arm64/boot/dts/qcom/sm8550.dtsi   | 21 +-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c  | 10 +++--
 drivers/gpu/drm/msm/adreno/adreno_device.c | 16 
 drivers/gpu/drm/msm/adreno/adreno_gpu.c| 39 --
 drivers/gpu/drm/msm/adreno/adreno_gpu.h| 13 --
 drivers/soc/qcom/smem.c| 66 ++
 drivers/soc/qcom/socinfo.c |  8 
 include/linux/soc/qcom/smem.h  |  2 +
 include/linux/soc/qcom/socinfo.h   | 36 
 9 files changed, 191 insertions(+), 20 deletions(-)
---
base-commit: 2b3d5988ae2cb5cd945ddbc653f0a71706231fdd
change-id: 20240404-topic-smem_speedbin-8deecd0bef0e

Best regards,
-- 
Konrad Dybcio

Re: [PATCH] phy: qcom: qmp-combo: Fix register base for QSERDES_DP_PHY_MODE

2024-04-05 Thread Johan Hovold

On Thu, Apr 04, 2024 at 05:56:32PM -0700, Bjorn Andersson wrote:
> On Thu, Apr 04, 2024 at 05:01:03PM -0700, Stephen Boyd wrote:
> > The register base that was used to write to the QSERDES_DP_PHY_MODE
> > register was 'dp_dp_phy' before commit 815891eee668 ("phy:
> > qcom-qmp-combo: Introduce orientation variable"). There isn't any
> > explanation in the commit why this is changed, so I suspect it was an
> > oversight or happened while being extracted from some other series.
> 
> Thanks for catching that, I wrote that patch long before Johan did the
> rename of "pcs" to "dp_dp_phy", and must have missed that while later
> rebasing the patch.
> 
> Reviewed-by: Bjorn Andersson 
>
> > Oddly the value being 0x4c or 0x5c doesn't seem to matter for me, so I
> > suspect this is dead code, but that can be fixed in another patch. It's
> > not good to write to the wrong register space, and maybe some other
> > version of this phy relies on this.

This code is still reached on sc8280xp, but I guess only Qualcomm can
tell us what these bits are for (and they should).

The write to qmp->pcs + QSERDES_DP_PHY_MODE does not seem to have any
effect on sc8280xp and that register still reads back as 0x2020202 after
the incorrect write.

qmp->dp_dp_phy + QSERDES_DP_PHY_MODE reads back as 0x4c4c4c4c before the
fixed write and either 0x4c4c4c4c or 0x5c5c5c5c after depending on the
orientation.

Can someone please replace the magic constants in this driver, and at
least explain what the impact of bit 0x10 not reflecting the orientation
is?

> > Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> > Signed-off-by: Stephen Boyd 

Either way, good catch, this was clearly unintentional:

Reviewed-by: Johan Hovold 

I think this should go to stable as well even if the impact is currently
not fully understood:

Cc: sta...@vger.kernel.org  # 6.5

> > @@ -2150,9 +2150,9 @@ static bool qmp_combo_configure_dp_mode(struct 
> > qmp_combo *qmp)
> > writel(val, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
> >  
> > if (reverse)
> > -   writel(0x4c, qmp->pcs + QSERDES_DP_PHY_MODE);
> > +   writel(0x4c, qmp->dp_dp_phy + QSERDES_DP_PHY_MODE);
> > else
> > -   writel(0x5c, qmp->pcs + QSERDES_DP_PHY_MODE);
> > +   writel(0x5c, qmp->dp_dp_phy + QSERDES_DP_PHY_MODE);

Johan

42 matches

Mail list logo