[PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.

[PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.

[Eric Anholt]

No compatible string for it yet, just the version-dependent changes.
They've now tied the hub and the core interrupt lines into a single
interrupt line coming out of the block.  It also turns out I made a
mistake in modeling the V3D v3.3 and v4.1 bridge as a part of V3D
itself -- the bridge is going away in favor of an external reset
controller in a larger HW module.

v2: Use consistent checks for whether we're on 4.2, and fix a leak in
    an error path.
v3: Use more general means of determining if the current 4.2 changes
    are in place, as apparently other platforms may switch back (noted
    by Dave).  Update the binding doc.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../devicetree/bindings/gpu/brcm,bcm-v3d.txt  | 11 ++++++--
 drivers/gpu/drm/v3d/v3d_drv.c                 | 21 +++++++++++---
 drivers/gpu/drm/v3d/v3d_drv.h                 |  2 ++
 drivers/gpu/drm/v3d/v3d_gem.c                 | 12 +++++++-
 drivers/gpu/drm/v3d/v3d_irq.c                 | 28 +++++++++++++++----
 5 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
index c907aa8dd755..b2df82b44625 100644
--- a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
+++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
@@ -6,15 +6,20 @@ For V3D 2.x, see brcm,bcm-vc4.txt.
 Required properties:
 - compatible:	Should be "brcm,7268-v3d" or "brcm,7278-v3d"
 - reg:		Physical base addresses and lengths of the register areas
-- reg-names:	Names for the register areas.  The "hub", "bridge", and "core0"
+- reg-names:	Names for the register areas.  The "hub" and "core0"
 		  register areas are always required.  The "gca" register area
-		  is required if the GCA cache controller is present.
+		  is required if the GCA cache controller is present.  The
+		  "bridge" register area is required if an external reset
+		  controller is not present.
 - interrupts:	The interrupt numbers.  The first interrupt is for the hub,
-		  while the following interrupts are for the cores.
+		  while the following interrupts are separate interrupt lines
+		  for the cores (if they don't share the hub's interrupt).
 		  See bindings/interrupt-controller/interrupts.txt
 
 Optional properties:
 - clocks:	The core clock the unit runs on
+- resets:	The reset line for v3d, if not using a mapping of the bridge
+		  See bindings/reset/reset.txt
 
 v3d {
 	compatible = "brcm,7268-v3d";
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index b189b1a0ae7e..392e458b55bd 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -19,6 +19,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_fb_helper.h>
@@ -496,10 +497,6 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d->pdev = pdev;
 	drm = &v3d->drm;
 
-	ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
-	if (ret)
-		goto dev_free;
-
 	ret = map_regs(v3d, &v3d->hub_regs, "hub");
 	if (ret)
 		goto dev_free;
@@ -514,6 +511,22 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d->cores = V3D_GET_FIELD(ident1, V3D_HUB_IDENT1_NCORES);
 	WARN_ON(v3d->cores > 1); /* multicore not yet implemented */
 
+	v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(v3d->reset)) {
+		ret = PTR_ERR(v3d->reset);
+
+		if (ret == -EPROBE_DEFER)
+			goto dev_free;
+
+		v3d->reset = NULL;
+		ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
+		if (ret) {
+			dev_err(dev,
+				"Failed to get reset control or bridge regs\n");
+			goto dev_free;
+		}
+	}
+
 	if (v3d->ver < 41) {
 		ret = map_regs(v3d, &v3d->gca_regs, "gca");
 		if (ret)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 9e0e11f57307..fab9979b7e1f 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -39,6 +39,7 @@ struct v3d_dev {
 	 * and revision.
 	 */
 	int ver;
+	bool single_irq_line;
 
 	struct device *dev;
 	struct platform_device *pdev;
@@ -47,6 +48,7 @@ struct v3d_dev {
 	void __iomem *bridge_regs;
 	void __iomem *gca_regs;
 	struct clk *clk;
+	struct reset_control *reset;
 
 	/* Virtual and DMA addresses of the single shared page table. */
 	volatile u32 *pt;
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 0d1e5e0b8042..109be31e47ea 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -6,6 +6,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/sched/signal.h>
@@ -69,7 +70,7 @@ v3d_idle_gca(struct v3d_dev *v3d)
 }
 
 static void
-v3d_reset_v3d(struct v3d_dev *v3d)
+v3d_reset_by_bridge(struct v3d_dev *v3d)
 {
 	int version = V3D_BRIDGE_READ(V3D_TOP_GR_BRIDGE_REVISION);
 
@@ -89,6 +90,15 @@ v3d_reset_v3d(struct v3d_dev *v3d)
 				 V3D_TOP_GR_BRIDGE_SW_INIT_1_V3D_CLK_108_SW_INIT);
 		V3D_BRIDGE_WRITE(V3D_TOP_GR_BRIDGE_SW_INIT_1, 0);
 	}
+}
+
+static void
+v3d_reset_v3d(struct v3d_dev *v3d)
+{
+	if (v3d->reset)
+		reset_control_reset(v3d->reset);
+	else
+		v3d_reset_by_bridge(v3d);
 
 	v3d_init_hw_state(v3d);
 }
diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 69338da70ddc..288ab1036a7a 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -27,6 +27,9 @@
 			    V3D_HUB_INT_MMU_CAP |	\
 			    V3D_HUB_INT_TFUC))
 
+static irqreturn_t
+v3d_hub_irq(int irq, void *arg);
+
 static void
 v3d_overflow_mem_work(struct work_struct *work)
 {
@@ -112,6 +115,12 @@ v3d_irq(int irq, void *arg)
 	if (intsts & V3D_INT_GMPV)
 		dev_err(v3d->dev, "GMP violation\n");
 
+	/* V3D 4.2 wires the hub and core IRQs together, so if we &
+	 * didn't see the common one then check hub for MMU IRQs.
+	 */
+	if (v3d->single_irq_line && status == IRQ_NONE)
+		return v3d_hub_irq(irq, arg);
+
 	return status;
 }
 
@@ -170,12 +179,19 @@ v3d_irq_init(struct v3d_dev *v3d)
 		V3D_CORE_WRITE(core, V3D_CTL_INT_CLR, V3D_CORE_IRQS);
 	V3D_WRITE(V3D_HUB_INT_CLR, V3D_HUB_IRQS);
 
-	ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
-			       v3d_hub_irq, IRQF_SHARED,
-			       "v3d_hub", v3d);
-	ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 1),
-			       v3d_irq, IRQF_SHARED,
-			       "v3d_core0", v3d);
+	if (platform_get_irq(v3d->pdev, 1) < 0) {
+		ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
+				       v3d_irq, IRQF_SHARED,
+				       "v3d", v3d);
+		v3d->single_irq_line = true;
+	} else {
+		ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
+				       v3d_hub_irq, IRQF_SHARED,
+				       "v3d_hub", v3d);
+		ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 1),
+				       v3d_irq, IRQF_SHARED,
+				       "v3d_core0", v3d);
+	}
 	if (ret)
 		dev_err(v3d->dev, "IRQ setup failed: %d\n", ret);
 
-- 
2.20.1

[PATCH v3 2/3] drm/v3d: Don't try to set OVRTMUOUT on V3D 4.x.

[Eric Anholt]

The old field is gone and the register now has a different field,
QRMAXCNT for how many TMU requests get serviced before thread switch.
We were accidentally reducing it from its default of 0x3 (4 requests)
to 0x0 (1).

v2: Skip setting the reg at all on 4.x, instead of trying to update
    only the old field.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/v3d/v3d_gem.c  | 3 ++-
 drivers/gpu/drm/v3d/v3d_regs.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 109be31e47ea..449d01ea54a0 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -25,7 +25,8 @@ v3d_init_core(struct v3d_dev *v3d, int core)
 	 * type.  If you want the default behavior, you can still put
 	 * "2" in the indirect texture state's output_type field.
 	 */
-	V3D_CORE_WRITE(core, V3D_CTL_MISCCFG, V3D_MISCCFG_OVRTMUOUT);
+	if (v3d->ver < 40)
+		V3D_CORE_WRITE(core, V3D_CTL_MISCCFG, V3D_MISCCFG_OVRTMUOUT);
 
 	/* Whenever we flush the L2T cache, we always want to flush
 	 * the whole thing.
diff --git a/drivers/gpu/drm/v3d/v3d_regs.h b/drivers/gpu/drm/v3d/v3d_regs.h
index 6ccdee9d47bd..8e88af237610 100644
--- a/drivers/gpu/drm/v3d/v3d_regs.h
+++ b/drivers/gpu/drm/v3d/v3d_regs.h
@@ -216,6 +216,8 @@
 # define V3D_IDENT2_BCG_INT                            BIT(28)
 
 #define V3D_CTL_MISCCFG                                0x00018
+# define V3D_CTL_MISCCFG_QRMAXCNT_MASK                 V3D_MASK(3, 1)
+# define V3D_CTL_MISCCFG_QRMAXCNT_SHIFT                1
 # define V3D_MISCCFG_OVRTMUOUT                         BIT(0)
 
 #define V3D_CTL_L2CACTL                                0x00020
-- 
2.20.1

[PATCH v3 3/3] drm/v3d: Make sure the GPU is on when measuring clocks.

[Eric Anholt]

You'll get garbage measurements if the registers always read back
0xdeadbeef

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/v3d/v3d_debugfs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c b/drivers/gpu/drm/v3d/v3d_debugfs.c
index eb2b2d2f8553..a24af2d2f574 100644
--- a/drivers/gpu/drm/v3d/v3d_debugfs.c
+++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
@@ -187,6 +187,11 @@ static int v3d_measure_clock(struct seq_file *m, void *unused)
 	uint32_t cycles;
 	int core = 0;
 	int measure_ms = 1000;
+	int ret;
+
+	ret = pm_runtime_get_sync(v3d->dev);
+	if (ret < 0)
+		return ret;
 
 	if (v3d->ver >= 40) {
 		V3D_CORE_WRITE(core, V3D_V4_PCTR_0_SRC_0_3,
@@ -210,6 +215,9 @@ static int v3d_measure_clock(struct seq_file *m, void *unused)
 		   cycles / (measure_ms * 1000),
 		   (cycles / (measure_ms * 100)) % 10);
 
+	pm_runtime_mark_last_busy(v3d->dev);
+	pm_runtime_put_autosuspend(v3d->dev);
+
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.

[Dave Emett]

On Wed, 20 Feb 2019 at 23:37, Eric Anholt <eric@anholt.net> wrote:
>
> No compatible string for it yet, just the version-dependent changes.
> They've now tied the hub and the core interrupt lines into a single
> interrupt line coming out of the block.  It also turns out I made a
> mistake in modeling the V3D v3.3 and v4.1 bridge as a part of V3D
> itself -- the bridge is going away in favor of an external reset
> controller in a larger HW module.
>
> v2: Use consistent checks for whether we're on 4.2, and fix a leak in
>     an error path.
> v3: Use more general means of determining if the current 4.2 changes
>     are in place, as apparently other platforms may switch back (noted
>     by Dave).  Update the binding doc.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>

Reviewed-by: Dave Emett <david.emett@broadcom.com>

> ---
>  .../devicetree/bindings/gpu/brcm,bcm-v3d.txt  | 11 ++++++--
>  drivers/gpu/drm/v3d/v3d_drv.c                 | 21 +++++++++++---
>  drivers/gpu/drm/v3d/v3d_drv.h                 |  2 ++
>  drivers/gpu/drm/v3d/v3d_gem.c                 | 12 +++++++-
>  drivers/gpu/drm/v3d/v3d_irq.c                 | 28 +++++++++++++++----
>  5 files changed, 60 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
> index c907aa8dd755..b2df82b44625 100644
> --- a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
> +++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
> @@ -6,15 +6,20 @@ For V3D 2.x, see brcm,bcm-vc4.txt.
>  Required properties:
>  - compatible:  Should be "brcm,7268-v3d" or "brcm,7278-v3d"
>  - reg:         Physical base addresses and lengths of the register areas
> -- reg-names:   Names for the register areas.  The "hub", "bridge", and "core0"
> +- reg-names:   Names for the register areas.  The "hub" and "core0"
>                   register areas are always required.  The "gca" register area
> -                 is required if the GCA cache controller is present.
> +                 is required if the GCA cache controller is present.  The
> +                 "bridge" register area is required if an external reset
> +                 controller is not present.
>  - interrupts:  The interrupt numbers.  The first interrupt is for the hub,
> -                 while the following interrupts are for the cores.
> +                 while the following interrupts are separate interrupt lines
> +                 for the cores (if they don't share the hub's interrupt).
>                   See bindings/interrupt-controller/interrupts.txt
>
>  Optional properties:
>  - clocks:      The core clock the unit runs on
> +- resets:      The reset line for v3d, if not using a mapping of the bridge
> +                 See bindings/reset/reset.txt
>
>  v3d {
>         compatible = "brcm,7268-v3d";
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> index b189b1a0ae7e..392e458b55bd 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -19,6 +19,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_fb_helper.h>
> @@ -496,10 +497,6 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
>         v3d->pdev = pdev;
>         drm = &v3d->drm;
>
> -       ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
> -       if (ret)
> -               goto dev_free;
> -
>         ret = map_regs(v3d, &v3d->hub_regs, "hub");
>         if (ret)
>                 goto dev_free;
> @@ -514,6 +511,22 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
>         v3d->cores = V3D_GET_FIELD(ident1, V3D_HUB_IDENT1_NCORES);
>         WARN_ON(v3d->cores > 1); /* multicore not yet implemented */
>
> +       v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
> +       if (IS_ERR(v3d->reset)) {
> +               ret = PTR_ERR(v3d->reset);
> +
> +               if (ret == -EPROBE_DEFER)
> +                       goto dev_free;
> +
> +               v3d->reset = NULL;
> +               ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
> +               if (ret) {
> +                       dev_err(dev,
> +                               "Failed to get reset control or bridge regs\n");
> +                       goto dev_free;
> +               }
> +       }
> +
>         if (v3d->ver < 41) {
>                 ret = map_regs(v3d, &v3d->gca_regs, "gca");
>                 if (ret)
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index 9e0e11f57307..fab9979b7e1f 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -39,6 +39,7 @@ struct v3d_dev {
>          * and revision.
>          */
>         int ver;
> +       bool single_irq_line;
>
>         struct device *dev;
>         struct platform_device *pdev;
> @@ -47,6 +48,7 @@ struct v3d_dev {
>         void __iomem *bridge_regs;
>         void __iomem *gca_regs;
>         struct clk *clk;
> +       struct reset_control *reset;
>
>         /* Virtual and DMA addresses of the single shared page table. */
>         volatile u32 *pt;
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index 0d1e5e0b8042..109be31e47ea 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -6,6 +6,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/sched/signal.h>
> @@ -69,7 +70,7 @@ v3d_idle_gca(struct v3d_dev *v3d)
>  }
>
>  static void
> -v3d_reset_v3d(struct v3d_dev *v3d)
> +v3d_reset_by_bridge(struct v3d_dev *v3d)
>  {
>         int version = V3D_BRIDGE_READ(V3D_TOP_GR_BRIDGE_REVISION);
>
> @@ -89,6 +90,15 @@ v3d_reset_v3d(struct v3d_dev *v3d)
>                                  V3D_TOP_GR_BRIDGE_SW_INIT_1_V3D_CLK_108_SW_INIT);
>                 V3D_BRIDGE_WRITE(V3D_TOP_GR_BRIDGE_SW_INIT_1, 0);
>         }
> +}
> +
> +static void
> +v3d_reset_v3d(struct v3d_dev *v3d)
> +{
> +       if (v3d->reset)
> +               reset_control_reset(v3d->reset);
> +       else
> +               v3d_reset_by_bridge(v3d);
>
>         v3d_init_hw_state(v3d);
>  }
> diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
> index 69338da70ddc..288ab1036a7a 100644
> --- a/drivers/gpu/drm/v3d/v3d_irq.c
> +++ b/drivers/gpu/drm/v3d/v3d_irq.c
> @@ -27,6 +27,9 @@
>                             V3D_HUB_INT_MMU_CAP |       \
>                             V3D_HUB_INT_TFUC))
>
> +static irqreturn_t
> +v3d_hub_irq(int irq, void *arg);
> +
>  static void
>  v3d_overflow_mem_work(struct work_struct *work)
>  {
> @@ -112,6 +115,12 @@ v3d_irq(int irq, void *arg)
>         if (intsts & V3D_INT_GMPV)
>                 dev_err(v3d->dev, "GMP violation\n");
>
> +       /* V3D 4.2 wires the hub and core IRQs together, so if we &
> +        * didn't see the common one then check hub for MMU IRQs.
> +        */
> +       if (v3d->single_irq_line && status == IRQ_NONE)
> +               return v3d_hub_irq(irq, arg);
> +
>         return status;
>  }
>
> @@ -170,12 +179,19 @@ v3d_irq_init(struct v3d_dev *v3d)
>                 V3D_CORE_WRITE(core, V3D_CTL_INT_CLR, V3D_CORE_IRQS);
>         V3D_WRITE(V3D_HUB_INT_CLR, V3D_HUB_IRQS);
>
> -       ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
> -                              v3d_hub_irq, IRQF_SHARED,
> -                              "v3d_hub", v3d);
> -       ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 1),
> -                              v3d_irq, IRQF_SHARED,
> -                              "v3d_core0", v3d);
> +       if (platform_get_irq(v3d->pdev, 1) < 0) {
> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
> +                                      v3d_irq, IRQF_SHARED,
> +                                      "v3d", v3d);
> +               v3d->single_irq_line = true;
> +       } else {
> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
> +                                      v3d_hub_irq, IRQF_SHARED,
> +                                      "v3d_hub", v3d);
> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 1),
> +                                      v3d_irq, IRQF_SHARED,
> +                                      "v3d_core0", v3d);
> +       }
>         if (ret)
>                 dev_err(v3d->dev, "IRQ setup failed: %d\n", ret);
>
> --
> 2.20.1
>

Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.

[Dave Emett]

Sorry, a few things I thought of after sending the Reviewed-by email...

> +       v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
> +       if (IS_ERR(v3d->reset)) {
> +               ret = PTR_ERR(v3d->reset);
> +
> +               if (ret == -EPROBE_DEFER)
> +                       goto dev_free;
Might be preferable to make this explicitly check against the
not-found error code (whatever that is)? As in if (not found)
<fallback to bridge> else <return error code>. Similarly...

> +       if (platform_get_irq(v3d->pdev, 1) < 0) {
This should probably explicitly check for not-found rather than any
error. As-is, we might silently go down the single-interrupt-line path
on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
hits some other error.

> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
> +                                      v3d_hub_irq, IRQF_SHARED,
> +                                      "v3d_hub", v3d);
> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 1),
> +                                      v3d_irq, IRQF_SHARED,
> +                                      "v3d_core0", v3d);
Not introduced by this change, but return value from first
devm_request_irq ignored here?

Re: [PATCH v3 2/3] drm/v3d: Don't try to set OVRTMUOUT on V3D 4.x.

[Dave Emett]

On Wed, 20 Feb 2019 at 23:37, Eric Anholt <eric@anholt.net> wrote:
>
> The old field is gone and the register now has a different field,
> QRMAXCNT for how many TMU requests get serviced before thread switch.
> We were accidentally reducing it from its default of 0x3 (4 requests)
> to 0x0 (1).
>
> v2: Skip setting the reg at all on 4.x, instead of trying to update
>     only the old field.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>

Reviewed-by: Dave Emett <david.emett@broadcom.com>


> ---
>  drivers/gpu/drm/v3d/v3d_gem.c  | 3 ++-
>  drivers/gpu/drm/v3d/v3d_regs.h | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index 109be31e47ea..449d01ea54a0 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -25,7 +25,8 @@ v3d_init_core(struct v3d_dev *v3d, int core)
>          * type.  If you want the default behavior, you can still put
>          * "2" in the indirect texture state's output_type field.
>          */
> -       V3D_CORE_WRITE(core, V3D_CTL_MISCCFG, V3D_MISCCFG_OVRTMUOUT);
> +       if (v3d->ver < 40)
> +               V3D_CORE_WRITE(core, V3D_CTL_MISCCFG, V3D_MISCCFG_OVRTMUOUT);
>
>         /* Whenever we flush the L2T cache, we always want to flush
>          * the whole thing.
> diff --git a/drivers/gpu/drm/v3d/v3d_regs.h b/drivers/gpu/drm/v3d/v3d_regs.h
> index 6ccdee9d47bd..8e88af237610 100644
> --- a/drivers/gpu/drm/v3d/v3d_regs.h
> +++ b/drivers/gpu/drm/v3d/v3d_regs.h
> @@ -216,6 +216,8 @@
>  # define V3D_IDENT2_BCG_INT                            BIT(28)
>
>  #define V3D_CTL_MISCCFG                                0x00018
> +# define V3D_CTL_MISCCFG_QRMAXCNT_MASK                 V3D_MASK(3, 1)
> +# define V3D_CTL_MISCCFG_QRMAXCNT_SHIFT                1
>  # define V3D_MISCCFG_OVRTMUOUT                         BIT(0)
>
>  #define V3D_CTL_L2CACTL                                0x00020
> --
> 2.20.1
>

Re: [PATCH v3 3/3] drm/v3d: Make sure the GPU is on when measuring clocks.

[Dave Emett]

On Wed, 20 Feb 2019 at 23:37, Eric Anholt <eric@anholt.net> wrote:
>
> You'll get garbage measurements if the registers always read back
> 0xdeadbeef
>
> Signed-off-by: Eric Anholt <eric@anholt.net>

Reviewed-by: Dave Emett <david.emett@broadcom.com>

> ---
>  drivers/gpu/drm/v3d/v3d_debugfs.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c b/drivers/gpu/drm/v3d/v3d_debugfs.c
> index eb2b2d2f8553..a24af2d2f574 100644
> --- a/drivers/gpu/drm/v3d/v3d_debugfs.c
> +++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
> @@ -187,6 +187,11 @@ static int v3d_measure_clock(struct seq_file *m, void *unused)
>         uint32_t cycles;
>         int core = 0;
>         int measure_ms = 1000;
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(v3d->dev);
> +       if (ret < 0)
> +               return ret;
>
>         if (v3d->ver >= 40) {
>                 V3D_CORE_WRITE(core, V3D_V4_PCTR_0_SRC_0_3,
> @@ -210,6 +215,9 @@ static int v3d_measure_clock(struct seq_file *m, void *unused)
>                    cycles / (measure_ms * 1000),
>                    (cycles / (measure_ms * 100)) % 10);
>
> +       pm_runtime_mark_last_busy(v3d->dev);
> +       pm_runtime_put_autosuspend(v3d->dev);
> +
>         return 0;
>  }
>
> --
> 2.20.1
>

Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

Dave Emett <david.emett@broadcom.com> writes:

> Sorry, a few things I thought of after sending the Reviewed-by email...
>
>> +       v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
>> +       if (IS_ERR(v3d->reset)) {
>> +               ret = PTR_ERR(v3d->reset);
>> +
>> +               if (ret == -EPROBE_DEFER)
>> +                       goto dev_free;
> Might be preferable to make this explicitly check against the
> not-found error code (whatever that is)? As in if (not found)
> <fallback to bridge> else <return error code>. Similarly...

You won't have both a bridge and an external reset controller in the DT,
so I'm not clear on what functional change you're looking for.  You're
just concerned about what the return code from this function is?
-EPROBE_DEFER is the only one that matters from a probe, really.

>> +       if (platform_get_irq(v3d->pdev, 1) < 0) {
> This should probably explicitly check for not-found rather than any
> error. As-is, we might silently go down the single-interrupt-line path
> on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
> hits some other error.

If I do the -EPROBE_DEFER check here, will that be good enough for you?

>> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
>> +                                      v3d_hub_irq, IRQF_SHARED,
>> +                                      "v3d_hub", v3d);
>> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 1),
>> +                                      v3d_irq, IRQF_SHARED,
>> +                                      "v3d_core0", v3d);
> Not introduced by this change, but return value from first
> devm_request_irq ignored here?

True, but let's handle separate bugs separately.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1913 bytes --]

Eric Anholt <eric@anholt.net> writes:

> [ Unknown signature status ]
> Dave Emett <david.emett@broadcom.com> writes:
>
>> Sorry, a few things I thought of after sending the Reviewed-by email...
>>
>>> +       v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
>>> +       if (IS_ERR(v3d->reset)) {
>>> +               ret = PTR_ERR(v3d->reset);
>>> +
>>> +               if (ret == -EPROBE_DEFER)
>>> +                       goto dev_free;
>> Might be preferable to make this explicitly check against the
>> not-found error code (whatever that is)? As in if (not found)
>> <fallback to bridge> else <return error code>. Similarly...
>
> You won't have both a bridge and an external reset controller in the DT,
> so I'm not clear on what functional change you're looking for.  You're
> just concerned about what the return code from this function is?
> -EPROBE_DEFER is the only one that matters from a probe, really.
>
>>> +       if (platform_get_irq(v3d->pdev, 1) < 0) {
>> This should probably explicitly check for not-found rather than any
>> error. As-is, we might silently go down the single-interrupt-line path
>> on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
>> hits some other error.
>
> If I do the -EPROBE_DEFER check here, will that be good enough for you?
>
>>> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 0),
>>> +                                      v3d_hub_irq, IRQF_SHARED,
>>> +                                      "v3d_hub", v3d);
>>> +               ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev, 1),
>>> +                                      v3d_irq, IRQF_SHARED,
>>> +                                      "v3d_core0", v3d);
>> Not introduced by this change, but return value from first
>> devm_request_irq ignored here?
>
> True, but let's handle separate bugs separately.

Resubmitted with the fix anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.

[Dave Emett]

On Fri, 8 Mar 2019 at 16:51, Eric Anholt <eric@anholt.net> wrote:
>
> Dave Emett <david.emett@broadcom.com> writes:
>
> > Sorry, a few things I thought of after sending the Reviewed-by email...
> >
> >> +       v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
> >> +       if (IS_ERR(v3d->reset)) {
> >> +               ret = PTR_ERR(v3d->reset);
> >> +
> >> +               if (ret == -EPROBE_DEFER)
> >> +                       goto dev_free;
> > Might be preferable to make this explicitly check against the
> > not-found error code (whatever that is)? As in if (not found)
> > <fallback to bridge> else <return error code>. Similarly...
>
> You won't have both a bridge and an external reset controller in the DT,
> so I'm not clear on what functional change you're looking for.  You're
> just concerned about what the return code from this function is?
> -EPROBE_DEFER is the only one that matters from a probe, really.

I don't think it matters here. I just figured it should probably be
done the same way as the IRQ.

> >> +       if (platform_get_irq(v3d->pdev, 1) < 0) {
> > This should probably explicitly check for not-found rather than any
> > error. As-is, we might silently go down the single-interrupt-line path
> > on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
> > hits some other error.
>
> If I do the -EPROBE_DEFER check here, will that be good enough for you?

If that's the only other error we can get then sure. It wasn't clear
to me what errors might be returned from platform_get_irq.