* [RFC PATCH 0/3] vlv sideband refactoring
@ 2013-05-15 18:40 Jani Nikula
2013-05-15 18:40 ` [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file Jani Nikula
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Jani Nikula @ 2013-05-15 18:40 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
Okay, I'm stuck with this a bit, and whichever approach I choose I
expect there to be a bunch of bikeshedding. So I won't polish this
further before comments.
Both the intel_dpio_{read,write} and valleyview_{punit,nc}_{read,write}
use the IOSF sideband interface. They access the same registers and do
mostly the same stuff, but no shared code. There are even duplicate
register defines for the same registers. Both have locking, but the
former use dpio_lock and the latter rps.hw_lock. It's racy.
These patches refactor the sideband access to a single function that
expects dpio_lock to be held. The dpio_lock is only used for sideband
stuff, so it's a better match than rps.hw_lock for the purpose. The rps
stuff still needs rps.hw_lock, since it's used to protect more than just
the register access, so rps code will need to hold both locks.
The bikeshedding department:
1) Do we need to propagate sideband errors from the functions (like the
valleyview_punit_* functions do), or, since the return values are
never checked anywhere anyway, can we just convert to the
intel_dpio_* style (functions return the register value only) for all
of them?
2) There will be quite a few more ports. Add new wrappers for each of
them, or create generic read/write functions that need a port
argument?
3) Should the rps stuff take dpio_lock at a higher level than the
wrappers? This is pretty much a requirement for the generic r/w
function.
4) Does dpio really need a devfn different from the rest?
5) Your additional bikeshedding here. ;)
Cheers,
Jani.
Jani Nikula (3):
drm/i915: group vlv iosf sideband register accessors to a new file
drm/i915: refactor all vlv sideband accessors to use one helper
drm/i915: drop redundant warnings on not holding dpio_lock
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/i915_reg.h | 93 ++++++++++++-------------
drivers/gpu/drm/i915/intel_display.c | 37 ----------
drivers/gpu/drm/i915/intel_dp.c | 6 --
drivers/gpu/drm/i915/intel_hdmi.c | 4 --
drivers/gpu/drm/i915/intel_pm.c | 60 ----------------
drivers/gpu/drm/i915/intel_sideband.c | 121 +++++++++++++++++++++++++++++++++
7 files changed, 165 insertions(+), 157 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_sideband.c
--
1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file
2013-05-15 18:40 [RFC PATCH 0/3] vlv sideband refactoring Jani Nikula
@ 2013-05-15 18:40 ` Jani Nikula
2013-05-21 8:04 ` Daniel Vetter
2013-05-15 18:40 ` [RFC PATCH 2/3] drm/i915: refactor all vlv sideband accessors to use one helper Jani Nikula
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2013-05-15 18:40 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
No functional changes.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/intel_display.c | 37 ----------
drivers/gpu/drm/i915/intel_pm.c | 60 ----------------
drivers/gpu/drm/i915/intel_sideband.c | 123 +++++++++++++++++++++++++++++++++
4 files changed, 124 insertions(+), 97 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_sideband.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 91f3ac6..40034ec 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -36,6 +36,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
intel_overlay.o \
intel_sprite.o \
intel_opregion.o \
+ intel_sideband.o \
dvo_ch7xxx.o \
dvo_ch7017.o \
dvo_ivch.o \
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7358e4e..39af0e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -381,43 +381,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
.find_pll = intel_vlv_find_best_pll,
};
-u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
-{
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
- DRM_ERROR("DPIO idle wait timed out\n");
- return 0;
- }
-
- I915_WRITE(DPIO_REG, reg);
- I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_READ | DPIO_PORTID |
- DPIO_BYTE);
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
- DRM_ERROR("DPIO read wait timed out\n");
- return 0;
- }
-
- return I915_READ(DPIO_DATA);
-}
-
-void intel_dpio_write(struct drm_i915_private *dev_priv, int reg, u32 val)
-{
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
- DRM_ERROR("DPIO idle wait timed out\n");
- return;
- }
-
- I915_WRITE(DPIO_DATA, val);
- I915_WRITE(DPIO_REG, reg);
- I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_WRITE | DPIO_PORTID |
- DPIO_BYTE);
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100))
- DRM_ERROR("DPIO write wait timed out\n");
-}
-
static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
int refclk)
{
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1a76572..a06118d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4952,66 +4952,6 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
return 0;
}
-static int vlv_punit_rw(struct drm_i915_private *dev_priv, u32 port, u8 opcode,
- u8 addr, u32 *val)
-{
- u32 cmd, devfn, be, bar;
-
- bar = 0;
- be = 0xf;
- devfn = PCI_DEVFN(2, 0);
-
- cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
- (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
- (bar << IOSF_BAR_SHIFT);
-
- WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
-
- if (I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) {
- DRM_DEBUG_DRIVER("warning: pcode (%s) mailbox access failed\n",
- opcode == PUNIT_OPCODE_REG_READ ?
- "read" : "write");
- return -EAGAIN;
- }
-
- I915_WRITE(VLV_IOSF_ADDR, addr);
- if (opcode == PUNIT_OPCODE_REG_WRITE)
- I915_WRITE(VLV_IOSF_DATA, *val);
- I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
-
- if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) == 0,
- 5)) {
- DRM_ERROR("timeout waiting for pcode %s (%d) to finish\n",
- opcode == PUNIT_OPCODE_REG_READ ? "read" : "write",
- addr);
- return -ETIMEDOUT;
- }
-
- if (opcode == PUNIT_OPCODE_REG_READ)
- *val = I915_READ(VLV_IOSF_DATA);
- I915_WRITE(VLV_IOSF_DATA, 0);
-
- return 0;
-}
-
-int valleyview_punit_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
-{
- return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_READ,
- addr, val);
-}
-
-int valleyview_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
-{
- return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_WRITE,
- addr, &val);
-}
-
-int valleyview_nc_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
-{
- return vlv_punit_rw(dev_priv, IOSF_PORT_NC, PUNIT_OPCODE_REG_READ,
- addr, val);
-}
-
int vlv_gpu_freq(int ddr_freq, int val)
{
int mult, base;
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
new file mode 100644
index 0000000..90c77bc
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -0,0 +1,123 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "i915_drv.h"
+#include "intel_drv.h"
+
+static int vlv_punit_rw(struct drm_i915_private *dev_priv, u32 port, u8 opcode,
+ u8 addr, u32 *val)
+{
+ u32 cmd, devfn, be, bar;
+
+ bar = 0;
+ be = 0xf;
+ devfn = PCI_DEVFN(2, 0);
+
+ cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
+ (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
+ (bar << IOSF_BAR_SHIFT);
+
+ WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+ if (I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) {
+ DRM_DEBUG_DRIVER("warning: pcode (%s) mailbox access failed\n",
+ opcode == PUNIT_OPCODE_REG_READ ?
+ "read" : "write");
+ return -EAGAIN;
+ }
+
+ I915_WRITE(VLV_IOSF_ADDR, addr);
+ if (opcode == PUNIT_OPCODE_REG_WRITE)
+ I915_WRITE(VLV_IOSF_DATA, *val);
+ I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
+
+ if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) == 0,
+ 5)) {
+ DRM_ERROR("timeout waiting for pcode %s (%d) to finish\n",
+ opcode == PUNIT_OPCODE_REG_READ ? "read" : "write",
+ addr);
+ return -ETIMEDOUT;
+ }
+
+ if (opcode == PUNIT_OPCODE_REG_READ)
+ *val = I915_READ(VLV_IOSF_DATA);
+ I915_WRITE(VLV_IOSF_DATA, 0);
+
+ return 0;
+}
+
+int valleyview_punit_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
+{
+ return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_READ,
+ addr, val);
+}
+
+int valleyview_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
+{
+ return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_WRITE,
+ addr, &val);
+}
+
+int valleyview_nc_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
+{
+ return vlv_punit_rw(dev_priv, IOSF_PORT_NC, PUNIT_OPCODE_REG_READ,
+ addr, val);
+}
+
+u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
+{
+ WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
+
+ if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
+ DRM_ERROR("DPIO idle wait timed out\n");
+ return 0;
+ }
+
+ I915_WRITE(DPIO_REG, reg);
+ I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_READ | DPIO_PORTID |
+ DPIO_BYTE);
+ if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
+ DRM_ERROR("DPIO read wait timed out\n");
+ return 0;
+ }
+
+ return I915_READ(DPIO_DATA);
+}
+
+void intel_dpio_write(struct drm_i915_private *dev_priv, int reg, u32 val)
+{
+ WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
+
+ if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
+ DRM_ERROR("DPIO idle wait timed out\n");
+ return;
+ }
+
+ I915_WRITE(DPIO_DATA, val);
+ I915_WRITE(DPIO_REG, reg);
+ I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_WRITE | DPIO_PORTID |
+ DPIO_BYTE);
+ if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100))
+ DRM_ERROR("DPIO write wait timed out\n");
+}
--
1.7.10.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC PATCH 2/3] drm/i915: refactor all vlv sideband accessors to use one helper
2013-05-15 18:40 [RFC PATCH 0/3] vlv sideband refactoring Jani Nikula
2013-05-15 18:40 ` [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file Jani Nikula
@ 2013-05-15 18:40 ` Jani Nikula
2013-05-15 18:40 ` [RFC PATCH 3/3] drm/i915: drop redundant warnings on not holding dpio_lock Jani Nikula
2013-05-21 8:33 ` [RFC PATCH 0/3] vlv sideband refactoring Daniel Vetter
3 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2013-05-15 18:40 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
Based on work by Shobhit Kumar <shobhit.kumar@intel.com> and
Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 93 ++++++++++++++----------------
drivers/gpu/drm/i915/intel_sideband.c | 102 ++++++++++++++++-----------------
2 files changed, 93 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7af7ae6..fcee92b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -344,27 +344,54 @@
#define DEBUG_RESET_DISPLAY (1<<9)
/*
- * DPIO - a special bus for various display related registers to hide behind:
- * 0x800c: m1, m2, n, p1, p2, k dividers
- * 0x8014: REF and SFR select
- * 0x8014: N divider, VCO select
- * 0x801c/3c: core clock bits
- * 0x8048/68: low pass filter coefficients
- * 0x8100: fast clock controls
+ * IOSF sideband
+ */
+#define VLV_IOSF_DOORBELL_REQ (VLV_DISPLAY_BASE + 0x2100)
+#define IOSF_DEVFN_SHIFT 24
+#define IOSF_OPCODE_SHIFT 16
+#define IOSF_PORT_SHIFT 8
+#define IOSF_BYTE_ENABLES_SHIFT 4
+#define IOSF_BAR_SHIFT 1
+#define IOSF_SB_BUSY (1<<0)
+#define IOSF_PORT_PUNIT 0x4
+#define IOSF_PORT_NC 0x11
+#define IOSF_PORT_DPIO 0x12
+#define VLV_IOSF_DATA (VLV_DISPLAY_BASE + 0x2104)
+#define VLV_IOSF_ADDR (VLV_DISPLAY_BASE + 0x2108)
+
+#define PUNIT_OPCODE_REG_READ 6
+#define PUNIT_OPCODE_REG_WRITE 7
+
+#define PUNIT_REG_GPU_LFM 0xd3
+#define PUNIT_REG_GPU_FREQ_REQ 0xd4
+#define PUNIT_REG_GPU_FREQ_STS 0xd8
+#define PUNIT_REG_MEDIA_TURBO_FREQ_REQ 0xdc
+
+#define PUNIT_FUSE_BUS2 0xf6 /* bits 47:40 */
+#define PUNIT_FUSE_BUS1 0xf5 /* bits 55:48 */
+
+#define IOSF_NC_FB_GFX_FREQ_FUSE 0x1c
+#define FB_GFX_MAX_FREQ_FUSE_SHIFT 3
+#define FB_GFX_MAX_FREQ_FUSE_MASK 0x000007f8
+#define FB_GFX_FGUARANTEED_FREQ_FUSE_SHIFT 11
+#define FB_GFX_FGUARANTEED_FREQ_FUSE_MASK 0x0007f800
+#define IOSF_NC_FB_GFX_FMAX_FUSE_HI 0x34
+#define FB_FMAX_VMIN_FREQ_HI_MASK 0x00000007
+#define IOSF_NC_FB_GFX_FMAX_FUSE_LO 0x30
+#define FB_FMAX_VMIN_FREQ_LO_SHIFT 27
+#define FB_FMAX_VMIN_FREQ_LO_MASK 0xf8000000
+
+/*
+ * DPIO - a special bus for various display related registers to hide behind
*
* DPIO is VLV only.
*
* Note: digital port B is DDI0, digital pot C is DDI1
*/
-#define DPIO_PKT (VLV_DISPLAY_BASE + 0x2100)
-#define DPIO_RID (0<<24)
-#define DPIO_OP_WRITE (1<<16)
-#define DPIO_OP_READ (0<<16)
-#define DPIO_PORTID (0x12<<8)
-#define DPIO_BYTE (0xf<<4)
-#define DPIO_BUSY (1<<0) /* status only */
-#define DPIO_DATA (VLV_DISPLAY_BASE + 0x2104)
-#define DPIO_REG (VLV_DISPLAY_BASE + 0x2108)
+#define DPIO_DEVFN 0
+#define DPIO_OPCODE_REG_WRITE 1
+#define DPIO_OPCODE_REG_READ 0
+
#define DPIO_CTL (VLV_DISPLAY_BASE + 0x2110)
#define DPIO_MODSEL1 (1<<3) /* if ref clk b == 27 */
#define DPIO_MODSEL0 (1<<2) /* if ref clk a == 27 */
@@ -4541,40 +4568,6 @@
#define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
#define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16
-#define VLV_IOSF_DOORBELL_REQ 0x182100
-#define IOSF_DEVFN_SHIFT 24
-#define IOSF_OPCODE_SHIFT 16
-#define IOSF_PORT_SHIFT 8
-#define IOSF_BYTE_ENABLES_SHIFT 4
-#define IOSF_BAR_SHIFT 1
-#define IOSF_SB_BUSY (1<<0)
-#define IOSF_PORT_PUNIT 0x4
-#define IOSF_PORT_NC 0x11
-#define VLV_IOSF_DATA 0x182104
-#define VLV_IOSF_ADDR 0x182108
-
-#define PUNIT_OPCODE_REG_READ 6
-#define PUNIT_OPCODE_REG_WRITE 7
-
-#define PUNIT_REG_GPU_LFM 0xd3
-#define PUNIT_REG_GPU_FREQ_REQ 0xd4
-#define PUNIT_REG_GPU_FREQ_STS 0xd8
-#define PUNIT_REG_MEDIA_TURBO_FREQ_REQ 0xdc
-
-#define PUNIT_FUSE_BUS2 0xf6 /* bits 47:40 */
-#define PUNIT_FUSE_BUS1 0xf5 /* bits 55:48 */
-
-#define IOSF_NC_FB_GFX_FREQ_FUSE 0x1c
-#define FB_GFX_MAX_FREQ_FUSE_SHIFT 3
-#define FB_GFX_MAX_FREQ_FUSE_MASK 0x000007f8
-#define FB_GFX_FGUARANTEED_FREQ_FUSE_SHIFT 11
-#define FB_GFX_FGUARANTEED_FREQ_FUSE_MASK 0x0007f800
-#define IOSF_NC_FB_GFX_FMAX_FUSE_HI 0x34
-#define FB_FMAX_VMIN_FREQ_HI_MASK 0x00000007
-#define IOSF_NC_FB_GFX_FMAX_FUSE_LO 0x30
-#define FB_FMAX_VMIN_FREQ_LO_SHIFT 27
-#define FB_FMAX_VMIN_FREQ_LO_MASK 0xf8000000
-
#define GEN6_GT_CORE_STATUS 0x138060
#define GEN6_CORE_CPD_STATE_MASK (7<<4)
#define GEN6_RCn_MASK 7
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index 90c77bc..c5c595b 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -25,42 +25,37 @@
#include "i915_drv.h"
#include "intel_drv.h"
-static int vlv_punit_rw(struct drm_i915_private *dev_priv, u32 port, u8 opcode,
- u8 addr, u32 *val)
+static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
+ u32 port, u32 opcode, u32 addr, u32 *val)
{
- u32 cmd, devfn, be, bar;
-
- bar = 0;
- be = 0xf;
- devfn = PCI_DEVFN(2, 0);
+ u32 cmd, be = 0xf, bar = 0;
+ bool is_read = (opcode == PUNIT_OPCODE_REG_READ ||
+ opcode == DPIO_OPCODE_REG_READ);
cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
(port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
(bar << IOSF_BAR_SHIFT);
- WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+ WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
- if (I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) {
- DRM_DEBUG_DRIVER("warning: pcode (%s) mailbox access failed\n",
- opcode == PUNIT_OPCODE_REG_READ ?
- "read" : "write");
+ if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) == 0, 5)) {
+ DRM_DEBUG_DRIVER("IOSF sideband idle wait (%s) timed out\n",
+ is_read ? "read" : "write");
return -EAGAIN;
}
I915_WRITE(VLV_IOSF_ADDR, addr);
- if (opcode == PUNIT_OPCODE_REG_WRITE)
+ if (!is_read)
I915_WRITE(VLV_IOSF_DATA, *val);
I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
- if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) == 0,
- 5)) {
- DRM_ERROR("timeout waiting for pcode %s (%d) to finish\n",
- opcode == PUNIT_OPCODE_REG_READ ? "read" : "write",
- addr);
+ if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) == 0, 5)) {
+ DRM_DEBUG_DRIVER("IOSF sideband finish wait (%s) timed out\n",
+ is_read ? "read" : "write");
return -ETIMEDOUT;
}
- if (opcode == PUNIT_OPCODE_REG_READ)
+ if (is_read)
*val = I915_READ(VLV_IOSF_DATA);
I915_WRITE(VLV_IOSF_DATA, 0);
@@ -69,55 +64,58 @@ static int vlv_punit_rw(struct drm_i915_private *dev_priv, u32 port, u8 opcode,
int valleyview_punit_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
{
- return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_READ,
- addr, val);
+ int ret;
+
+ WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+ mutex_lock(&dev_priv->dpio_lock);
+ ret = vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_PUNIT,
+ PUNIT_OPCODE_REG_READ, addr, val);
+ mutex_unlock(&dev_priv->dpio_lock);
+
+ return ret;
}
int valleyview_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
{
- return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_WRITE,
- addr, &val);
+ int ret;
+
+ WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+ mutex_lock(&dev_priv->dpio_lock);
+ ret = vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_PUNIT,
+ PUNIT_OPCODE_REG_WRITE, addr, &val);
+ mutex_unlock(&dev_priv->dpio_lock);
+
+ return ret;
}
int valleyview_nc_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
{
- return vlv_punit_rw(dev_priv, IOSF_PORT_NC, PUNIT_OPCODE_REG_READ,
- addr, val);
+ int ret;
+
+ WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+ mutex_lock(&dev_priv->dpio_lock);
+ ret = vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_NC,
+ PUNIT_OPCODE_REG_READ, addr, val);
+ mutex_unlock(&dev_priv->dpio_lock);
+
+ return ret;
}
u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
+ u32 val = 0;
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
- DRM_ERROR("DPIO idle wait timed out\n");
- return 0;
- }
+ vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_DPIO,
+ DPIO_OPCODE_REG_READ, reg, &val);
- I915_WRITE(DPIO_REG, reg);
- I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_READ | DPIO_PORTID |
- DPIO_BYTE);
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
- DRM_ERROR("DPIO read wait timed out\n");
- return 0;
- }
-
- return I915_READ(DPIO_DATA);
+ return val;
}
void intel_dpio_write(struct drm_i915_private *dev_priv, int reg, u32 val)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
- DRM_ERROR("DPIO idle wait timed out\n");
- return;
- }
-
- I915_WRITE(DPIO_DATA, val);
- I915_WRITE(DPIO_REG, reg);
- I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_WRITE | DPIO_PORTID |
- DPIO_BYTE);
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100))
- DRM_ERROR("DPIO write wait timed out\n");
+ vlv_sideband_rw(dev_priv, DPIO_DEVFN, IOSF_PORT_DPIO,
+ DPIO_OPCODE_REG_WRITE, reg, &val);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC PATCH 3/3] drm/i915: drop redundant warnings on not holding dpio_lock
2013-05-15 18:40 [RFC PATCH 0/3] vlv sideband refactoring Jani Nikula
2013-05-15 18:40 ` [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file Jani Nikula
2013-05-15 18:40 ` [RFC PATCH 2/3] drm/i915: refactor all vlv sideband accessors to use one helper Jani Nikula
@ 2013-05-15 18:40 ` Jani Nikula
2013-05-21 8:33 ` [RFC PATCH 0/3] vlv sideband refactoring Daniel Vetter
3 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2013-05-15 18:40 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
The lower level sideband read/write functions already do this.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 6 ------
drivers/gpu/drm/i915/intel_hdmi.c | 4 ----
2 files changed, 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2bb4009..12d5c18 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1428,8 +1428,6 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
int pipe = intel_crtc->pipe;
u32 val;
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
val = intel_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
if (pipe)
@@ -1456,8 +1454,6 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
if (!IS_VALLEYVIEW(dev))
return;
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
/* Program Tx lane resets to default */
intel_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
@@ -1608,8 +1604,6 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
uint8_t train_set = intel_dp->train_set[0];
int port = vlv_dport_to_channel(dport);
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
switch (train_set & DP_TRAIN_PRE_EMPHASIS_MASK) {
case DP_TRAIN_PRE_EMPHASIS_0:
preemph_reg_value = 0x0004000;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 93de5ff..9562fe4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -988,8 +988,6 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
if (!IS_VALLEYVIEW(dev))
return;
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
/* Enable clock channels for this port */
val = intel_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
@@ -1033,8 +1031,6 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
if (!IS_VALLEYVIEW(dev))
return;
- WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
-
/* Program Tx lane resets to default */
intel_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file
2013-05-15 18:40 ` [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file Jani Nikula
@ 2013-05-21 8:04 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-05-21 8:04 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Wed, May 15, 2013 at 09:40:30PM +0300, Jani Nikula wrote:
> No functional changes.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
I'd vote to move the sbi sideband stuff on the haswell pch to this place,
too. Maybe in a follow-up patch though.
-Daniel
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/intel_display.c | 37 ----------
> drivers/gpu/drm/i915/intel_pm.c | 60 ----------------
> drivers/gpu/drm/i915/intel_sideband.c | 123 +++++++++++++++++++++++++++++++++
> 4 files changed, 124 insertions(+), 97 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/intel_sideband.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 91f3ac6..40034ec 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -36,6 +36,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
> intel_overlay.o \
> intel_sprite.o \
> intel_opregion.o \
> + intel_sideband.o \
> dvo_ch7xxx.o \
> dvo_ch7017.o \
> dvo_ivch.o \
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7358e4e..39af0e2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -381,43 +381,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
> .find_pll = intel_vlv_find_best_pll,
> };
>
> -u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
> -{
> - WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
> -
> - if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> - DRM_ERROR("DPIO idle wait timed out\n");
> - return 0;
> - }
> -
> - I915_WRITE(DPIO_REG, reg);
> - I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_READ | DPIO_PORTID |
> - DPIO_BYTE);
> - if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> - DRM_ERROR("DPIO read wait timed out\n");
> - return 0;
> - }
> -
> - return I915_READ(DPIO_DATA);
> -}
> -
> -void intel_dpio_write(struct drm_i915_private *dev_priv, int reg, u32 val)
> -{
> - WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
> -
> - if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> - DRM_ERROR("DPIO idle wait timed out\n");
> - return;
> - }
> -
> - I915_WRITE(DPIO_DATA, val);
> - I915_WRITE(DPIO_REG, reg);
> - I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_WRITE | DPIO_PORTID |
> - DPIO_BYTE);
> - if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100))
> - DRM_ERROR("DPIO write wait timed out\n");
> -}
> -
> static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
> int refclk)
> {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1a76572..a06118d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4952,66 +4952,6 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> return 0;
> }
>
> -static int vlv_punit_rw(struct drm_i915_private *dev_priv, u32 port, u8 opcode,
> - u8 addr, u32 *val)
> -{
> - u32 cmd, devfn, be, bar;
> -
> - bar = 0;
> - be = 0xf;
> - devfn = PCI_DEVFN(2, 0);
> -
> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
> - (bar << IOSF_BAR_SHIFT);
> -
> - WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> -
> - if (I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) {
> - DRM_DEBUG_DRIVER("warning: pcode (%s) mailbox access failed\n",
> - opcode == PUNIT_OPCODE_REG_READ ?
> - "read" : "write");
> - return -EAGAIN;
> - }
> -
> - I915_WRITE(VLV_IOSF_ADDR, addr);
> - if (opcode == PUNIT_OPCODE_REG_WRITE)
> - I915_WRITE(VLV_IOSF_DATA, *val);
> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
> -
> - if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) == 0,
> - 5)) {
> - DRM_ERROR("timeout waiting for pcode %s (%d) to finish\n",
> - opcode == PUNIT_OPCODE_REG_READ ? "read" : "write",
> - addr);
> - return -ETIMEDOUT;
> - }
> -
> - if (opcode == PUNIT_OPCODE_REG_READ)
> - *val = I915_READ(VLV_IOSF_DATA);
> - I915_WRITE(VLV_IOSF_DATA, 0);
> -
> - return 0;
> -}
> -
> -int valleyview_punit_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
> -{
> - return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_READ,
> - addr, val);
> -}
> -
> -int valleyview_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
> -{
> - return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_WRITE,
> - addr, &val);
> -}
> -
> -int valleyview_nc_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
> -{
> - return vlv_punit_rw(dev_priv, IOSF_PORT_NC, PUNIT_OPCODE_REG_READ,
> - addr, val);
> -}
> -
> int vlv_gpu_freq(int ddr_freq, int val)
> {
> int mult, base;
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> new file mode 100644
> index 0000000..90c77bc
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -0,0 +1,123 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_drv.h"
> +
> +static int vlv_punit_rw(struct drm_i915_private *dev_priv, u32 port, u8 opcode,
> + u8 addr, u32 *val)
> +{
> + u32 cmd, devfn, be, bar;
> +
> + bar = 0;
> + be = 0xf;
> + devfn = PCI_DEVFN(2, 0);
> +
> + cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
> + (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
> + (bar << IOSF_BAR_SHIFT);
> +
> + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> + if (I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) {
> + DRM_DEBUG_DRIVER("warning: pcode (%s) mailbox access failed\n",
> + opcode == PUNIT_OPCODE_REG_READ ?
> + "read" : "write");
> + return -EAGAIN;
> + }
> +
> + I915_WRITE(VLV_IOSF_ADDR, addr);
> + if (opcode == PUNIT_OPCODE_REG_WRITE)
> + I915_WRITE(VLV_IOSF_DATA, *val);
> + I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
> +
> + if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) & IOSF_SB_BUSY) == 0,
> + 5)) {
> + DRM_ERROR("timeout waiting for pcode %s (%d) to finish\n",
> + opcode == PUNIT_OPCODE_REG_READ ? "read" : "write",
> + addr);
> + return -ETIMEDOUT;
> + }
> +
> + if (opcode == PUNIT_OPCODE_REG_READ)
> + *val = I915_READ(VLV_IOSF_DATA);
> + I915_WRITE(VLV_IOSF_DATA, 0);
> +
> + return 0;
> +}
> +
> +int valleyview_punit_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
> +{
> + return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_READ,
> + addr, val);
> +}
> +
> +int valleyview_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
> +{
> + return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_WRITE,
> + addr, &val);
> +}
> +
> +int valleyview_nc_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
> +{
> + return vlv_punit_rw(dev_priv, IOSF_PORT_NC, PUNIT_OPCODE_REG_READ,
> + addr, val);
> +}
> +
> +u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
> +{
> + WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
> +
> + if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> + DRM_ERROR("DPIO idle wait timed out\n");
> + return 0;
> + }
> +
> + I915_WRITE(DPIO_REG, reg);
> + I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_READ | DPIO_PORTID |
> + DPIO_BYTE);
> + if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> + DRM_ERROR("DPIO read wait timed out\n");
> + return 0;
> + }
> +
> + return I915_READ(DPIO_DATA);
> +}
> +
> +void intel_dpio_write(struct drm_i915_private *dev_priv, int reg, u32 val)
> +{
> + WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
> +
> + if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> + DRM_ERROR("DPIO idle wait timed out\n");
> + return;
> + }
> +
> + I915_WRITE(DPIO_DATA, val);
> + I915_WRITE(DPIO_REG, reg);
> + I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_WRITE | DPIO_PORTID |
> + DPIO_BYTE);
> + if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100))
> + DRM_ERROR("DPIO write wait timed out\n");
> +}
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/3] vlv sideband refactoring
2013-05-15 18:40 [RFC PATCH 0/3] vlv sideband refactoring Jani Nikula
` (2 preceding siblings ...)
2013-05-15 18:40 ` [RFC PATCH 3/3] drm/i915: drop redundant warnings on not holding dpio_lock Jani Nikula
@ 2013-05-21 8:33 ` Daniel Vetter
3 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-05-21 8:33 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Wed, May 15, 2013 at 09:40:29PM +0300, Jani Nikula wrote:
> Okay, I'm stuck with this a bit, and whichever approach I choose I
> expect there to be a bunch of bikeshedding. So I won't polish this
> further before comments.
>
> Both the intel_dpio_{read,write} and valleyview_{punit,nc}_{read,write}
> use the IOSF sideband interface. They access the same registers and do
> mostly the same stuff, but no shared code. There are even duplicate
> register defines for the same registers. Both have locking, but the
> former use dpio_lock and the latter rps.hw_lock. It's racy.
>
> These patches refactor the sideband access to a single function that
> expects dpio_lock to be held. The dpio_lock is only used for sideband
> stuff, so it's a better match than rps.hw_lock for the purpose. The rps
> stuff still needs rps.hw_lock, since it's used to protect more than just
> the register access, so rps code will need to hold both locks.
>
> The bikeshedding department:
>
> 1) Do we need to propagate sideband errors from the functions (like the
> valleyview_punit_* functions do), or, since the return values are
> never checked anywhere anyway, can we just convert to the
> intel_dpio_* style (functions return the register value only) for all
> of them?
I vote for void return and obnoxious WARNs (WARNs are optional if you
want). There's usually not much we can do if the hw has gone south like
that.
> 2) There will be quite a few more ports. Add new wrappers for each of
> them, or create generic read/write functions that need a port
> argument?
Imo the wrappers aren't too bad right now still, i.e. we can re-bikeshed
this once it happens.
> 3) Should the rps stuff take dpio_lock at a higher level than the
> wrappers? This is pretty much a requirement for the generic r/w
> function.
I'm more unhappy about the inconsistency in locking, since the dpio lock
is now held around large swaths of crtc disable/enable code, but only very
small parts for the rps stuff. I expect this to eventually bite us if dpio
sideband usage keeps growing. But again something we can fix once it blows
up.
> 4) Does dpio really need a devfn different from the rest?
I have no idea about this. Definitely looks ... interesting though ;-) I
think Jesse should take a look here.
> 5) Your additional bikeshedding here. ;)
Dropped one to also move the sbi stuff around.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-21 8:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-15 18:40 [RFC PATCH 0/3] vlv sideband refactoring Jani Nikula
2013-05-15 18:40 ` [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file Jani Nikula
2013-05-21 8:04 ` Daniel Vetter
2013-05-15 18:40 ` [RFC PATCH 2/3] drm/i915: refactor all vlv sideband accessors to use one helper Jani Nikula
2013-05-15 18:40 ` [RFC PATCH 3/3] drm/i915: drop redundant warnings on not holding dpio_lock Jani Nikula
2013-05-21 8:33 ` [RFC PATCH 0/3] vlv sideband refactoring Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.