date:20210524

[Freedreno] [PATCH] drm/msm/dp: Drop unnecessary NULL checks after container_of

2021-05-24 Thread Guenter Roeck

The result of container_of() operations is never NULL unless the embedded
element is the first element of the structure. This is not the case here.
The NULL check on the result of container_of() is therefore unnecessary
and misleading. Remove it.

This change was made automatically with the following Coccinelle script.

@@
type t;
identifier v;
statement s;
@@

<+...
(
  t v = container_of(...);
|
  v = container_of(...);
)
  ...
  when != v
- if (\( !v \| v == NULL \) ) s
...+>

While at it, remove unused but assigned variable hpd in
dp_display_usbpd_attention_cb().

Signed-off-by: Guenter Roeck 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 25 -
 1 file changed, 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 1784e119269b..a74e7ef96fcf 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -208,10 +208,6 @@ static int dp_display_bind(struct device *dev, struct 
device *master,
 
dp = container_of(g_dp_display,
struct dp_display_private, dp_display);
-   if (!dp) {
-   DRM_ERROR("DP driver bind failed. Invalid driver data\n");
-   return -EINVAL;
-   }
 
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
@@ -252,10 +248,6 @@ static void dp_display_unbind(struct device *dev, struct 
device *master,
 
dp = container_of(g_dp_display,
struct dp_display_private, dp_display);
-   if (!dp) {
-   DRM_ERROR("Invalid DP driver data\n");
-   return;
-   }
 
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
@@ -406,11 +398,6 @@ static int dp_display_usbpd_configure_cb(struct device 
*dev)
 
dp = container_of(g_dp_display,
struct dp_display_private, dp_display);
-   if (!dp) {
-   DRM_ERROR("no driver data found\n");
-   rc = -ENODEV;
-   goto end;
-   }
 
dp_display_host_init(dp, false);
 
@@ -437,11 +424,6 @@ static int dp_display_usbpd_disconnect_cb(struct device 
*dev)
 
dp = container_of(g_dp_display,
struct dp_display_private, dp_display);
-   if (!dp) {
-   DRM_ERROR("no driver data found\n");
-   rc = -ENODEV;
-   return rc;
-   }
 
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
 
@@ -502,7 +484,6 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
int rc = 0;
u32 sink_request;
struct dp_display_private *dp;
-   struct dp_usbpd *hpd;
 
if (!dev) {
DRM_ERROR("invalid dev\n");
@@ -511,12 +492,6 @@ static int dp_display_usbpd_attention_cb(struct device 
*dev)
 
dp = container_of(g_dp_display,
struct dp_display_private, dp_display);
-   if (!dp) {
-   DRM_ERROR("no driver data found\n");
-   return -ENODEV;
-   }
-
-   hpd = dp->usbpd;
 
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
-- 
2.25.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v18 2/4] dt-bindings: msm: dsi: add yaml schemas for DSI bindings

2021-05-24 Thread Rob Herring

On Mon, 24 May 2021 17:14:11 +0530, Krishna Manikandan wrote:
> Add YAML schema for the device tree bindings for DSI
> 
> Signed-off-by: Krishna Manikandan 
> Reviewed-by: Bjorn Andersson 
> Reviewed-by: Stephen Boyd 
> ---
> Changes in v1:
> - Separate dsi controller bindings to a separate patch (Stephen Boyd)
> - Merge dsi-common-controller.yaml and dsi-controller-main.yaml to
>   a single file (Stephen Boyd)
> - Drop supply entries and definitions from properties (Stephen Boyd)
> - Modify phy-names property for dsi controller (Stephen Boyd)
> - Remove boolean from description (Stephen Boyd)
> - Drop pinctrl properties as they are standard entries (Stephen Boyd)
> - Modify the description for ports property and keep the reference
>   to the generic binding where this is defined (Stephen Boyd)
> - Add description to clock names (Stephen Boyd)
> - Correct the indendation (Stephen Boyd)
> - Drop the label for display dt nodes and correct the node
>   name (Stephen Boyd)
> 
> Changes in v2:
> - Drop maxItems for clock (Stephen Boyd)
> - Drop qcom,mdss-mdp-transfer-time-us as it is not used in upstream
>   dt file (Stephen Boyd)
> - Keep child node directly under soc node (Stephen Boyd)
> - Drop qcom,sync-dual-dsi as it is not used in upstream dt
> 
> Changes in v3:
> - Add description for register property (Stephen Boyd)
> 
> Changes in v4:
> - Add maxItems for phys property (Stephen Boyd)
> - Add maxItems for reg property (Stephen Boyd)
> - Add reference for data-lanes property (Stephen Boyd)
> - Remove soc from example (Stephen Boyd)
> 
> Changes in v5:
> - Modify title and description (Stephen Boyd)
> - Add required properties for ports node (Stephen Boyd)
> - Add data-lanes in the example (Stephen Boyd)
> - Drop qcom,master-dsi property (Stephen Boyd)
> 
> Changes in v6:
> - Add required properties for port@0, port@1 and corresponding
>   endpoints (Stephen Boyd)
> - Add address-cells and size-cells for ports (Stephen Boyd)
> - Use additionalProperties instead of unevaluatedProperties (Stephen Boyd)
> 
> Changes in v7:
> - Add reference for ports and data-lanes (Rob Herring)
> - Add maxItems and minItems for data-lanes (Rob Herring)
> 
> Changes in v8:
> - Drop common properties and description from ports (Rob Herring)
> - Add reference for endpoint (Rob Herring)
> - Add correct reference for data-lanes (Rob Herring)
> - Drop common properties from required list for ports (Rob Herring)
> 
> Changes in v9:
> - Drop reference for data-lanes (Rob Herring)
> - Add unevaluatedProperties for endpoint (Rob Herring)
> 
>  .../bindings/display/msm/dsi-controller-main.yaml  | 185 +++
>  .../devicetree/bindings/display/msm/dsi.txt| 249 
> -
>  2 files changed, 185 insertions(+), 249 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>  delete mode 100644 Documentation/devicetree/bindings/display/msm/dsi.txt
> 

Reviewed-by: Rob Herring 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v2 6/6] drm/msm/dpu: simplify IRQ enabling/disabling

2021-05-24 Thread abhinavk


On 2021-05-16 13:29, Dmitry Baryshkov wrote:

Merge dpu_core_irq_enable() into dpu_core_irq_register_callback() and
dpu_core_irq_disable() into dpu_core_irq_unregister_callback(), because
they are called in pairs. There is no need to have separate
enable/disable pair, we can enable hardware IRQ when first callback is
registered and when the last callback is unregistered.

Since this change also removes the enable_counts atomic counter, I was a 
bit hesitant
because its helpful to protect against vblank discrepancies from 
usermode during enable/disable

but i think we have more protection for blank using vblank_refcount

413 if (enable && atomic_inc_return(_enc->vblank_refcount) == 1)
414 ret = dpu_encoder_helper_register_irq(phys_enc, INTR_IDX_VSYNC);
415 	else if (!enable && atomic_dec_return(_enc->vblank_refcount) 
== 0)

416 ret = dpu_encoder_helper_unregister_irq(phys_enc,
417 INTR_IDX_VSYNC);
418
419 end:

So this should still be okay. Hence

Signed-off-by: Dmitry Baryshkov 

Reviewed-by: Abhinav Kumar 

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c | 168 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h |  30 
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c  |  18 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  |   2 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h|   8 -
 5 files changed, 27 insertions(+), 199 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
index 11c0abed21ee..4f110c428b60 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
@@ -26,10 +26,8 @@ static void dpu_core_irq_callback_handler(void
*arg, int irq_idx)

pr_debug("irq_idx=%d\n", irq_idx);

-   if (list_empty(_obj->irq_cb_tbl[irq_idx])) {
-   DRM_ERROR("no registered cb, idx:%d enable_count:%d\n", irq_idx,
-   atomic_read(_kms->irq_obj.enable_counts[irq_idx]));
-   }
+   if (list_empty(_obj->irq_cb_tbl[irq_idx]))
+   DRM_ERROR("no registered cb, idx:%d\n", irq_idx);

atomic_inc(_obj->irq_counts[irq_idx]);

@@ -43,127 +41,6 @@ static void dpu_core_irq_callback_handler(void
*arg, int irq_idx)
spin_unlock_irqrestore(_kms->irq_obj.cb_lock, irq_flags);
 }

-/**
- * _dpu_core_irq_enable - enable core interrupt given by the index
- * @dpu_kms:   Pointer to dpu kms context
- * @irq_idx:   interrupt index
- */
-static int _dpu_core_irq_enable(struct dpu_kms *dpu_kms, int irq_idx)
-{
-   unsigned long irq_flags;
-   int ret = 0, enable_count;
-
-   if (!dpu_kms->hw_intr ||
-   !dpu_kms->irq_obj.enable_counts ||
-   !dpu_kms->irq_obj.irq_counts) {
-   DPU_ERROR("invalid params\n");
-   return -EINVAL;
-   }
-
-   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->total_irqs) {
-   DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx);
-   return -EINVAL;
-   }
-
-   enable_count = atomic_read(_kms->irq_obj.enable_counts[irq_idx]);
-   DRM_DEBUG_KMS("irq_idx=%d enable_count=%d\n", irq_idx, enable_count);
-   trace_dpu_core_irq_enable_idx(irq_idx, enable_count);
-
-	if (atomic_inc_return(_kms->irq_obj.enable_counts[irq_idx]) == 1) 
{

-   ret = dpu_kms->hw_intr->ops.enable_irq(
-   dpu_kms->hw_intr,
-   irq_idx);
-   if (ret)
-   DPU_ERROR("Fail to enable IRQ for irq_idx:%d\n",
-   irq_idx);
-
-   DPU_DEBUG("irq_idx=%d ret=%d\n", irq_idx, ret);
-
-   spin_lock_irqsave(_kms->irq_obj.cb_lock, irq_flags);
-   /* empty callback list but interrupt is enabled */
-   if (list_empty(_kms->irq_obj.irq_cb_tbl[irq_idx]))
-   DPU_ERROR("irq_idx=%d enabled with no callback\n",
-   irq_idx);
-   spin_unlock_irqrestore(_kms->irq_obj.cb_lock, irq_flags);
-   }
-
-   return ret;
-}
-
-int dpu_core_irq_enable(struct dpu_kms *dpu_kms, int *irq_idxs, u32 
irq_count)

-{
-   int i, ret = 0, counts;
-
-   if (!irq_idxs || !irq_count) {
-   DPU_ERROR("invalid params\n");
-   return -EINVAL;
-   }
-
-   counts = atomic_read(_kms->irq_obj.enable_counts[irq_idxs[0]]);
-   if (counts)
-   DRM_ERROR("irq_idx=%d enable_count=%d\n", irq_idxs[0], counts);
-
-   for (i = 0; (i < irq_count) && !ret; i++)
-   ret = _dpu_core_irq_enable(dpu_kms, irq_idxs[i]);
-
-   return ret;
-}
-
-/**
- * _dpu_core_irq_disable - disable core interrupt given by the index
- * @dpu_kms:   Pointer to dpu kms context
- * @irq_idx:   interrupt index
- */
-static int _dpu_core_irq_disable(struct dpu_kms *dpu_kms, int irq_idx)
-{
-   int

Re: [Freedreno] [PATCH v2 4/6] drm/msm/dpu: replace IRQ lookup with the data in hw catalog

2021-05-24 Thread abhinavk


On 2021-05-16 13:29, Dmitry Baryshkov wrote:

The IRQ table in the dpu_hw_interrupts.h is big, ugly, and hard to
maintain. There are only few interrupts used from that table. Newer
generations use different IRQ locations. Move this data to hw catalog.

I think you can drop the line that "only few interrupts ..." are used as 
that
was specific to sc7280 as you mentioned on IRC or give the example of 
sc7280

here to explain that as being one of the motivations for this cleanup.


Signed-off-by: Dmitry Baryshkov 


With that fixed, I think this cleanup looks quite reasonable to me.
I computed a few of the intr offsets and the values were matching the 
prev hard-coded
offsets. Since I dont have the sc7280 HW to test this, I am checking if 
someone with that
hw in our team can test it out as well to make sure that it covers that 
hw as well.


But otherwise,

Reviewed-by: Abhinav Kumar 

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c  |  20 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h  |  13 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  64 +++-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |   2 -
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  36 ++---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  31 +---
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 150 +++---
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h|  12 +-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 137 +++-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  17 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |  28 ++--
 11 files changed, 229 insertions(+), 281 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
index fd11a2aeab6c..11c0abed21ee 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
@@ -43,16 +43,6 @@ static void dpu_core_irq_callback_handler(void
*arg, int irq_idx)
spin_unlock_irqrestore(_kms->irq_obj.cb_lock, irq_flags);
 }

-int dpu_core_irq_idx_lookup(struct dpu_kms *dpu_kms,
-   enum dpu_intr_type intr_type, u32 instance_idx)
-{
-   if (!dpu_kms->hw_intr || !dpu_kms->hw_intr->ops.irq_idx_lookup)
-   return -EINVAL;
-
-   return dpu_kms->hw_intr->ops.irq_idx_lookup(dpu_kms->hw_intr,
-   intr_type, instance_idx);
-}
-
 /**
  * _dpu_core_irq_enable - enable core interrupt given by the index
  * @dpu_kms:   Pointer to dpu kms context
@@ -70,7 +60,7 @@ static int _dpu_core_irq_enable(struct dpu_kms
*dpu_kms, int irq_idx)
return -EINVAL;
}

-   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->irq_idx_tbl_size) {
+   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->total_irqs) {
DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx);
return -EINVAL;
}
@@ -133,7 +123,7 @@ static int _dpu_core_irq_disable(struct dpu_kms
*dpu_kms, int irq_idx)
return -EINVAL;
}

-   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->irq_idx_tbl_size) {
+   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->total_irqs) {
DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx);
return -EINVAL;
}
@@ -208,7 +198,7 @@ int dpu_core_irq_register_callback(struct dpu_kms
*dpu_kms, int irq_idx,
return -EINVAL;
}

-   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->irq_idx_tbl_size) {
+   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->total_irqs) {
DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx);
return -EINVAL;
}
@@ -243,7 +233,7 @@ int dpu_core_irq_unregister_callback(struct
dpu_kms *dpu_kms, int irq_idx,
return -EINVAL;
}

-   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->irq_idx_tbl_size) {
+   if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->total_irqs) {
DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx);
return -EINVAL;
}
@@ -328,7 +318,7 @@ void dpu_core_irq_preinstall(struct dpu_kms 
*dpu_kms)

spin_lock_init(_kms->irq_obj.cb_lock);

/* Create irq callbacks for all possible irq_idx */
-   dpu_kms->irq_obj.total_irqs = dpu_kms->hw_intr->irq_idx_tbl_size;
+   dpu_kms->irq_obj.total_irqs = dpu_kms->hw_intr->total_irqs;
dpu_kms->irq_obj.irq_cb_tbl = kcalloc(dpu_kms->irq_obj.total_irqs,
sizeof(struct list_head), GFP_KERNEL);
dpu_kms->irq_obj.enable_counts = kcalloc(dpu_kms->irq_obj.total_irqs,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
index e30775e6585b..d147784d5531 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
@@ -29,19 +29,6 @@ void dpu_core_irq_uninstall(struct dpu_kms 
*dpu_kms);

  */
 irqreturn_t dpu_core_irq(struct dpu_kms *dpu_kms);

-/**
- *

Re: [Freedreno] [PATCH v2 3/6] drm/msm/dpu: define interrupt register names

2021-05-24 Thread abhinavk


On 2021-05-16 13:29, Dmitry Baryshkov wrote:

In order to make mdss_irqs readable (and error-prone) define names for

I think you meant "less error-prone" here.

interrupt register indices.

Signed-off-by: Dmitry Baryshkov 

With that nit fixed,
Reviewed-by: Abhinav Kumar 

---
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 45 ---
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 18 
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index b569030a0847..9a77d64d3fd4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include "dpu_hw_mdss.h"
+#include "dpu_hw_interrupts.h"
 #include "dpu_hw_catalog.h"
 #include "dpu_kms.h"

@@ -56,6 +57,23 @@

 #define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)

+#define IRQ_SDM845_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
+BIT(MDP_SSPP_TOP0_INTR2) | \
+BIT(MDP_SSPP_TOP0_HIST_INTR) | \
+BIT(MDP_INTF0_INTR) | \
+BIT(MDP_INTF1_INTR) | \
+BIT(MDP_INTF2_INTR) | \
+BIT(MDP_INTF3_INTR) | \
+BIT(MDP_INTF4_INTR) | \
+BIT(MDP_AD4_0_INTR) | \
+BIT(MDP_AD4_1_INTR))
+
+#define IRQ_SC7180_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
+BIT(MDP_SSPP_TOP0_INTR2) | \
+BIT(MDP_SSPP_TOP0_HIST_INTR) | \
+BIT(MDP_INTF0_INTR) | \
+BIT(MDP_INTF1_INTR))
+
 #define INTR_SC7180_MASK \
(BIT(DPU_IRQ_TYPE_PING_PONG_RD_PTR) |\
BIT(DPU_IRQ_TYPE_PING_PONG_WR_PTR) |\
@@ -63,6 +81,23 @@
BIT(DPU_IRQ_TYPE_PING_PONG_TEAR_CHECK) |\
BIT(DPU_IRQ_TYPE_PING_PONG_TE_CHECK))

+#define IRQ_SC7280_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
+BIT(MDP_SSPP_TOP0_INTR2) | \
+BIT(MDP_SSPP_TOP0_HIST_INTR) | \
+BIT(MDP_INTF0_7xxx_INTR) | \
+BIT(MDP_INTF1_7xxx_INTR) | \
+BIT(MDP_INTF5_7xxx_INTR))
+
+#define IRQ_SM8250_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
+BIT(MDP_SSPP_TOP0_INTR2) | \
+BIT(MDP_SSPP_TOP0_HIST_INTR) | \
+BIT(MDP_INTF0_INTR) | \
+BIT(MDP_INTF1_INTR) | \
+BIT(MDP_INTF2_INTR) | \
+BIT(MDP_INTF3_INTR) | \
+BIT(MDP_INTF4_INTR))
+
+
 #define DEFAULT_PIXEL_RAM_SIZE (50 * 1024)
 #define DEFAULT_DPU_LINE_WIDTH 2048
 #define DEFAULT_DPU_OUTPUT_LINE_WIDTH  2560
@@ -1060,7 +1095,7 @@ static void sdm845_cfg_init(struct dpu_mdss_cfg 
*dpu_cfg)

.reg_dma_count = 1,
.dma_cfg = sdm845_regdma,
.perf = sdm845_perf_data,
-   .mdss_irqs = 0x3ff,
+   .mdss_irqs = IRQ_SDM845_MASK,
};
 }

@@ -1091,7 +1126,7 @@ static void sc7180_cfg_init(struct dpu_mdss_cfg 
*dpu_cfg)

.reg_dma_count = 1,
.dma_cfg = sdm845_regdma,
.perf = sc7180_perf_data,
-   .mdss_irqs = 0x3f,
+   .mdss_irqs = IRQ_SC7180_MASK,
.obsolete_irq = INTR_SC7180_MASK,
};
 }
@@ -1125,7 +1160,7 @@ static void sm8150_cfg_init(struct dpu_mdss_cfg 
*dpu_cfg)

.reg_dma_count = 1,
.dma_cfg = sm8150_regdma,
.perf = sm8150_perf_data,
-   .mdss_irqs = 0x3ff,
+   .mdss_irqs = IRQ_SDM845_MASK,
};
 }

@@ -1158,7 +1193,7 @@ static void sm8250_cfg_init(struct dpu_mdss_cfg 
*dpu_cfg)

.reg_dma_count = 1,
.dma_cfg = sm8250_regdma,
.perf = sm8250_perf_data,
-   .mdss_irqs = 0xff,
+   .mdss_irqs = IRQ_SM8250_MASK,
};
 }

@@ -1181,7 +1216,7 @@ static void sc7280_cfg_init(struct dpu_mdss_cfg 
*dpu_cfg)

.vbif_count = ARRAY_SIZE(sdm845_vbif),
.vbif = sdm845_vbif,
.perf = sc7280_perf_data,
-   .mdss_irqs = 0x1c07,
+   .mdss_irqs = IRQ_SC7280_MASK,
.obsolete_irq = INTR_SC7180_MASK,
};
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
index 5bade5637ecc..b26a3445a8eb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
@@ -74,6 +74,24 @@ enum dpu_intr_type {
DPU_IRQ_TYPE_RESERVED,
 };

+/* When making changes be sure to sync with dpu_intr_set */
+enum dpu_hw_intr_reg {
+   MDP_SSPP_TOP0_INTR,
+   MDP_SSPP_TOP0_INTR2,
+   MDP_SSPP_TOP0_HIST_INTR,
+   MDP_INTF0_INTR,
+

Re: [Freedreno] [PATCH v2 2/6] drm/msm/dpu: hw_intr: always call dpu_hw_intr_clear_intr_status_nolock

2021-05-24 Thread abhinavk


On 2021-05-16 13:29, Dmitry Baryshkov wrote:

Always call dpu_hw_intr_clear_intr_status_nolock() from the
dpu_hw_intr_dispatch_irqs(). This simplifies the callback function
(which call clears the interrupts anyway) and enforces clearing the hw
interrupt status.


In the subject line you can remove the "hw_intr:"


Signed-off-by: Dmitry Baryshkov 
Reviewed-by: Bjorn Andersson 

With that nit fixed,
Reviewed-by: Abhinav Kumar 

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c  |  9 -
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 39 +--
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  9 -
 3 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
index 54b34746a587..fd11a2aeab6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
@@ -41,15 +41,6 @@ static void dpu_core_irq_callback_handler(void
*arg, int irq_idx)
if (cb->func)
cb->func(cb->arg, irq_idx);
spin_unlock_irqrestore(_kms->irq_obj.cb_lock, irq_flags);
-
-   /*
-* Clear pending interrupt status in HW.
-* NOTE: dpu_core_irq_callback_handler is protected by top-level
-*   spinlock, so it is safe to clear any interrupt status here.
-*/
-   dpu_kms->hw_intr->ops.clear_intr_status_nolock(
-   dpu_kms->hw_intr,
-   irq_idx);
 }

 int dpu_core_irq_idx_lookup(struct dpu_kms *dpu_kms,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index cf9bfd45aa59..8bd22e060437 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -1362,6 +1362,22 @@ static int dpu_hw_intr_irqidx_lookup(struct
dpu_hw_intr *intr,
return -EINVAL;
 }

+static void dpu_hw_intr_clear_intr_status_nolock(struct dpu_hw_intr 
*intr,

+   int irq_idx)
+{
+   int reg_idx;
+
+   if (!intr)
+   return;
+
+   reg_idx = dpu_irq_map[irq_idx].reg_idx;
+   DPU_REG_WRITE(>hw, dpu_intr_set[reg_idx].clr_off,
+   dpu_irq_map[irq_idx].irq_mask);
+
+   /* ensure register writes go through */
+   wmb();
+}
+
 static void dpu_hw_intr_dispatch_irq(struct dpu_hw_intr *intr,
void (*cbfunc)(void *, int),
void *arg)
@@ -1430,9 +1446,8 @@ static void dpu_hw_intr_dispatch_irq(struct
dpu_hw_intr *intr,
 */
if (cbfunc)
cbfunc(arg, irq_idx);
-   else
-   intr->ops.clear_intr_status_nolock(
-   intr, irq_idx);
+
+   dpu_hw_intr_clear_intr_status_nolock(intr, 
irq_idx);

/*
 * When callback finish, clear the irq_status
@@ -1597,23 +1612,6 @@ static int dpu_hw_intr_disable_irqs(struct
dpu_hw_intr *intr)
return 0;
 }

-
-static void dpu_hw_intr_clear_intr_status_nolock(struct dpu_hw_intr 
*intr,

-   int irq_idx)
-{
-   int reg_idx;
-
-   if (!intr)
-   return;
-
-   reg_idx = dpu_irq_map[irq_idx].reg_idx;
-   DPU_REG_WRITE(>hw, dpu_intr_set[reg_idx].clr_off,
-   dpu_irq_map[irq_idx].irq_mask);
-
-   /* ensure register writes go through */
-   wmb();
-}
-
 static u32 dpu_hw_intr_get_interrupt_status(struct dpu_hw_intr *intr,
int irq_idx, bool clear)
 {
@@ -1655,7 +1653,6 @@ static void __setup_intr_ops(struct 
dpu_hw_intr_ops *ops)

ops->dispatch_irqs = dpu_hw_intr_dispatch_irq;
ops->clear_all_irqs = dpu_hw_intr_clear_irqs;
ops->disable_all_irqs = dpu_hw_intr_disable_irqs;
-   ops->clear_intr_status_nolock = dpu_hw_intr_clear_intr_status_nolock;
ops->get_interrupt_status = dpu_hw_intr_get_interrupt_status;
 }

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
index 5a1c304ba93f..5bade5637ecc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
@@ -142,15 +142,6 @@ struct dpu_hw_intr_ops {
void (*cbfunc)(void *arg, int irq_idx),
void *arg);

-   /**
-* clear_intr_status_nolock() - clears the HW interrupts without lock
-* @intr:   HW interrupt handle
-* @irq_idx:Lookup irq index return from irq_idx_lookup
-*/
-   void (*clear_intr_status_nolock)(
-   struct dpu_hw_intr *intr,
-   int irq_idx);
-
/**
 * get_interrupt_status - Gets HW interrupt status, and clear if set,

Re: [Freedreno] [PATCH v2 1/6] drm/msm/dpu: merge dpu_hw_intr_get_interrupt_statuses into dpu_hw_intr_dispatch_irqs

2021-05-24 Thread abhinavk


On 2021-05-16 13:29, Dmitry Baryshkov wrote:
There is little sense in reading interrupt statuses and right after 
that

going after the array of statuses to dispatch them. Merge both loops
into single function doing read and dispatch.

Signed-off-by: Dmitry Baryshkov 
Reviewed-by: Bjorn Andersson 

Reviewed-by: Abhinav Kumar 

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c  | 10 +--
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 66 ++-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  8 ---
 3 files changed, 20 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
index cdec3fbe6ff4..54b34746a587 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
@@ -376,15 +376,6 @@ void dpu_core_irq_uninstall(struct dpu_kms 
*dpu_kms)


 irqreturn_t dpu_core_irq(struct dpu_kms *dpu_kms)
 {
-   /*
-* Read interrupt status from all sources. Interrupt status are
-* stored within hw_intr.
-* Function will also clear the interrupt status after reading.
-* Individual interrupt status bit will only get stored if it
-* is enabled.
-*/
-   dpu_kms->hw_intr->ops.get_interrupt_statuses(dpu_kms->hw_intr);
-
/*
 * Dispatch to HW driver to handle interrupt lookup that is being
 * fired. When matching interrupt is located, HW driver will call to
@@ -392,6 +383,7 @@ irqreturn_t dpu_core_irq(struct dpu_kms *dpu_kms)
 * dpu_core_irq_callback_handler will perform the registered function
 * callback, and do the interrupt status clearing once the registered
 * callback is finished.
+* Function will also clear the interrupt status after reading.
 */
dpu_kms->hw_intr->ops.dispatch_irqs(
dpu_kms->hw_intr,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 48c96b812126..cf9bfd45aa59 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -1371,6 +1371,7 @@ static void dpu_hw_intr_dispatch_irq(struct
dpu_hw_intr *intr,
int start_idx;
int end_idx;
u32 irq_status;
+   u32 enable_mask;
unsigned long irq_flags;

if (!intr)
@@ -1383,8 +1384,6 @@ static void dpu_hw_intr_dispatch_irq(struct
dpu_hw_intr *intr,
 */
spin_lock_irqsave(>irq_lock, irq_flags);
for (reg_idx = 0; reg_idx < ARRAY_SIZE(dpu_intr_set); reg_idx++) {
-   irq_status = intr->save_irq_status[reg_idx];
-
/*
 * Each Interrupt register has a range of 64 indexes, and
 * that is static for dpu_irq_map.
@@ -1396,6 +1395,20 @@ static void dpu_hw_intr_dispatch_irq(struct
dpu_hw_intr *intr,
start_idx >= ARRAY_SIZE(dpu_irq_map))
continue;

+   /* Read interrupt status */
+		irq_status = DPU_REG_READ(>hw, 
dpu_intr_set[reg_idx].status_off);

+
+   /* Read enable mask */
+   enable_mask = DPU_REG_READ(>hw, 
dpu_intr_set[reg_idx].en_off);
+
+   /* and clear the interrupt */
+   if (irq_status)
+   DPU_REG_WRITE(>hw, dpu_intr_set[reg_idx].clr_off,
+irq_status);
+
+   /* Finally update IRQ status based on enable mask */
+   irq_status &= enable_mask;
+
/*
 * Search through matching intr status from irq map.
 * start_idx and end_idx defined the search range in
@@ -1429,6 +1442,10 @@ static void dpu_hw_intr_dispatch_irq(struct
dpu_hw_intr *intr,
irq_status &= ~dpu_irq_map[irq_idx].irq_mask;
}
}
+
+   /* ensure register writes go through */
+   wmb();
+
spin_unlock_irqrestore(>irq_lock, irq_flags);
 }

@@ -1580,41 +1597,6 @@ static int dpu_hw_intr_disable_irqs(struct
dpu_hw_intr *intr)
return 0;
 }

-static void dpu_hw_intr_get_interrupt_statuses(struct dpu_hw_intr 
*intr)

-{
-   int i;
-   u32 enable_mask;
-   unsigned long irq_flags;
-
-   if (!intr)
-   return;
-
-   spin_lock_irqsave(>irq_lock, irq_flags);
-   for (i = 0; i < ARRAY_SIZE(dpu_intr_set); i++) {
-   if (!test_bit(i, >irq_mask))
-   continue;
-
-   /* Read interrupt status */
-   intr->save_irq_status[i] = DPU_REG_READ(>hw,
-   dpu_intr_set[i].status_off);
-
-   /* Read enable mask */
-   enable_mask = DPU_REG_READ(>hw, dpu_intr_set[i].en_off);
-
-   /* and clear the interrupt */
-   if (intr->save_irq_status[i])
-   DPU_REG_WRITE(>hw, dpu_intr_set[i].clr_off,
-

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

2021-05-24 Thread khsieh


On 2021-05-07 14:25, Stephen Boyd wrote:

Let's look at the irq status bits after a transfer and see if we got a
nack or a defer or a timeout, instead of telling drm layers that
everything was fine, while still printing an error message. I wasn't
sure about NACK+DEFER so I lumped all those various errors along with a
nack so that the drm core can figure out that things are just not going
well. The important thing is that we're now returning -ETIMEDOUT when
the message times out and nacks for bad addresses.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 


Reviewed-by: Kuogee Hsieh 


---
 drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++--
 drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
 2 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index b49810396513..4a3293b590b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -9,7 +9,15 @@
 #include "dp_reg.h"
 #include "dp_aux.h"

-#define DP_AUX_ENUM_STR(x) #x
+enum msm_dp_aux_err {
+   DP_AUX_ERR_NONE,
+   DP_AUX_ERR_ADDR,
+   DP_AUX_ERR_TOUT,
+   DP_AUX_ERR_NACK,
+   DP_AUX_ERR_DEFER,
+   DP_AUX_ERR_NACK_DEFER,
+   DP_AUX_ERR_PHY,
+};

 struct dp_aux_private {
struct device *dev;
@@ -18,7 +26,7 @@ struct dp_aux_private {
struct mutex mutex;
struct completion comp;

-   u32 aux_error_num;
+   enum msm_dp_aux_err aux_error_num;
u32 retry_cnt;
bool cmd_busy;
bool native;
@@ -33,62 +41,45 @@ struct dp_aux_private {

 #define MAX_AUX_RETRIES5

-static const char *dp_aux_get_error(u32 aux_error)
-{
-   switch (aux_error) {
-   case DP_AUX_ERR_NONE:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
-   case DP_AUX_ERR_ADDR:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
-   case DP_AUX_ERR_TOUT:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
-   case DP_AUX_ERR_NACK:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
-   case DP_AUX_ERR_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
-   case DP_AUX_ERR_NACK_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
-   default:
-   return "unknown";
-   }
-}
-
-static u32 dp_aux_write(struct dp_aux_private *aux,
+static ssize_t dp_aux_write(struct dp_aux_private *aux,
struct drm_dp_aux_msg *msg)
 {
-   u32 data[4], reg, len;
+   u8 data[4];
+   u32 reg;
+   ssize_t len;
u8 *msgdata = msg->buffer;
int const AUX_CMD_FIFO_LEN = 128;
int i = 0;

if (aux->read)
-   len = 4;
+   len = 0;
else
-   len = msg->size + 4;
+   len = msg->size;

/*
 * cmd fifo only has depth of 144 bytes
 * limit buf length to 128 bytes here
 */
-   if (len > AUX_CMD_FIFO_LEN) {
+   if (len > AUX_CMD_FIFO_LEN - 4) {
DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
-   return 0;
+   return -EINVAL;
}

/* Pack cmd and write to HW */
-   data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
+   data[0] = (msg->address >> 16) & 0xf;  /* addr[19:16] */
if (aux->read)
-   data[0] |=  BIT(4); /* R/W */
+   data[0] |=  BIT(4); /* R/W */

-   data[1] = (msg->address >> 8) & 0xff;  /* addr[15:8] */
-   data[2] = msg->address & 0xff;   /* addr[7:0] */
-   data[3] = (msg->size - 1) & 0xff;/* len[7:0] */
+   data[1] = msg->address >> 8;   /* addr[15:8] */
+   data[2] = msg->address;  /* addr[7:0] */
+   data[3] = msg->size - 1; /* len[7:0] */

-   for (i = 0; i < len; i++) {
+   for (i = 0; i < len + 4; i++) {
reg = (i < 4) ? data[i] : msgdata[i - 4];
+   reg <<= DP_AUX_DATA_OFFSET;
+   reg &= DP_AUX_DATA_MASK;
+   reg |= DP_AUX_DATA_WRITE;
/* index = 0, write */
-   reg = (((reg) << DP_AUX_DATA_OFFSET)
-  & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
if (i == 0)
reg |= DP_AUX_DATA_INDEX_WRITE;
aux->catalog->aux_data = reg;
@@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private 
*aux,

return len;
 }

-static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
  struct drm_dp_aux_msg *msg)
 {
-   u32 ret, len, timeout;
-   int aux_timeout_ms = HZ/4;
+   ssize_t ret;
+   unsigned long time_left;

reinit_completion(>comp);

-   len = dp_aux_write(aux,

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

2021-05-24 Thread khsieh

On 2021-05-24 12:19, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-05-24 09:33:49)

On 2021-05-07 14:25, Stephen Boyd wrote:
> @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
>   }
>
>   ret = dp_aux_cmd_fifo_tx(aux, msg);
> -
>   if (ret < 0) {
>   if (aux->native) {
>   aux->retry_cnt++;
>   if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>   dp_catalog_aux_update_cfg(aux->catalog);
>   }
> - usleep_range(400, 500); /* at least 400us to next try */
> - goto unlock_exit;
> - }

1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
dp_catalog_aux_reset(aux->catalog);
dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
potentially cause pending hpd interrupts got lost.
Therefore I think we should not do
dp_catalog_aux_update_cfg(aux->catalog) for now.
reset aux controller will reset hpd control block probolem will be 
fixed

at next chipset.
after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
by dp_catalog_aux_reset(aux->catalog) back at next chipset.

Hmm ok. So the phy calibration logic that tweaks the tuning values is
never used? Why can't the phy be tuned while it is active? I don't
understand why we would ever want to reset the aux phy when changing 
the

settings for a retry. Either way, this is not actually changing in this
patch so it would be another patch to remove this code.

ok,

2) according to DP specification, aux read/write failed have to wait 
at

least 400us before next try can start.
Otherwise, DP compliant test will failed

Yes. The caller of this function, drm_dp_dpcd_access(), has the delay
already

if (ret != 0 && ret != -ETIMEDOUT) {
usleep_range(AUX_RETRY_INTERVAL,
 AUX_RETRY_INTERVAL + 100);
}

so this delay here is redundant.

yes, you are right. This is enough.

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

2021-05-24 Thread Stephen Boyd

Quoting khs...@codeaurora.org (2021-05-24 09:33:49)
> On 2021-05-07 14:25, Stephen Boyd wrote:
> > @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> > *dp_aux,
> >   }
> >
> >   ret = dp_aux_cmd_fifo_tx(aux, msg);
> > -
> >   if (ret < 0) {
> >   if (aux->native) {
> >   aux->retry_cnt++;
> >   if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> >   dp_catalog_aux_update_cfg(aux->catalog);
> >   }
> > - usleep_range(400, 500); /* at least 400us to next try */
> > - goto unlock_exit;
> > - }
>
> 1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
> dp_catalog_aux_reset(aux->catalog);
> dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
> potentially cause pending hpd interrupts got lost.
> Therefore I think we should not do
> dp_catalog_aux_update_cfg(aux->catalog) for now.
> reset aux controller will reset hpd control block probolem will be fixed
> at next chipset.
> after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
> by dp_catalog_aux_reset(aux->catalog) back at next chipset.

Hmm ok. So the phy calibration logic that tweaks the tuning values is
never used? Why can't the phy be tuned while it is active? I don't
understand why we would ever want to reset the aux phy when changing the
settings for a retry. Either way, this is not actually changing in this
patch so it would be another patch to remove this code.

>
> 2) according to DP specification, aux read/write failed have to wait at
> least 400us before next try can start.
> Otherwise, DP compliant test will failed

Yes. The caller of this function, drm_dp_dpcd_access(), has the delay
already

if (ret != 0 && ret != -ETIMEDOUT) {
usleep_range(AUX_RETRY_INTERVAL,
 AUX_RETRY_INTERVAL + 100);
}

so this delay here is redundant.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

2021-05-24 Thread khsieh


On 2021-05-07 14:25, Stephen Boyd wrote:

Let's look at the irq status bits after a transfer and see if we got a
nack or a defer or a timeout, instead of telling drm layers that
everything was fine, while still printing an error message. I wasn't
sure about NACK+DEFER so I lumped all those various errors along with a
nack so that the drm core can figure out that things are just not going
well. The important thing is that we're now returning -ETIMEDOUT when
the message times out and nacks for bad addresses.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 
---
 drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++--
 drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
 2 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index b49810396513..4a3293b590b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -9,7 +9,15 @@
 #include "dp_reg.h"
 #include "dp_aux.h"

-#define DP_AUX_ENUM_STR(x) #x
+enum msm_dp_aux_err {
+   DP_AUX_ERR_NONE,
+   DP_AUX_ERR_ADDR,
+   DP_AUX_ERR_TOUT,
+   DP_AUX_ERR_NACK,
+   DP_AUX_ERR_DEFER,
+   DP_AUX_ERR_NACK_DEFER,
+   DP_AUX_ERR_PHY,
+};

 struct dp_aux_private {
struct device *dev;
@@ -18,7 +26,7 @@ struct dp_aux_private {
struct mutex mutex;
struct completion comp;

-   u32 aux_error_num;
+   enum msm_dp_aux_err aux_error_num;
u32 retry_cnt;
bool cmd_busy;
bool native;
@@ -33,62 +41,45 @@ struct dp_aux_private {

 #define MAX_AUX_RETRIES5

-static const char *dp_aux_get_error(u32 aux_error)
-{
-   switch (aux_error) {
-   case DP_AUX_ERR_NONE:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
-   case DP_AUX_ERR_ADDR:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
-   case DP_AUX_ERR_TOUT:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
-   case DP_AUX_ERR_NACK:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
-   case DP_AUX_ERR_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
-   case DP_AUX_ERR_NACK_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
-   default:
-   return "unknown";
-   }
-}
-
-static u32 dp_aux_write(struct dp_aux_private *aux,
+static ssize_t dp_aux_write(struct dp_aux_private *aux,
struct drm_dp_aux_msg *msg)
 {
-   u32 data[4], reg, len;
+   u8 data[4];
+   u32 reg;
+   ssize_t len;
u8 *msgdata = msg->buffer;
int const AUX_CMD_FIFO_LEN = 128;
int i = 0;

if (aux->read)
-   len = 4;
+   len = 0;
else
-   len = msg->size + 4;
+   len = msg->size;

/*
 * cmd fifo only has depth of 144 bytes
 * limit buf length to 128 bytes here
 */
-   if (len > AUX_CMD_FIFO_LEN) {
+   if (len > AUX_CMD_FIFO_LEN - 4) {
DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
-   return 0;
+   return -EINVAL;
}

/* Pack cmd and write to HW */
-   data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
+   data[0] = (msg->address >> 16) & 0xf;  /* addr[19:16] */
if (aux->read)
-   data[0] |=  BIT(4); /* R/W */
+   data[0] |=  BIT(4); /* R/W */

-   data[1] = (msg->address >> 8) & 0xff;  /* addr[15:8] */
-   data[2] = msg->address & 0xff;   /* addr[7:0] */
-   data[3] = (msg->size - 1) & 0xff;/* len[7:0] */
+   data[1] = msg->address >> 8;   /* addr[15:8] */
+   data[2] = msg->address;  /* addr[7:0] */
+   data[3] = msg->size - 1; /* len[7:0] */

-   for (i = 0; i < len; i++) {
+   for (i = 0; i < len + 4; i++) {
reg = (i < 4) ? data[i] : msgdata[i - 4];
+   reg <<= DP_AUX_DATA_OFFSET;
+   reg &= DP_AUX_DATA_MASK;
+   reg |= DP_AUX_DATA_WRITE;
/* index = 0, write */
-   reg = (((reg) << DP_AUX_DATA_OFFSET)
-  & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
if (i == 0)
reg |= DP_AUX_DATA_INDEX_WRITE;
aux->catalog->aux_data = reg;
@@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private 
*aux,

return len;
 }

-static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
  struct drm_dp_aux_msg *msg)
 {
-   u32 ret, len, timeout;
-   int aux_timeout_ms = HZ/4;
+   ssize_t ret;
+   unsigned long time_left;

reinit_completion(>comp);

-   len = dp_aux_write(aux, msg);
-   if (len == 0) {
-

Re: [Freedreno] [PATCH 2/3] drm/msm/dp: Shrink locking area of dp_aux_transfer()

2021-05-24 Thread khsieh


On 2021-05-07 14:25, Stephen Boyd wrote:

We don't need to hold the lock to inspect the message we're going to
transfer, and we don't need to clear the busy flag either. Take the 
lock

later and bail out earlier if conditions aren't met.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 


Reviewed-by: Kuogee Hsieh 

---
 drivers/gpu/drm/msm/dp/dp_aux.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 91188466cece..b49810396513 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -329,30 +329,29 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

ssize_t ret;
int const aux_cmd_native_max = 16;
int const aux_cmd_i2c_max = 128;
-   struct dp_aux_private *aux = container_of(dp_aux,
-   struct dp_aux_private, dp_aux);
+   struct dp_aux_private *aux;

-   mutex_lock(>mutex);
+   aux = container_of(dp_aux, struct dp_aux_private, dp_aux);

 	aux->native = msg->request & (DP_AUX_NATIVE_WRITE & 
DP_AUX_NATIVE_READ);


/* Ignore address only message */
-   if ((msg->size == 0) || (msg->buffer == NULL)) {
+   if (msg->size == 0 || !msg->buffer) {
msg->reply = aux->native ?
DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
-   ret = msg->size;
-   goto unlock_exit;
+   return msg->size;
}

/* msg sanity check */
-   if ((aux->native && (msg->size > aux_cmd_native_max)) ||
-   (msg->size > aux_cmd_i2c_max)) {
+   if ((aux->native && msg->size > aux_cmd_native_max) ||
+   msg->size > aux_cmd_i2c_max) {
DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
__func__, msg->size, msg->request);
-   ret = -EINVAL;
-   goto unlock_exit;
+   return -EINVAL;
}

+   mutex_lock(>mutex);
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 1/3] drm/msm/dp: Simplify aux irq handling code

2021-05-24 Thread khsieh


On 2021-05-07 14:25, Stephen Boyd wrote:

We don't need to stash away 'isr' in the aux structure to pass to two
functions. Let's use a local variable instead. And we can complete the
completion variable in one place instead of two to simplify the code.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 


Reviewed-by: Kuogee Hsieh 

---
 drivers/gpu/drm/msm/dp/dp_aux.c | 22 --
 drivers/gpu/drm/msm/dp/dp_catalog.c |  2 +-
 drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
 3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 7c22bfe0fc7d..91188466cece 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -27,7 +27,6 @@ struct dp_aux_private {
bool no_send_stop;
u32 offset;
u32 segment;
-   u32 isr;

struct drm_dp_aux dp_aux;
 };
@@ -181,10 +180,8 @@ static void dp_aux_cmd_fifo_rx(struct 
dp_aux_private *aux,

}
 }

-static void dp_aux_native_handler(struct dp_aux_private *aux)
+static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
 {
-   u32 isr = aux->isr;
-
if (isr & DP_INTR_AUX_I2C_DONE)
aux->aux_error_num = DP_AUX_ERR_NONE;
else if (isr & DP_INTR_WRONG_ADDR)
@@ -197,14 +194,10 @@ static void dp_aux_native_handler(struct
dp_aux_private *aux)
aux->aux_error_num = DP_AUX_ERR_PHY;
dp_catalog_aux_clear_hw_interrupts(aux->catalog);
}
-
-   complete(>comp);
 }

-static void dp_aux_i2c_handler(struct dp_aux_private *aux)
+static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
 {
-   u32 isr = aux->isr;
-
if (isr & DP_INTR_AUX_I2C_DONE) {
if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
aux->aux_error_num = DP_AUX_ERR_NACK;
@@ -226,8 +219,6 @@ static void dp_aux_i2c_handler(struct 
dp_aux_private *aux)

dp_catalog_aux_clear_hw_interrupts(aux->catalog);
}
}
-
-   complete(>comp);
 }

 static void dp_aux_update_offset_and_segment(struct dp_aux_private 
*aux,
@@ -412,6 +403,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,


 void dp_aux_isr(struct drm_dp_aux *dp_aux)
 {
+   u32 isr;
struct dp_aux_private *aux;

if (!dp_aux) {
@@ -421,15 +413,17 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)

aux = container_of(dp_aux, struct dp_aux_private, dp_aux);

-   aux->isr = dp_catalog_aux_get_irq(aux->catalog);
+   isr = dp_catalog_aux_get_irq(aux->catalog);

if (!aux->cmd_busy)
return;

if (aux->native)
-   dp_aux_native_handler(aux);
+   dp_aux_native_handler(aux, isr);
else
-   dp_aux_i2c_handler(aux);
+   dp_aux_i2c_handler(aux, isr);
+
+   complete(>comp);
 }

 void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b1a9b1b98f5f..a70c238f34b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -292,7 +292,7 @@ void dp_catalog_dump_regs(struct dp_catalog 
*dp_catalog)

dump_regs(catalog->io->dp_controller.base + offset, len);
 }

-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
 {
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 176a9020a520..502bc0dc7787 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -80,7 +80,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct
dp_catalog *dp_catalog);
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool 
enable);

 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);

 /* DP Controller APIs */
 void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 
state);

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

2021-05-24 Thread Bjorn Andersson

On Mon 24 May 02:30 CDT 2021, Vinod Koul wrote:

> On 21-05-21, 09:42, Bjorn Andersson wrote:
> > On Fri 21 May 07:49 CDT 2021, Vinod Koul wrote:
> > 
> > > DSC enables streams to be compressed before we send to panel. This
> > > requires DSC enabled encoder and a panel to be present. So we add this
> > > information in board DTS and find if DSC can be enabled and the
> > > parameters required to configure DSC are added to binding document along
> > > with example
> > > 
> > > Signed-off-by: Vinod Koul 
> > > ---
> > >  .../devicetree/bindings/display/msm/dsi.txt   | 15 +++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
> > > b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > index b9a64d3ff184..83d2fb92267e 100644
> > > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > @@ -48,6 +48,13 @@ Optional properties:
> > >  - pinctrl-n: the "sleep" pinctrl state
> > >  - ports: contains DSI controller input and output ports as children, each
> > >containing one endpoint subnode.
> > > +- qcom,mdss-dsc-enabled: Display Stream Compression (DSC) is enabled
> > > +- qcom,mdss-slice-height: DSC slice height in pixels
> > > +- qcom,mdss-slice-width: DSC slice width in pixels
> > > +- qcom,mdss-slice-per-pkt: DSC slices per packet
> > > +- qcom,mdss-bit-per-component: DSC bits per component
> > > +- qcom,mdss-bit-per-pixel: DSC bits per pixel
> > > +- qcom,mdss-block-prediction-enable: Block prediction mode of DSC enabled
> > >  
> > >DSI Endpoint properties:
> > >- remote-endpoint: For port@0, set to phandle of the connected 
> > > panel/bridge's
> > > @@ -188,6 +195,14 @@ Example:
> > >   qcom,master-dsi;
> > >   qcom,sync-dual-dsi;
> > >  
> > > + qcom,mdss-dsc-enabled;
> > 
> > To me the activation of DSC seems to be a property of the panel.
> 
> I think there are three parts to the problem
> 1. Panel needs to support it

In the case of DP there's bits to be read in the panel to figure this
out, for DSI panels this seems like a property that the panel (driver)
should know about.

> 2. Host needs to support it

Right, so this needs to be known by the driver. My suggestion is that we
derive it from the compatible or from the HW version.

> 3. Someone needs to decide to use when both the above conditions are
> met.
> 
> There are cases where above 1, 2 will be satisfied, but we might be okay
> without DSC too.. so how to decide when to do DSC :)
> 

Can we describe those cases? E.g. is it because enabling DSC would not
cause a reduction in clock speed that's worth the effort? Or do we only
use DSC for DSI when it allows us to squeeze everything into a single
link?

Regards,
Bjorn

> I feel it is more of a system property. And I also think that these
> parameters here are host configuration and not really for panel...
> 
> > 
> > > + qcom,mdss-slice-height = <16>;
> > > + qcom,mdss-slice-width = <540>;
> > > + qcom,mdss-slice-per-pkt = <1>;
> > > + qcom,mdss-bit-per-component = <8>;
> > > + qcom,mdss-bit-per-pixel = <8>;
> > > + qcom,mdss-block-prediction-enable;
> > 
> > Which of these properties relates to the DSC encoder and what needs to
> > be agreed with the sink? Can't we derive e.g. bpp from the information
> > we have from the attached panel already?
> 
> Let me go back and check on this a bit more
> 
> Thanks
> -- 
> ~Vinod
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH v18 1/4] dt-bindings: msm: disp: add yaml schemas for DPU bindings

2021-05-24 Thread Krishna Manikandan

MSM Mobile Display Subsystem (MDSS) encapsulates sub-blocks
like DPU display controller, DSI etc. Add YAML schema
for DPU device tree bindings.

Signed-off-by: Krishna Manikandan 
Reviewed-by: Rob Herring 
Reviewed-by: Bjorn Andersson 
Reviewed-by: Stephen Boyd 
---
Changes in v2:
- Changed dpu to DPU (Sam Ravnborg)
- Fixed indentation issues (Sam Ravnborg)
- Added empty line between different properties (Sam Ravnborg)
- Replaced reference txt files with  their corresponding
  yaml files (Sam Ravnborg)
- Modified the file to use "|" only when it is
  necessary (Sam Ravnborg)

Changes in v3:
- Corrected the license used (Rob Herring)
- Added maxItems for properties (Rob Herring)
- Dropped generic descriptions (Rob Herring)
- Added ranges property (Rob Herring)
- Corrected the indendation (Rob Herring)
- Added additionalProperties (Rob Herring)
- Split dsi file into two, one for dsi controller
  and another one for dsi phy per target (Rob Herring)
- Corrected description for pinctrl-names (Rob Herring)
- Corrected the examples used in yaml file (Rob Herring)
- Delete dsi.txt and dpu.txt (Rob Herring)

Changes in v4:
- Move schema up by one level (Rob Herring)
- Add patternProperties for mdp node (Rob Herring)
- Corrected description of some properties (Rob Herring)

Changes in v5:
- Correct the indentation (Rob Herring)
- Remove unnecessary description from properties (Rob Herring)
- Correct the number of interconnect entries (Rob Herring)
- Add interconnect names for sc7180 (Rob Herring)
- Add description for ports (Rob Herring)
- Remove common properties (Rob Herring)
- Add unevalutatedProperties (Rob Herring)
- Reference existing dsi controller yaml in the common
  dsi controller file (Rob Herring)
- Correct the description of clock names to include only the
  clocks that are required (Rob Herring)
- Remove properties which are already covered under the common
  binding (Rob Herring)
- Add dsi phy supply nodes which are required for sc7180 and
  sdm845 targets (Rob Herring)
- Add type ref for syscon-sfpb (Rob Herring)

Changes in v6:
- Fixed errors during dt_binding_check (Rob Herring)
- Add maxItems for phys and phys-names (Rob Herring)
- Use unevaluatedProperties wherever required (Rob Herring)
- Removed interrupt controller from required properties for
  dsi controller (Rob Herring)
- Add constraints for dsi-phy reg-names based on the compatible
  phy version (Rob Herring)
- Add constraints for dsi-phy supply nodes based on the
  compatible phy version (Rob Herring)

Changes in v7:
- Add default value for qcom,mdss-mdp-transfer-time-us (Rob Herring)
- Modify the schema for data-lanes (Rob Herring)
- Split the phy schema into separate schemas based on
  the phy version (Rob Herring)

Changes in v8:
- Resolve merge conflicts with latest dsi.txt file
- Include dp yaml change also in the same series

Changes in v9:
- Combine target specific dsi controller yaml files
  to a single yaml file (Rob Herring)
- Combine target specific dsi phy yaml files into a
  single yaml file (Rob Herring)
- Use unevaluatedProperties and additionalProperties
  wherever required
- Remove duplicate properties from common yaml files

Changes in v10:
- Split the patch into separate patches for DPU, DSI and
  PHY (Stephen Boyd)
- Drop unnecessary fullstop (Stephen Boyd)
- Add newline whereever required (Stephen Boyd)
- Add description for clock used (Stephen Boyd)
- Modify the description for interconnect entries  (Stephen Boyd)
- Drop assigned clock entries as it a generic property (Stephen Boyd)
- Correct the definition for interrupts (Stephen Boyd)
- Drop clock names from required properties (Stephen Boyd)
- Drop labels for display nodes from example (Stephen Boyd)
- Drop flags from interrupts entries (Stephen Boyd)

Changes in v11:
- Drop maxItems for clocks (Stephen Boyd)

Changes in v12:
- Add description for register property (Stephen Boyd)
- Add maxItems for interrupts (Stephen Boyd)
- Add description for iommus property (Stephen Boyd)
- Add description for interconnects (Stephen Boyd)
- Change display node name to display_controller (Stephen Boyd)

Changes in v13:
- Add maxItems for reg property (Stephen Boyd)
- Add ranges property in example (Stephen Boyd)
- Modify description for iommus property (Stephen Boyd)
- Add Dp bindings for ports in the same patch (Stephen Boyd)
- Remove soc from examples and change address and size cells
  accordingly (Stephen Boyd)
- Add reference for ports

Changes in v14:
- Modify title for SC7180 and SDM845 yaml files (Stephen Boyd)
- Add required list for display-controller node (Stephen Boyd)

Changes in v16:
- Add reference for port (Rob Herring)

[Freedreno] [PATCH v18 2/4] dt-bindings: msm: dsi: add yaml schemas for DSI bindings

2021-05-24 Thread Krishna Manikandan

Add YAML schema for the device tree bindings for DSI

Signed-off-by: Krishna Manikandan 
Reviewed-by: Bjorn Andersson 
Reviewed-by: Stephen Boyd 
---
Changes in v1:
- Separate dsi controller bindings to a separate patch (Stephen Boyd)
- Merge dsi-common-controller.yaml and dsi-controller-main.yaml to
  a single file (Stephen Boyd)
- Drop supply entries and definitions from properties (Stephen Boyd)
- Modify phy-names property for dsi controller (Stephen Boyd)
- Remove boolean from description (Stephen Boyd)
- Drop pinctrl properties as they are standard entries (Stephen Boyd)
- Modify the description for ports property and keep the reference
  to the generic binding where this is defined (Stephen Boyd)
- Add description to clock names (Stephen Boyd)
- Correct the indendation (Stephen Boyd)
- Drop the label for display dt nodes and correct the node
  name (Stephen Boyd)

Changes in v2:
- Drop maxItems for clock (Stephen Boyd)
- Drop qcom,mdss-mdp-transfer-time-us as it is not used in upstream
  dt file (Stephen Boyd)
- Keep child node directly under soc node (Stephen Boyd)
- Drop qcom,sync-dual-dsi as it is not used in upstream dt

Changes in v3:
- Add description for register property (Stephen Boyd)

Changes in v4:
- Add maxItems for phys property (Stephen Boyd)
- Add maxItems for reg property (Stephen Boyd)
- Add reference for data-lanes property (Stephen Boyd)
- Remove soc from example (Stephen Boyd)

Changes in v5:
- Modify title and description (Stephen Boyd)
- Add required properties for ports node (Stephen Boyd)
- Add data-lanes in the example (Stephen Boyd)
- Drop qcom,master-dsi property (Stephen Boyd)

Changes in v6:
- Add required properties for port@0, port@1 and corresponding
  endpoints (Stephen Boyd)
- Add address-cells and size-cells for ports (Stephen Boyd)
- Use additionalProperties instead of unevaluatedProperties (Stephen Boyd)

Changes in v7:
- Add reference for ports and data-lanes (Rob Herring)
- Add maxItems and minItems for data-lanes (Rob Herring)

Changes in v8:
- Drop common properties and description from ports (Rob Herring)
- Add reference for endpoint (Rob Herring)
- Add correct reference for data-lanes (Rob Herring)
- Drop common properties from required list for ports (Rob Herring)

Changes in v9:
- Drop reference for data-lanes (Rob Herring)
- Add unevaluatedProperties for endpoint (Rob Herring)

 .../bindings/display/msm/dsi-controller-main.yaml  | 185 +++
 .../devicetree/bindings/display/msm/dsi.txt| 249 -
 2 files changed, 185 insertions(+), 249 deletions(-)
 create mode 100644 
Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/msm/dsi.txt

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
new file mode 100644
index 000..76348b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -0,0 +1,185 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dsi-controller-main.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display DSI controller
+
+maintainers:
+  - Krishna Manikandan 
+
+allOf:
+  - $ref: "../dsi-controller.yaml#"
+
+properties:
+  compatible:
+items:
+  - const: qcom,mdss-dsi-ctrl
+
+  reg:
+maxItems: 1
+
+  reg-names:
+const: dsi_ctrl
+
+  interrupts:
+maxItems: 1
+
+  clocks:
+items:
+  - description: Display byte clock
+  - description: Display byte interface clock
+  - description: Display pixel clock
+  - description: Display escape clock
+  - description: Display AHB clock
+  - description: Display AXI clock
+
+  clock-names:
+items:
+  - const: byte
+  - const: byte_intf
+  - const: pixel
+  - const: core
+  - const: iface
+  - const: bus
+
+  phys:
+maxItems: 1
+
+  phy-names:
+const: dsi
+
+  "#address-cells": true
+
+  "#size-cells": true
+
+  syscon-sfpb:
+description: A phandle to mmss_sfpb syscon node (only for DSIv2).
+$ref: "/schemas/types.yaml#/definitions/phandle"
+
+  qcom,dual-dsi-mode:
+type: boolean
+description: |
+  Indicates if the DSI controller is driving a panel which needs
+  2 DSI links.
+
+  power-domains:
+maxItems: 1
+
+  operating-points-v2: true
+
+  ports:
+$ref: "/schemas/graph.yaml#/properties/ports"
+description: |
+  Contains DSI controller input and output ports as children, each
+  containing one endpoint subnode.
+
+properties:
+  port@0:
+$ref: "/schemas/graph.yaml#/properties/port"
+description: |
+  Input endpoints of the controller.
+

[Freedreno] [PATCH v18 4/4] dt-bindings: msm/dp: Add bindings of MSM DisplayPort controller

2021-05-24 Thread Krishna Manikandan

Add bindings for Snapdragon DisplayPort controller driver.

Signed-off-by: Chandan Uddaraju 
Signed-off-by: Vara Reddy 
Signed-off-by: Tanmay Shah 
Signed-off-by: Kuogee Hsieh 
Signed-off-by: Krishna Manikandan 
Reviewed-by: Bjorn Andersson 
Reviewed-by: Rob Herring 
Reviewed-by: Stephen Boyd 
---
Changes in V2:
-Provide details about sel-gpio

Changes in V4:
-Provide details about max dp lanes
-Change the commit text

Changes in V5:
-moved dp.txt to yaml file

Changes in v6:
- Squash all AUX LUT properties into one pattern Property
- Make aux-cfg[0-9]-settings properties optional
- Remove PLL/PHY bindings from DP controller dts
- Add DP clocks description
- Remove _clk suffix from clock names
- Rename pixel clock to stream_pixel
- Remove redundant bindings (GPIO, PHY, HDCP clock, etc..)
- Fix indentation
- Add Display Port as interface of DPU in DPU bindings
  and add port mapping accordingly.

Chages in v7:
- Add dp-controller.yaml file common between multiple SOC
- Rename dp-sc7180.yaml to dp-controller-sc7180.yaml
- change compatible string and add SOC name to it.
- Remove Root clock generator for pixel clock
- Add assigned-clocks and assigned-clock-parents bindings
- Remove redundant properties, descriptions and blank lines
- Add DP port in DPU bindings
- Update depends-on tag in commit message and rebase change accordingly

Changes in v8:
- Add MDSS AHB clock in bindings

Changes in v9:
- Remove redundant reg-name property
- Change assigned-clocks and assigned-clocks-parents counts to 2
- Use IRQ flags in example dts

Changes in v10:
- Change title of this patch as it does not contain PLL bindings anymore
- Remove redundant properties
- Remove use of IRQ flag
- Fix ports property

Changes in v11:
- add ports required of both #address-cells and  #size-cells
- add required operating-points-v2
- add required #sound-dai-cells
- add required power-domains
- update maintainer list

Changes in v12:
- remove soc node from examples (Stephen Boyd)
- split dpu-sc7180.yaml changes to separate patch (Stephen Boyd)

Changes in v13:
- add assigned-clocks
- add assigned-clock-parents

Changes in v14:
- add reference for ports (Rob Herring)

Changes in v15:
- drop common properties from ports (Rob Herring)

 .../bindings/display/msm/dp-controller.yaml| 146 +
 1 file changed, 146 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/msm/dp-controller.yaml

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
new file mode 100644
index 000..64d8d9e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dp-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MSM Display Port Controller
+
+maintainers:
+  - Kuogee Hsieh 
+
+description: |
+  Device tree bindings for DisplayPort host controller for MSM targets
+  that are compatible with VESA DisplayPort interface specification.
+
+properties:
+  compatible:
+enum:
+  - qcom,sc7180-dp
+
+  reg:
+maxItems: 1
+
+  interrupts:
+maxItems: 1
+
+  clocks:
+items:
+  - description: AHB clock to enable register access
+  - description: Display Port AUX clock
+  - description: Display Port Link clock
+  - description: Link interface clock between DP and PHY
+  - description: Display Port Pixel clock
+
+  clock-names:
+items:
+  - const: core_iface
+  - const: core_aux
+  - const: ctrl_link
+  - const: ctrl_link_iface
+  - const: stream_pixel
+
+  assigned-clocks:
+items:
+  - description: link clock source
+  - description: pixel clock source
+
+  assigned-clock-parents:
+items:
+  - description: phy 0 parent
+  - description: phy 1 parent
+
+  phys:
+maxItems: 1
+
+  phy-names:
+items:
+  - const: dp
+
+  operating-points-v2:
+maxItems: 1
+
+  power-domains:
+maxItems: 1
+
+  "#sound-dai-cells":
+const: 0
+
+  ports:
+$ref: /schemas/graph.yaml#/properties/ports
+properties:
+  port@0:
+$ref: /schemas/graph.yaml#/properties/port
+description: Input endpoint of the controller
+
+  port@1:
+$ref: /schemas/graph.yaml#/properties/port
+description: Output endpoint of the controller
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - phys
+  - phy-names
+  - "#sound-dai-cells"
+  - power-domains
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+#include 
+#include 
+#include 
+#include 
+
+displayport-controller@ae9 {
+compatible = "qcom,sc7180-dp";
+reg = <0xae9 0x1400>;
+interrupt-parent = <>;
+interrupts = <12>;
+clocks = < DISP_CC_MDSS_AHB_CLK>,
+ <

[Freedreno] [PATCH v18 3/4] dt-bindings: msm: dsi: add yaml schemas for DSI PHY bindings

2021-05-24 Thread Krishna Manikandan

Add YAML schema for the device tree bindings for DSI PHY.

Signed-off-by: Krishna Manikandan 
Reviewed-by: Bjorn Andersson 
Reviewed-by: Stephen Boyd 
Reviewed-by: Rob Herring 
---
Changes in v1:
   - Merge dsi-phy.yaml and dsi-phy-10nm.yaml (Stephen Boyd)
   - Remove qcom,dsi-phy-regulator-ldo-mode (Stephen Boyd)
   - Add clock cells properly (Stephen Boyd)
   - Remove unnecessary decription from clock names (Stephen Boyd)
   - Add pin names for the supply entries for 10nm phy which is
 used in sc7180 and sdm845 (Stephen Boyd)
   - Remove unused header files from examples (Stephen Boyd)
   - Drop labels for display nodes and correct node name (Stephen Boyd)

Changes in v2:
   - Drop maxItems for clock (Stephen Boyd)
   - Add vdds supply pin information for sdm845 (Stephen Boyd)
   - Add examples for 14nm, 20nm and 28nm phy yaml files (Stephen Boyd)
   - Keep child nodes directly under soc node (Stephen Boyd)

Changes in v3:
   - Use a separate yaml file to describe the common properties
 for all the dsi phy versions (Stephen Boyd)
   - Remove soc from examples (Stephen Boyd)
   - Add description for register property

Changes in v4:
   - Modify the title for all the phy versions (Stephen Boyd)
   - Drop description for all the phy versions (Stephen Boyd)
   - Modify the description for register property (Stephen Boyd)

Changes in v5:
   - Remove unused properties from common dsi phy file
   - Add clock-cells and phy-cells to required property
 list (Stephen Boyd)

Changes in v6:
   - Add proper compatible string in example
   
 .../bindings/display/msm/dsi-phy-10nm.yaml | 68 +
 .../bindings/display/msm/dsi-phy-14nm.yaml | 66 
 .../bindings/display/msm/dsi-phy-20nm.yaml | 71 ++
 .../bindings/display/msm/dsi-phy-28nm.yaml | 68 +
 .../bindings/display/msm/dsi-phy-common.yaml   | 40 
 5 files changed, 313 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
 create mode 100644 
Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml
 create mode 100644 
Documentation/devicetree/bindings/display/msm/dsi-phy-20nm.yaml
 create mode 100644 
Documentation/devicetree/bindings/display/msm/dsi-phy-28nm.yaml
 create mode 100644 
Documentation/devicetree/bindings/display/msm/dsi-phy-common.yaml

diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
new file mode 100644
index 000..4a26bef
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dsi-phy-10nm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display DSI 10nm PHY
+
+maintainers:
+  - Krishna Manikandan 
+
+allOf:
+  - $ref: dsi-phy-common.yaml#
+
+properties:
+  compatible:
+oneOf:
+  - const: qcom,dsi-phy-10nm
+  - const: qcom,dsi-phy-10nm-8998
+
+  reg:
+items:
+  - description: dsi phy register set
+  - description: dsi phy lane register set
+  - description: dsi pll register set
+
+  reg-names:
+items:
+  - const: dsi_phy
+  - const: dsi_phy_lane
+  - const: dsi_pll
+
+  vdds-supply:
+description: |
+  Connected to DSI0_MIPI_DSI_PLL_VDDA0P9 pin for sc7180 target and
+  connected to VDDA_MIPI_DSI_0_PLL_0P9 pin for sdm845 target
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - vdds-supply
+
+unevaluatedProperties: false
+
+examples:
+  - |
+ #include 
+ #include 
+
+ dsi-phy@ae94400 {
+ compatible = "qcom,dsi-phy-10nm";
+ reg = <0x0ae94400 0x200>,
+   <0x0ae94600 0x280>,
+   <0x0ae94a00 0x1e0>;
+ reg-names = "dsi_phy",
+ "dsi_phy_lane",
+ "dsi_pll";
+
+ #clock-cells = <1>;
+ #phy-cells = <0>;
+
+ vdds-supply = <_mipi_dsi0_pll>;
+ clocks = < DISP_CC_MDSS_AHB_CLK>,
+  < RPMH_CXO_CLK>;
+ clock-names = "iface", "ref";
+ };
+...
diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml
new file mode 100644
index 000..72a00cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dsi-phy-14nm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display DSI 14nm PHY
+
+maintainers:
+  - Krishna Manikandan 
+
+allOf:
+  - $ref: dsi-phy-common.yaml#
+
+properties:
+  compatible:
+oneOf:
+  - const: qcom,dsi-phy-14nm
+  - const: qcom,dsi-phy-14nm-660
+
+  reg:
+items:
+

Re: [Freedreno] [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

2021-05-24 Thread Vinod Koul

On 21-05-21, 09:42, Bjorn Andersson wrote:
> On Fri 21 May 07:49 CDT 2021, Vinod Koul wrote:
> 
> > DSC enables streams to be compressed before we send to panel. This
> > requires DSC enabled encoder and a panel to be present. So we add this
> > information in board DTS and find if DSC can be enabled and the
> > parameters required to configure DSC are added to binding document along
> > with example
> > 
> > Signed-off-by: Vinod Koul 
> > ---
> >  .../devicetree/bindings/display/msm/dsi.txt   | 15 +++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
> > b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > index b9a64d3ff184..83d2fb92267e 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > @@ -48,6 +48,13 @@ Optional properties:
> >  - pinctrl-n: the "sleep" pinctrl state
> >  - ports: contains DSI controller input and output ports as children, each
> >containing one endpoint subnode.
> > +- qcom,mdss-dsc-enabled: Display Stream Compression (DSC) is enabled
> > +- qcom,mdss-slice-height: DSC slice height in pixels
> > +- qcom,mdss-slice-width: DSC slice width in pixels
> > +- qcom,mdss-slice-per-pkt: DSC slices per packet
> > +- qcom,mdss-bit-per-component: DSC bits per component
> > +- qcom,mdss-bit-per-pixel: DSC bits per pixel
> > +- qcom,mdss-block-prediction-enable: Block prediction mode of DSC enabled
> >  
> >DSI Endpoint properties:
> >- remote-endpoint: For port@0, set to phandle of the connected 
> > panel/bridge's
> > @@ -188,6 +195,14 @@ Example:
> > qcom,master-dsi;
> > qcom,sync-dual-dsi;
> >  
> > +   qcom,mdss-dsc-enabled;
> 
> To me the activation of DSC seems to be a property of the panel.

I think there are three parts to the problem
1. Panel needs to support it
2. Host needs to support it
3. Someone needs to decide to use when both the above conditions are
met.

There are cases where above 1, 2 will be satisfied, but we might be okay
without DSC too.. so how to decide when to do DSC :)

I feel it is more of a system property. And I also think that these
parameters here are host configuration and not really for panel...

> 
> > +   qcom,mdss-slice-height = <16>;
> > +   qcom,mdss-slice-width = <540>;
> > +   qcom,mdss-slice-per-pkt = <1>;
> > +   qcom,mdss-bit-per-component = <8>;
> > +   qcom,mdss-bit-per-pixel = <8>;
> > +   qcom,mdss-block-prediction-enable;
> 
> Which of these properties relates to the DSC encoder and what needs to
> be agreed with the sink? Can't we derive e.g. bpp from the information
> we have from the attached panel already?

Let me go back and check on this a bit more

Thanks
-- 
~Vinod
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC PATCH 01/13] drm/dsc: Add dsc pps header init function

2021-05-24 Thread Vinod Koul

On 21-05-21, 17:29, Daniel Vetter wrote:
> On Fri, May 21, 2021 at 06:19:30PM +0530, Vinod Koul wrote:
> > We required a helper to create and set the dsc_dce_header, so add the
> > dsc_dce_header and API drm_dsc_dsi_pps_header_init
> > 
> > Signed-off-by: Vinod Koul 
> > ---
> >  drivers/gpu/drm/drm_dsc.c | 11 +++
> >  include/drm/drm_dsc.h | 16 
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
> > index ff602f7ec65b..0c1b745090e2 100644
> > --- a/drivers/gpu/drm/drm_dsc.c
> > +++ b/drivers/gpu/drm/drm_dsc.c
> > @@ -49,6 +49,17 @@ void drm_dsc_dp_pps_header_init(struct dp_sdp_header 
> > *pps_header)
> >  }
> >  EXPORT_SYMBOL(drm_dsc_dp_pps_header_init);
> >  
> > +void drm_dsc_dsi_pps_header_init(struct dsc_dce_header *dsc_header)
> 
> Kerneldoc for anything exported to drivers please, also ideally for all
> the structures.

Sorry missed that, will add

-- 
~Vinod
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 7/7] drm/msm: Migrate to aggregate driver

2021-05-24 Thread Stephen Boyd

Quoting Daniel Vetter (2021-05-20 12:58:52)
> On Wed, May 19, 2021 at 05:25:19PM -0700, Stephen Boyd wrote:
> > @@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device 
> > *pdev)
> >   if (ret)
> >   goto fail;
> >
> > - ret = component_master_add_with_match(>dev, _drm_ops, 
> > match);
> > + msm_drm_aggregate_driver.match = match;
>
> This is a bit awkward design, because it means the driver struct can't be
> made const, and it will blow up when you have multiple instance of the
> same driver. I think the match should stay as part of the register
> function call, and be stored in the aggregate_device struct somewhere.

Got it. The driver struct can't be const for other reasons but I agree
it is awkward. I'm currently using the match pointer to figure out if
this aggregate driver is related to the aggregate device. The match
pointer is already stored in the aggregate_device. The problem is we
need to know if some driver is associated with some aggregate_device,
and so I took the easy way out and registered the aggregate_device at
the same time that the aggregate_driver is registered and stashed the
match pointer in both structures to match them up later during driver
binding.

If we want to support multiple aggregate_devices for an aggregate_driver
then I'll have to come up with some other way of associating the
aggregate_devices created in the component code with the aggregate
driver that registered it. I suppose a list of aggregate_devices will
work. Is any sort of driver doing this right now and registering the
bind/unbind ops with multiple devices? I just wonder if there's any
point in doing it if it will always be a 1:1 relationship between
aggregate device and driver.

>
> Otherwise I think this looks really solid and fixes your issue properly.
> Obviously needs careful review from Greg KH for the device model side of
> things, and from Rafael Wysocki for pm side.

Yeah apparently it fixes my issue because the aggregate_device is added
after all the other devices described in DT (and the i2c bridge) have
been added to the dpm list. Otherwise I would still have problems, but
using device links should help me guarantee the aggregate_device is in
the right location on the list. I still have to check that the i2c
bridge is linked to the DSI encoder though.

>
> Bunch of thoughts from a very cursory reading:
>
> - I think it'd be good if we pass the aggregate_device to all components
>   when we bind them, plus the void * parameter just to make this less
>   disruptive. Even more device model goodies.

So the idea is to pass aggregate_device into the struct
component_ops::{bind,unbind}() functions? Right now it takes the parent
device, so we'll need to introduce another set of function pointers for
the "modern" way of doing things in the component and then pass the
aggregate_device pointer instead of the parent. I can roll that into
another patch and then deprecate the bind/unbind function pointers.

I'll pass the aggregate_device instead of a device pointer so that
compilation will break if the code isn't migrated properly. I also see
that in the msm case the component driver probe is mostly just punted
into the component bind ops. I'd like to change that so the component
drivers get all their resources in their real probe, i.e. platform
driver probe, and then only do things related to making the graphics
card "whole" in their bind. This mostly means that power management
stuff will move out of the bind callback and into the probe callback and
then only once the power management stuff is ready will we actually
register the component device.

>
> - Maybe splatter a pile of sysfs links around so that this all becomes
>   visible? Could be interesting for debugging ordering issues. Just an
>   idea, feel free to entirely ignore.

Sure. I'll do the device link stuff from the components to the aggregate
driver and that should help, as Saravana mentioned earlier.

>
> - Needs solid kerneldoc for everything exposed to drivers and good
>   overview DOC:

Ok I'll layer that on at the end.

>
> - Needs deprecation warnings in the kerneldoc for all the
>   component_master_* and if feasible with a mechanical conversion,
>   converting existing users. I'd like to not be stuck with the old model
>   forever, plus this will give a pile more people to review this code
>   here.

Ok. I'll dust off coccinelle or just do it by hand. There aren't that
many. I hope.

>
> Anyway the name changes in probe and remove hooks below are already worth
> this on their own imo. That's why I'd like to see them in all drivers.
>

Cool, thanks for taking a look. It may take me a couple more days to get
v2 out the door and I'll have to spend a bunch of time converting more
drivers to shake out more problems.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 3/7] component: Introduce struct aggregate_device

2021-05-24 Thread Stephen Boyd

Quoting Saravana Kannan (2021-05-20 13:20:45)
> On Wed, May 19, 2021 at 5:25 PM Stephen Boyd  wrote:
> >
> > -   master->parent = parent;
> > -   master->ops = ops;
> > -   master->match = match;
> > +   id = ida_alloc(_ida, GFP_KERNEL);
> > +   if (id < 0) {
> > +   kfree(adev);
> > +   return id;
> > +   }
> > +
> > +   adev->id = id;
> > +   adev->parent = parent;
> > +   adev->dev.parent = parent;
>
> Don't set adev->dev.parent. You are creating a functional 1-1
> dependency where none exists. The real dependencies are the 1-many
> dependencies between the aggregate and the components. Use device
> links to capture that and enforce proper suspend/resume and runtime PM
> ordering.
>

Ah ok. Yeah it seems like that was the thing causing me runtime PM
problems. I've removed the parent patch from this series now and I'll
look at working in the device links now.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH] drm/msm/dp: Drop unnecessary NULL checks after container_of

Re: [Freedreno] [PATCH v18 2/4] dt-bindings: msm: dsi: add yaml schemas for DSI bindings

Re: [Freedreno] [PATCH v2 6/6] drm/msm/dpu: simplify IRQ enabling/disabling

Re: [Freedreno] [PATCH v2 4/6] drm/msm/dpu: replace IRQ lookup with the data in hw catalog

Re: [Freedreno] [PATCH v2 3/6] drm/msm/dpu: define interrupt register names

Re: [Freedreno] [PATCH v2 2/6] drm/msm/dpu: hw_intr: always call dpu_hw_intr_clear_intr_status_nolock

Re: [Freedreno] [PATCH v2 1/6] drm/msm/dpu: merge dpu_hw_intr_get_interrupt_statuses into dpu_hw_intr_dispatch_irqs

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

Re: [Freedreno] [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

Re: [Freedreno] [PATCH 2/3] drm/msm/dp: Shrink locking area of dp_aux_transfer()

Re: [Freedreno] [PATCH 1/3] drm/msm/dp: Simplify aux irq handling code

Re: [Freedreno] [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Freedreno] [PATCH v18 1/4] dt-bindings: msm: disp: add yaml schemas for DPU bindings

[Freedreno] [PATCH v18 2/4] dt-bindings: msm: dsi: add yaml schemas for DSI bindings

[Freedreno] [PATCH v18 4/4] dt-bindings: msm/dp: Add bindings of MSM DisplayPort controller

[Freedreno] [PATCH v18 3/4] dt-bindings: msm: dsi: add yaml schemas for DSI PHY bindings

Re: [Freedreno] [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

Re: [Freedreno] [RFC PATCH 01/13] drm/dsc: Add dsc pps header init function

Re: [Freedreno] [PATCH 7/7] drm/msm: Migrate to aggregate driver

Re: [Freedreno] [PATCH 3/7] component: Introduce struct aggregate_device

22 matches

Site Navigation

Mail list logo

Footer information