[Intel-gfx] [PATCH V4 5/6] drm/i915:Add the VCS2 switch in Intel_ring_setup_status_page
The Gen7 doesn't have the second BSD ring. But it will complain the switch check warning message during compilation. So just add it to remove the switch check warning. V1->V2: Follow Daniel's comment to update the comment Reviewed-by: Imre Deak Signed-off-by: Zhao Yakui --- drivers/gpu/drm/i915/intel_ringbuffer.c |5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 7e64ab6..1c08dbb 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -988,6 +988,11 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring) case BCS: mmio = BLT_HWS_PGA_GEN7; break; + /* +* VCS2 actually doesn't exist on Gen7. Only shut up +* gcc switch check warning +*/ + case VCS2: case VCS: mmio = BSD_HWS_PGA_GEN7; break; -- 1.7.10.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH V4 1/6] drm/i915: Split the BDW device definition to prepare for dual BSD rings on BDW GT3
Based on the hardware spec, the BDW GT3 has the different configuration
with the BDW GT1/GT2. So split the BDW device info definition.
This is to do the preparation for adding the Dual BSD rings on BDW GT3 machine.
V1->V2: Follow Daniel's comment to pay attention to the stolen check for BDW
in kernel/early-quirks.c
Reviewed-by: Imre Deak
Signed-off-by: Zhao Yakui
---
drivers/gpu/drm/i915/i915_drv.c | 26 --
include/drm/i915_pciids.h | 22 +-
2 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5d8250f..17fbbe5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -279,6 +279,26 @@ static const struct intel_device_info
intel_broadwell_m_info = {
GEN_DEFAULT_PIPEOFFSETS,
};
+static const struct intel_device_info intel_broadwell_gt3d_info = {
+ .gen = 8, .num_pipes = 3,
+ .need_gfx_hws = 1, .has_hotplug = 1,
+ .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+ .has_llc = 1,
+ .has_ddi = 1,
+ .has_fbc = 1,
+ GEN_DEFAULT_PIPEOFFSETS,
+};
+
+static const struct intel_device_info intel_broadwell_gt3m_info = {
+ .gen = 8, .is_mobile = 1, .num_pipes = 3,
+ .need_gfx_hws = 1, .has_hotplug = 1,
+ .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+ .has_llc = 1,
+ .has_ddi = 1,
+ .has_fbc = 1,
+ GEN_DEFAULT_PIPEOFFSETS,
+};
+
/*
* Make sure any device matches here are from most specific to most
* general. For example, since the Quanta match is based on the subsystem
@@ -311,8 +331,10 @@ static const struct intel_device_info
intel_broadwell_m_info = {
INTEL_HSW_M_IDS(&intel_haswell_m_info), \
INTEL_VLV_M_IDS(&intel_valleyview_m_info), \
INTEL_VLV_D_IDS(&intel_valleyview_d_info), \
- INTEL_BDW_M_IDS(&intel_broadwell_m_info), \
- INTEL_BDW_D_IDS(&intel_broadwell_d_info)
+ INTEL_BDW_GT12M_IDS(&intel_broadwell_m_info), \
+ INTEL_BDW_GT12D_IDS(&intel_broadwell_d_info), \
+ INTEL_BDW_GT3M_IDS(&intel_broadwell_gt3m_info), \
+ INTEL_BDW_GT3D_IDS(&intel_broadwell_gt3d_info)
static const struct pci_device_id pciidlist[] = { /* aka */
INTEL_PCI_IDS,
diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
index 940ece4..24f3cad 100644
--- a/include/drm/i915_pciids.h
+++ b/include/drm/i915_pciids.h
@@ -223,14 +223,26 @@
_INTEL_BDW_D(gt, 0x160A, info), /* Server */ \
_INTEL_BDW_D(gt, 0x160D, info) /* Workstation */
-#define INTEL_BDW_M_IDS(info) \
+#define INTEL_BDW_GT12M_IDS(info) \
_INTEL_BDW_M_IDS(1, info), \
- _INTEL_BDW_M_IDS(2, info), \
- _INTEL_BDW_M_IDS(3, info)
+ _INTEL_BDW_M_IDS(2, info)
-#define INTEL_BDW_D_IDS(info) \
+#define INTEL_BDW_GT12D_IDS(info) \
_INTEL_BDW_D_IDS(1, info), \
- _INTEL_BDW_D_IDS(2, info), \
+ _INTEL_BDW_D_IDS(2, info)
+
+#define INTEL_BDW_GT3M_IDS(info) \
+ _INTEL_BDW_M_IDS(3, info)
+
+#define INTEL_BDW_GT3D_IDS(info) \
_INTEL_BDW_D_IDS(3, info)
+#define INTEL_BDW_M_IDS(info) \
+ INTEL_BDW_GT12M_IDS(info), \
+ INTEL_BDW_GT3M_IDS(info)
+
+#define INTEL_BDW_D_IDS(info) \
+ INTEL_BDW_GT12D_IDS(info), \
+ INTEL_BDW_GT3D_IDS(info)
+
#endif /* _I915_PCIIDS_H */
--
1.7.10.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH V4 3/6] drm/i915:Initialize the second BSD ring on BDW GT3 machine
Based on the hardware spec, the BDW GT3 machine has two independent
BSD ring that can be used to dispatch the video commands.
So just initialize it.
V3->V4: Follow Imre's comment to do some minor updates. For example:
more comments are added to describe the semaphore between ring.
Reviewed-by: Imre Deak
Signed-off-by: Zhao Yakui
---
drivers/gpu/drm/i915/i915_drv.c |4 +-
drivers/gpu/drm/i915/i915_drv.h |2 +
drivers/gpu/drm/i915/i915_gem.c |9 +++-
drivers/gpu/drm/i915/i915_gpu_error.c |1 +
drivers/gpu/drm/i915/i915_reg.h |1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 78 +++
drivers/gpu/drm/i915/intel_ringbuffer.h |4 +-
7 files changed, 95 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 17fbbe5..2a7842b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -282,7 +282,7 @@ static const struct intel_device_info
intel_broadwell_m_info = {
static const struct intel_device_info intel_broadwell_gt3d_info = {
.gen = 8, .num_pipes = 3,
.need_gfx_hws = 1, .has_hotplug = 1,
- .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+ .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
.has_llc = 1,
.has_ddi = 1,
.has_fbc = 1,
@@ -292,7 +292,7 @@ static const struct intel_device_info
intel_broadwell_gt3d_info = {
static const struct intel_device_info intel_broadwell_gt3m_info = {
.gen = 8, .is_mobile = 1, .num_pipes = 3,
.need_gfx_hws = 1, .has_hotplug = 1,
- .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+ .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
.has_llc = 1,
.has_ddi = 1,
.has_fbc = 1,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 92c3095..74aef6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1833,7 +1833,9 @@ struct drm_i915_cmd_table {
#define BSD_RING (1ring_mask & BSD2_RING)
#define HAS_BLT(dev)(INTEL_INFO(dev)->ring_mask & BLT_RING)
#define HAS_VEBOX(dev)(INTEL_INFO(dev)->ring_mask & VEBOX_RING)
#define HAS_LLC(dev)(INTEL_INFO(dev)->has_llc)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 85c9cf0..65c441c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4374,13 +4374,20 @@ static int i915_gem_init_rings(struct drm_device *dev)
goto cleanup_blt_ring;
}
+ if (HAS_BSD2(dev)) {
+ ret = intel_init_bsd2_ring_buffer(dev);
+ if (ret)
+ goto cleanup_vebox_ring;
+ }
ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
if (ret)
- goto cleanup_vebox_ring;
+ goto cleanup_bsd2_ring;
return 0;
+cleanup_bsd2_ring:
+ intel_cleanup_ring_buffer(&dev_priv->ring[VCS2]);
cleanup_vebox_ring:
intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
cleanup_blt_ring:
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 4865ade..282164c 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -42,6 +42,7 @@ static const char *ring_str(int ring)
case VCS: return "bsd";
case BCS: return "blt";
case VECS: return "vebox";
+ case VCS2: return "bsd2";
default: return "";
}
}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f84555..0b88508 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -760,6 +760,7 @@ enum punit_power_well {
#define RENDER_RING_BASE 0x02000
#define BSD_RING_BASE 0x04000
#define GEN6_BSD_RING_BASE 0x12000
+#define GEN8_BSD2_RING_BASE0x1c000
#define VEBOX_RING_BASE0x1a000
#define BLT_RING_BASE 0x22000
#define RING_TAIL(base)((base)+0x30)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index eb3dd26..7e64ab6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1920,14 +1920,22 @@ int intel_init_render_ring_buffer(struct drm_device
*dev)
ring->get_seqno = gen6_ring_get_seqno;
ring->set_seqno = ring_set_seqno;
ring->sync_to = gen6_ring_sync;
+ /*
+* The current semaphore is only applied on pre-gen8 platform.
+* And there is no VCS2 ring on the pre-gen8 platform. So the
+* semaphore between RCS and VCS2 is initialized as INVALID.
+
[Intel-gfx] [PATCH V4 6/6] drm/i915: Use the coarse ping-pong mechanism based on drm fd to dispatch the BSD command on BDW GT3
The BDW GT3 has two independent BSD rings, which can be used to process the
video commands. To be simpler, it is transparent to user-space driver/middle.
Instead the kernel driver will decide which ring is to dispatch the BSD video
command.
As every BSD ring is powerful, it is enough to dispatch the BSD video command
based on the drm fd. In such case it can play back video stream while encoding
another video stream. The coarse ping-pong mechanism is used to determine
which BSD ring is used to dispatch the BSD video command.
V1->V2: Follow Daniel's comment and use the simple ping-pong mechanism.
This is only to add the support of dual BSD rings on BDW GT3 machine.
The further optimization will be considered in another patch set.
V2->V3: Follow Daniel's comment to use the struct_mutext instead of
atomic_t during determining which ring can be used to dispatch Video command.
Reviewed-by: Imre Deak
Signed-off-by: Zhao Yakui
---
drivers/gpu/drm/i915/i915_dma.c|3 +++
drivers/gpu/drm/i915/i915_drv.h|3 +++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 40 +++-
3 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0b38f88..f7558f5 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1572,6 +1572,7 @@ int i915_driver_load(struct drm_device *dev, unsigned
long flags)
spin_lock_init(&dev_priv->backlight_lock);
spin_lock_init(&dev_priv->uncore.lock);
spin_lock_init(&dev_priv->mm.object_stat_lock);
+ dev_priv->ring_index = 0;
mutex_init(&dev_priv->dpio_lock);
mutex_init(&dev_priv->modeset_restore_lock);
@@ -1929,6 +1930,8 @@ void i915_driver_postclose(struct drm_device *dev, struct
drm_file *file)
{
struct drm_i915_file_private *file_priv = file->driver_priv;
+ if (file_priv && file_priv->bsd_ring)
+ file_priv->bsd_ring = NULL;
kfree(file_priv);
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 74aef6a..032f992 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1472,6 +1472,8 @@ struct drm_i915_private {
struct i915_dri1_state dri1;
/* Old ums support infrastructure, same warning applies. */
struct i915_ums_state ums;
+ /* the indicator for dispatch video commands on two BSD rings */
+ int ring_index;
};
static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
@@ -1679,6 +1681,7 @@ struct drm_i915_file_private {
struct i915_hw_context *private_default_ctx;
atomic_t rps_wait_boost;
+ struct intel_ring_buffer *bsd_ring;
};
/*
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 341ec68..1dc6f03 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -999,6 +999,37 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
return 0;
}
+/**
+ * Find one BSD ring to dispatch the corresponding BSD command.
+ * The Ring ID is returned.
+ */
+static int gen8_dispatch_bsd_ring(struct drm_device *dev,
+ struct drm_file *file)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_file_private *file_priv = file->driver_priv;
+
+ /* Check whether the file_priv is using one ring */
+ if (file_priv->bsd_ring)
+ return file_priv->bsd_ring->id;
+ else {
+ /* If no, use the ping-pong mechanism to select one ring */
+ int ring_id;
+
+ mutex_lock(&dev->struct_mutex);
+ if (dev_priv->ring_index == 0) {
+ ring_id = VCS;
+ dev_priv->ring_index = 1;
+ } else {
+ ring_id = VCS2;
+ dev_priv->ring_index = 0;
+ }
+ file_priv->bsd_ring = &dev_priv->ring[ring_id];
+ mutex_unlock(&dev->struct_mutex);
+ return ring_id;
+ }
+}
+
static int
i915_gem_do_execbuffer(struct drm_device *dev, void *data,
struct drm_file *file,
@@ -1043,7 +1074,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT)
ring = &dev_priv->ring[RCS];
- else
+ else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) {
+ if (HAS_BSD2(dev)) {
+ int ring_id;
+ ring_id = gen8_dispatch_bsd_ring(dev, file);
+ ring = &dev_priv->ring[ring_id];
+ } else
+ ring = &dev_priv->ring[VCS];
+ } else
ring = &dev_priv->ring[(args->flags & I915_EXEC_RING_MASK) - 1];
if
[Intel-gfx] [PATCH V4 4/6] drm/i915:Handle the irq interrupt for the second BSD ring
Reviewed-by: Imre Deak
Signed-off-by: Zhao Yakui
---
drivers/gpu/drm/i915/i915_irq.c |5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7a4d3ae..63bd5de 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1347,13 +1347,16 @@ static irqreturn_t gen8_gt_irq_handler(struct
drm_device *dev,
DRM_ERROR("The master control interrupt lied (GT0)!\n");
}
- if (master_ctl & GEN8_GT_VCS1_IRQ) {
+ if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
tmp = I915_READ(GEN8_GT_IIR(1));
if (tmp) {
ret = IRQ_HANDLED;
vcs = tmp >> GEN8_VCS1_IRQ_SHIFT;
if (vcs & GT_RENDER_USER_INTERRUPT)
notify_ring(dev, &dev_priv->ring[VCS]);
+ vcs = tmp >> GEN8_VCS2_IRQ_SHIFT;
+ if (vcs & GT_RENDER_USER_INTERRUPT)
+ notify_ring(dev, &dev_priv->ring[VCS2]);
I915_WRITE(GEN8_GT_IIR(1), tmp);
} else
DRM_ERROR("The master control interrupt lied (GT1)!\n");
--
1.7.10.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH V4 2/6] drm/i915: Update the restrict check to filter out wrong Ring ID passed by user-space
Signed-off-by: Zhao Yakui
Reviewed-by: Imre Deak
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |2 +-
drivers/gpu/drm/i915/intel_ringbuffer.h|1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3491402..341ec68 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1035,7 +1035,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
if (args->flags & I915_EXEC_IS_PINNED)
flags |= I915_DISPATCH_PINNED;
- if ((args->flags & I915_EXEC_RING_MASK) > I915_NUM_RINGS) {
+ if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) {
DRM_DEBUG("execbuf with unknown ring: %d\n",
(int)(args->flags & I915_EXEC_RING_MASK));
return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 413cdc7..ec9d978 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -63,6 +63,7 @@ struct intel_ring_buffer {
VECS,
} id;
#define I915_NUM_RINGS 4
+#define LAST_USER_RING (VECS + 1)
u32 mmio_base;
void__iomem *virtual_start;
struct drm_device *dev;
--
1.7.10.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH V4 0/6] drm/i915: Add the support of dual BSD rings on BDW GT3
This is the patch set that tries to add the support of dual BSD rings on BDW GT3. Based on hardware spec, the BDW GT3 has two independent BSD rings, which can be used to process the video commands. To be simpler, it is transparent to user-space driver/middleware. In such case the kernel driver will decide which ring is to dispatch the BSD video command. As every BSD ring is powerful, it is enough to dispatch the BSD video command based on the drm fd. In such case the different BSD ring is used for video playing back and encoding. V1->V2: Follow Daniel's comment to do the following update: a. consider the stolen check for BDW in kernel/early-quirks.c in patch 01 b. update the comment in Patch 04 c. use the simple ping-pong mechanism to add the support of dual BSD rings. The further optimization will be considered in another patch set. V2->V3: Follow Daniel's comment to use the struct_mutext instead of atomic_t during determining which ring can be used to dispatch Video command. V3->V4: Follow Imre's comment to adjust the patch order and do some minor updates. For example: add some comments to describe the semaphore in Patch 03 and update the ring name for the second bsd ring. Zhao Yakui (6): drm/i915: Split the BDW device definition to prepare for dual BSD rings on BDW GT3 drm/i915: Update the restrict check to filter out wrong Ring ID passed by user-space drm/i915:Initialize the second BSD ring on BDW GT3 machine drm/i915:Handle the irq interrupt for the second BSD ring drm/i915:Add the VCS2 switch in Intel_ring_setup_status_page for Gen7 to remove the switch check warning drm/i915: Use the coarse ping-pong mechanism based on drm fd to dispatch the BSD command on BDW GT3 drivers/gpu/drm/i915/i915_dma.c|3 + drivers/gpu/drm/i915/i915_drv.c| 26 - drivers/gpu/drm/i915/i915_drv.h|5 ++ drivers/gpu/drm/i915/i915_gem.c|9 ++- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 42 +- drivers/gpu/drm/i915/i915_gpu_error.c |1 + drivers/gpu/drm/i915/i915_irq.c|5 +- drivers/gpu/drm/i915/i915_reg.h|1 + drivers/gpu/drm/i915/intel_ringbuffer.c| 83 drivers/gpu/drm/i915/intel_ringbuffer.h|5 +- include/drm/i915_pciids.h | 22 ++-- 11 files changed, 190 insertions(+), 12 deletions(-) -- 1.7.10.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V3 2/6] drm/i915:Initialize the second BSD ring on BDW GT3 machine
On Wed, 2014-04-16 at 10:23 -0600, Deak, Imre wrote:
> On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote:
> > Based on the hardware spec, the BDW GT3 machine has two independent
> > BSD ring that can be used to dispatch the video commands.
> > So just initialize it.
> >
> > Signed-off-by: Zhao Yakui
>
> A couple of nitpicks below, with or without those:
> Reviewed-by: Imre Deak
Hi, Imre
Thanks for your review and the comments.
I will update the patch based on your comment.
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c |4 +--
> > drivers/gpu/drm/i915/i915_drv.h |2 ++
> > drivers/gpu/drm/i915/i915_gem.c |9 +-
> > drivers/gpu/drm/i915/i915_gpu_error.c |1 +
> > drivers/gpu/drm/i915/i915_reg.h |1 +
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 54
> > +++
> > drivers/gpu/drm/i915/intel_ringbuffer.h |4 ++-
> > 7 files changed, 71 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 17fbbe5..2a7842b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -282,7 +282,7 @@ static const struct intel_device_info
> > intel_broadwell_m_info = {
> > static const struct intel_device_info intel_broadwell_gt3d_info = {
> > .gen = 8, .num_pipes = 3,
> > .need_gfx_hws = 1, .has_hotplug = 1,
> > - .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> > + .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> > .has_llc = 1,
> > .has_ddi = 1,
> > .has_fbc = 1,
> > @@ -292,7 +292,7 @@ static const struct intel_device_info
> > intel_broadwell_gt3d_info = {
> > static const struct intel_device_info intel_broadwell_gt3m_info = {
> > .gen = 8, .is_mobile = 1, .num_pipes = 3,
> > .need_gfx_hws = 1, .has_hotplug = 1,
> > - .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> > + .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> > .has_llc = 1,
> > .has_ddi = 1,
> > .has_fbc = 1,
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 92c3095..74aef6a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1833,7 +1833,9 @@ struct drm_i915_cmd_table {
> > #define BSD_RING (1< > #define BLT_RING (1< > #define VEBOX_RING (1< > +#define BSD2_RING (1< > #define HAS_BSD(dev)(INTEL_INFO(dev)->ring_mask & BSD_RING)
> > +#define HAS_BSD2(dev) (INTEL_INFO(dev)->ring_mask & BSD2_RING)
> > #define HAS_BLT(dev)(INTEL_INFO(dev)->ring_mask & BLT_RING)
> > #define HAS_VEBOX(dev)(INTEL_INFO(dev)->ring_mask & VEBOX_RING)
> > #define HAS_LLC(dev)(INTEL_INFO(dev)->has_llc)
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 85c9cf0..b4dcf2a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4374,13 +4374,20 @@ static int i915_gem_init_rings(struct drm_device
> > *dev)
> > goto cleanup_blt_ring;
> > }
> >
> > + if (HAS_BSD2(dev)) {
> > + ret = intel_init_bsd2_ring_buffer(dev);
> > + if (ret)
> > + goto cleanup_vebox_ring;
> > + }
> >
> > ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
> > if (ret)
> > - goto cleanup_vebox_ring;
> > + goto cleanup_ring;
>
> maybe cleanup_bsd2_ring?
>
> >
> > return 0;
> >
> > +cleanup_ring:
> > + intel_cleanup_ring_buffer(&dev_priv->ring[VCS2]);
> > cleanup_vebox_ring:
> > intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
> > cleanup_blt_ring:
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 4865ade..3cab7f9 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -42,6 +42,7 @@ static const char *ring_str(int ring)
> > case VCS: return "bsd";
> > case BCS: return "blt";
> > case VECS: return "vebox";
> > + case VCS2: return "second bsd";
>
> "bsd2" would be more concise
>
OK. I will update it.
> > default: return "";
> > }
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 8f84555..0b88508 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -760,6 +760,7 @@ enum punit_power_well {
> > #define RENDER_RING_BASE 0x02000
> > #define BSD_RING_BASE 0x04000
> > #define GEN6_BSD_RING_BASE 0x12000
> > +#define GEN8_BSD2_RING_BASE0x1c000
> > #define VEBOX_RING_BASE0x1a000
> > #define BLT_RING_BASE 0x22000
> > #define RING_TAIL(base)((base)+0x30)
> > diff --git a/drivers/gpu/drm/
Re: [Intel-gfx] REGRESSION 3.14 i915 warning & mouse cursor vanishing
On Wed, Apr 16, 2014 at 2:46 PM, Jani Nikula wrote: > On Tue, 15 Apr 2014, Imre Deak wrote: >> On Tue, 2014-04-15 at 21:43 +0200, Daniel Vetter wrote: >>> On Mon, Apr 14, 2014 at 11:56:03AM -0700, Steven Noonan wrote: >>> > On Mon, Apr 14, 2014 at 11:35:05AM -0700, Keith Packard wrote: >>> > > Steven Noonan writes: >>> > > >>> > > > Was using my machine normally, then my mouse cursor vanished. After >>> > > > switching >>> > > > to a VT and back to X11, my cursor came back. But I did notice a >>> > > > nasty trace in >>> > > > dmesg (below). >>> > > >>> > > I don't think the trace below is related to the cursor disappearing. >>> > >>> > Any idea what the trace is all about then? Seems it has something to do >>> > with runtime power management (maybe my aggressive kernel command-line >>> > options are triggering it). >>> >>> Please test without them. Currently runtime pm should be disabled still on >>> vlv (since it's incomplete in 3.14). If you've force-enabled that then you >>> get to keep all pieces ;-) >>> >>> In general don't set any i915 options if you're not a developer or someone >>> else who _really_ knows what's going on. >> >> Note that the lspci output and the >> >> [ 1795.275026] [drm:hsw_unclaimed_reg_clear] *ERROR* Unknown unclaimed >> register before writing to 70084 >> >> line suggests HSW and the specs for ThinkPad Yoga suggests the same. But >> I don't know how the vlv_* functions can possible end up in those traces >> then, perhaps just a coincidence, random data on stack? > > I'm wondering the same. Perhaps double check your kernel build and > modules are all right and matching? > It was a clean build (built in a clean chroot, no ccache or anything fancy), so those stack traces are as "right" as they could be under those conditions. The "good" news (or perhaps scary news) is that I've been running 3.14.1 for the past 36 hours and haven't been able to reproduce either problem since then (warnings or ninja mouse cursor). Nothing in the changelog for v3.14..v3.14.1 really stands out as a clear fix though. The only changes that appear to directly affect my configuration would be the futex changes, iwlwifi change, efi change, and ipv6 change. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] REGRESSION 3.14 i915 warning & mouse cursor vanishing
On Tue, 15 Apr 2014, Imre Deak wrote: > On Tue, 2014-04-15 at 21:43 +0200, Daniel Vetter wrote: >> On Mon, Apr 14, 2014 at 11:56:03AM -0700, Steven Noonan wrote: >> > On Mon, Apr 14, 2014 at 11:35:05AM -0700, Keith Packard wrote: >> > > Steven Noonan writes: >> > > >> > > > Was using my machine normally, then my mouse cursor vanished. After >> > > > switching >> > > > to a VT and back to X11, my cursor came back. But I did notice a nasty >> > > > trace in >> > > > dmesg (below). >> > > >> > > I don't think the trace below is related to the cursor disappearing. >> > >> > Any idea what the trace is all about then? Seems it has something to do >> > with runtime power management (maybe my aggressive kernel command-line >> > options are triggering it). >> >> Please test without them. Currently runtime pm should be disabled still on >> vlv (since it's incomplete in 3.14). If you've force-enabled that then you >> get to keep all pieces ;-) >> >> In general don't set any i915 options if you're not a developer or someone >> else who _really_ knows what's going on. > > Note that the lspci output and the > > [ 1795.275026] [drm:hsw_unclaimed_reg_clear] *ERROR* Unknown unclaimed > register before writing to 70084 > > line suggests HSW and the specs for ThinkPad Yoga suggests the same. But > I don't know how the vlv_* functions can possible end up in those traces > then, perhaps just a coincidence, random data on stack? I'm wondering the same. Perhaps double check your kernel build and modules are all right and matching? BR, Jani. > > For HSW the rc6 kernel option shouldn't make a difference. > > --Imre > > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/15] drm/i915: vlv: clean up GTLC wake control/status register macros
On Wed, 2014-04-16 at 18:08 -0300, Rodrigo Vivi wrote:
> Well, first of all I couldn't find the regs definitions. I decided to
> do this review during fly but I didn't had this regs definitions on
> VLV docs I have here with me. Could you please point me to the correct
> docs?
These regs are defined in the Gunit register HAS, I'll send you in
private the details.
> Anyway this patch isn't really modifying any behaviour. So I would
> tend to let my Reviewed-by: Rodrigo Vivi here
> anyway. Considering that the original author of changes plus the first
> reviewer already agreed with you.
Note that there is already a v2 of this patchset, so please review that
one.
--Imre
> On Tue, Apr 8, 2014 at 1:57 PM, Imre Deak wrote:
> > These will be needed by the upcoming VLV RPM helpers.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 5 +++--
> > drivers/gpu/drm/i915/i915_reg.h | 10 --
> > 2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 6370a76..d000d0f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4458,8 +4458,9 @@ int i915_gem_init(struct drm_device *dev)
> >
> > if (IS_VALLEYVIEW(dev)) {
> > /* VLVA0 (potential hack), BIOS isn't actually waking us */
> > - I915_WRITE(VLV_GTLC_WAKE_CTRL, 1);
> > - if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) & 1) == 1, 10))
> > + I915_WRITE(VLV_GTLC_WAKE_CTRL, VLV_GTLC_ALLOWWAKEREQ);
> > + if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) &
> > + VLV_GTLC_ALLOWWAKEACK), 10))
> > DRM_DEBUG_DRIVER("allow wake ack timed out\n");
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 8e60737..0997da3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4985,9 +4985,15 @@ enum punit_power_well {
> > #define FORCEWAKE_ACK_HSW 0x130044
> > #define FORCEWAKE_ACK 0x130090
> > #define VLV_GTLC_WAKE_CTRL0x130090
> > +#define VLV_GTLC_RENDER_CTX_EXISTS (1 << 25)
> > +#define VLV_GTLC_MEDIA_CTX_EXISTS(1 << 24)
> > +#define VLV_GTLC_ALLOWWAKEREQ(1 << 0)
> > +
> > #define VLV_GTLC_PW_STATUS0x130094
> > -#define VLV_GTLC_PW_RENDER_STATUS_MASK 0x80
> > -#define VLV_GTLC_PW_MEDIA_STATUS_MASK 0x20
> > +#define VLV_GTLC_ALLOWWAKEACK(1 << 0)
> > +#define VLV_GTLC_ALLOWWAKEERR(1 << 1)
> > +#define VLV_GTLC_PW_MEDIA_STATUS_MASK(1 << 5)
> > +#define VLV_GTLC_PW_RENDER_STATUS_MASK (1 << 7)
> > #define FORCEWAKE_MT 0xa188 /* multi-threaded */
> > #define FORCEWAKE_KERNEL 0x1
> > #define FORCEWAKE_USER 0x2
> > --
> > 1.8.4
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/15] drm/i915: vlv: clean up GTLC wake control/status register macros
Well, first of all I couldn't find the regs definitions. I decided to
do this review during fly but I didn't had this regs definitions on
VLV docs I have here with me. Could you please point me to the correct
docs?
Anyway this patch isn't really modifying any behaviour. So I would
tend to let my Reviewed-by: Rodrigo Vivi here
anyway. Considering that the original author of changes plus the first
reviewer already agreed with you.
On Tue, Apr 8, 2014 at 1:57 PM, Imre Deak wrote:
> These will be needed by the upcoming VLV RPM helpers.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_gem.c | 5 +++--
> drivers/gpu/drm/i915/i915_reg.h | 10 --
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6370a76..d000d0f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4458,8 +4458,9 @@ int i915_gem_init(struct drm_device *dev)
>
> if (IS_VALLEYVIEW(dev)) {
> /* VLVA0 (potential hack), BIOS isn't actually waking us */
> - I915_WRITE(VLV_GTLC_WAKE_CTRL, 1);
> - if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) & 1) == 1, 10))
> + I915_WRITE(VLV_GTLC_WAKE_CTRL, VLV_GTLC_ALLOWWAKEREQ);
> + if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) &
> + VLV_GTLC_ALLOWWAKEACK), 10))
> DRM_DEBUG_DRIVER("allow wake ack timed out\n");
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e60737..0997da3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4985,9 +4985,15 @@ enum punit_power_well {
> #define FORCEWAKE_ACK_HSW 0x130044
> #define FORCEWAKE_ACK 0x130090
> #define VLV_GTLC_WAKE_CTRL0x130090
> +#define VLV_GTLC_RENDER_CTX_EXISTS (1 << 25)
> +#define VLV_GTLC_MEDIA_CTX_EXISTS(1 << 24)
> +#define VLV_GTLC_ALLOWWAKEREQ(1 << 0)
> +
> #define VLV_GTLC_PW_STATUS0x130094
> -#define VLV_GTLC_PW_RENDER_STATUS_MASK 0x80
> -#define VLV_GTLC_PW_MEDIA_STATUS_MASK 0x20
> +#define VLV_GTLC_ALLOWWAKEACK(1 << 0)
> +#define VLV_GTLC_ALLOWWAKEERR(1 << 1)
> +#define VLV_GTLC_PW_MEDIA_STATUS_MASK(1 << 5)
> +#define VLV_GTLC_PW_RENDER_STATUS_MASK (1 << 7)
> #define FORCEWAKE_MT 0xa188 /* multi-threaded */
> #define FORCEWAKE_KERNEL 0x1
> #define FORCEWAKE_USER 0x2
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Updated drm-intel-testing
Hi all, New -testing cycle with cool stuff: - vlv infoframe fixes from Jesse - dsi/mipi fixes from Shobhit - gen8 pageflip fixes for LRI/SRM from Damien - cmd parser fixes from Brad Volkin - some prep patches for CHV, DRRS, ... - and tons of little things all over A bit earlier than usual since I'm heading off for an extended easter w/e, and also not with a hole lot of patches really. I expect a flood of reviewed patches when I'm back home next Tuesday ;-) Happy testing! Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 21/25] drm/i915: vlv: factor out vlv_force_gfx_clock
On Mon, Apr 14, 2014 at 08:24:42PM +0300, Imre Deak wrote:
> This will be needed by the VLV runtime PM helpers too, so factor it out.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.c | 37 +
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 16 ++--
> 3 files changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1f88917..0609f77 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -905,6 +905,43 @@ static void hsw_runtime_resume(struct drm_i915_private
> *dev_priv)
> hsw_disable_pc8(dev_priv);
> }
>
> +int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> +{
> + u32 val;
> + int err;
> +
> + val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
> + WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
> +
> +#define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
> + /* Wait for a previous force on/off to settle */
> + if (force_on) {
> + err = wait_for(!COND, 5);
> + if (err) {
> + DRM_ERROR("timeout waiting for GFX clock force-off
> (%08x)\n",
> + I915_READ(VLV_GTLC_SURVIVABILITY_REG));
> + return err;
> + }
> + }
We didn't do this part previously. Not sure if it's needed or not, but
maybe add a note to the commit message about this.
> +
> + val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
> + val &= ~VLV_GFX_CLK_FORCE_ON_BIT;
> + if (force_on)
> + val |= VLV_GFX_CLK_FORCE_ON_BIT;
> + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, val);
> +
> + if (!force_on)
> + return 0;
> +
> + err = wait_for(COND, 5);
> + if (err)
> + DRM_ERROR("timeout waiting for GFX clock force-on (%08x)\n",
> + I915_READ(VLV_GTLC_SURVIVABILITY_REG));
> +
> + return err;
> +#undef COND
> +}
> +
> static int intel_runtime_suspend(struct device *device)
> {
> struct pci_dev *pdev = to_pci_dev(device);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5254f4b..3cac434 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1968,6 +1968,7 @@ extern unsigned long i915_chipset_val(struct
> drm_i915_private *dev_priv);
> extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
> extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
> extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
> +int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>
> extern void intel_console_resume(struct work_struct *work);
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bc38213..5a61075 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3129,16 +3129,7 @@ static void vlv_set_rps_idle(struct drm_i915_private
> *dev_priv)
> /* Mask turbo interrupt so that they will not come in between */
> I915_WRITE(GEN6_PMINTRMSK, 0x);
>
> - /* Bring up the Gfx clock */
> - I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> - I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
> - VLV_GFX_CLK_FORCE_ON_BIT);
> -
> - if (wait_for(((VLV_GFX_CLK_STATUS_BIT &
> - I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
> - DRM_ERROR("GFX_CLK_ON request timed out\n");
> - return;
> - }
> + vlv_force_gfx_clock(dev_priv, true);
>
> dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
>
> @@ -3149,10 +3140,7 @@ static void vlv_set_rps_idle(struct drm_i915_private
> *dev_priv)
> & GENFREQSTATUS) == 0, 5))
> DRM_ERROR("timed out waiting for Punit\n");
>
> - /* Release the Gfx clock */
> - I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> - I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
> - ~VLV_GFX_CLK_FORCE_ON_BIT);
> + vlv_force_gfx_clock(dev_priv, false);
>
> I915_WRITE(GEN6_PMINTRMSK,
> gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 19/25] drm/i915: reinit GT power save during resume
On Mon, Apr 14, 2014 at 08:24:40PM +0300, Imre Deak wrote:
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b87109c..1f88917 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -920,6 +920,7 @@ static int intel_runtime_suspend(struct device *device)
> DRM_DEBUG_KMS("Suspending device\n");
>
> intel_runtime_pm_disable_interrupts(dev);
> + cancel_work_sync(&dev_priv->rps.work);
What happens if the rps work still runs after
intel_runtime_pm_disable_interrupts()? To me it looks like it can hit
the WARN in snb_update_pm_irq(), or there's a race and it'll miss the
dev_priv->pm.irqs_disabled update and unmask something in PMIMR.
Hmm, I guess the same issue already exists in intel_disable_gt_powersave().
But since it's just PMIMR I guess it's not really dangerous since IER
isn't touched so we don't actually get any interrupts. But it feels a bit
fragile.
>
> if (IS_GEN6(dev))
> ;
> @@ -968,6 +969,7 @@ static int intel_runtime_resume(struct device *device)
>
> i915_gem_init_swizzling(dev);
> gen6_update_ring_freq(dev);
> + intel_reset_gt_powersave(dev);
>
> intel_runtime_pm_restore_interrupts(dev);
>
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure
On 25/03/2014 13:23, Chris Wilson wrote:
Try to flush out dirty pages into the swapcache (and from there into the
swapfile) when under memory pressure and forced to drop GEM objects from
memory. In effect, this should just allow us to discard unused pages for
memory reclaim and to start writeback earlier.
v2: Hugh Dickins warned that explicitly starting writeback from
shrink_slab was prone to deadlocks within shmemfs.
Cc: Hugh Dickins
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gem.c | 38 +++---
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 135ee8bd55f6..8287fd6701c6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker
*shrinker,
struct shrink_control *sc);
static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long
target);
static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
-static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
static bool cpu_cache_is_coherent(struct drm_device *dev,
@@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void
*data,
return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
}
+static inline int
+i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
+{
+ return obj->madv == I915_MADV_DONTNEED;
+}
+
/* Immediately discard the backing storage */
static void
i915_gem_object_truncate(struct drm_i915_gem_object *obj)
{
- struct inode *inode;
-
i915_gem_object_free_mmap_offset(obj);
if (obj->base.filp == NULL)
@@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object
*obj)
* To do this we must instruct the shmfs to drop all of its
* backing pages, *now*.
*/
- inode = file_inode(obj->base.filp);
- shmem_truncate_range(inode, 0, (loff_t)-1);
-
+ shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
obj->madv = __I915_MADV_PURGED;
}
-static inline int
-i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
+/* Try to discard unwanted pages */
+static void
+i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
{
- return obj->madv == I915_MADV_DONTNEED;
+ struct address_space *mapping;
+
+ switch (obj->madv) {
+ case I915_MADV_DONTNEED:
+ i915_gem_object_truncate(obj);
+ case __I915_MADV_PURGED:
+ return;
+ }
+
+ if (obj->base.filp == NULL)
+ return;
+
+ mapping = file_inode(obj->base.filp)->i_mapping,
+ invalidate_mapping_pages(mapping, 0, (loff_t)-1);
}
static void
@@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
ops->put_pages(obj);
obj->pages = NULL;
- if (i915_gem_object_is_purgeable(obj))
- i915_gem_object_truncate(obj);
+ i915_gem_object_invalidate(obj);
return 0;
}
@@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
if (WARN_ON(obj->pages_pin_count))
obj->pages_pin_count = 0;
+ if (obj->madv != __I915_MADV_PURGED)
+ obj->madv = I915_MADV_DONTNEED;
i915_gem_object_put_pages(obj);
i915_gem_object_free_mmap_offset(obj);
i915_gem_object_release_stolen(obj);
Functionally it looks good to me.
Though, you may want a /* fall-through */ comment (some people cant
mentally parse fallthroughs without being prompted) and a default:
break; (to avoid any static code analysis complaints) in the switch in
i915_gem_object_invalidate.
Reviewed-by: Robert Beckett
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 17/25] drm/i915: factor out gen6_update_ring_freq
On Mon, Apr 14, 2014 at 08:24:38PM +0300, Imre Deak wrote:
> This is needed by the next patch moving the call out from platform
> specific RPM callbacks to platform independent code.
>
> No functional change.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 --
> drivers/gpu/drm/i915/intel_display.c | 2 --
> drivers/gpu/drm/i915/intel_pm.c | 18 +++---
> 3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f3f9a33..afc31e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -899,9 +899,7 @@ static void snb_runtime_resume(struct drm_i915_private
> *dev_priv)
>
> intel_init_pch_refclk(dev);
> i915_gem_init_swizzling(dev);
> - mutex_lock(&dev_priv->rps.hw_lock);
> gen6_update_ring_freq(dev);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> }
>
> static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index bda79ec..596ae69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7088,9 +7088,7 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>
> intel_prepare_ddi(dev);
> i915_gem_init_swizzling(dev);
> - mutex_lock(&dev_priv->rps.hw_lock);
> gen6_update_ring_freq(dev);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> }
>
> static void snb_modeset_global_resources(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3068f51..f88d64d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3522,7 +3522,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> }
>
> -void gen6_update_ring_freq(struct drm_device *dev)
> +static void __gen6_update_ring_freq(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> int min_freq = 15;
> @@ -3592,6 +3592,18 @@ void gen6_update_ring_freq(struct drm_device *dev)
> }
> }
>
> +void gen6_update_ring_freq(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (!(INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev)))
This is a bit hard to parse.
if (gen < 6 || VLV)
would be easier.
> + return;
> +
> + mutex_lock(&dev_priv->rps.hw_lock);
> + __gen6_update_ring_freq(dev);
> + mutex_unlock(&dev_priv->rps.hw_lock);
> +}
> +
> int valleyview_rps_max_freq(struct drm_i915_private *dev_priv)
> {
> u32 val, rp0;
> @@ -4563,10 +4575,10 @@ static void intel_gen6_powersave_work(struct
> work_struct *work)
> valleyview_enable_rps(dev);
> } else if (IS_BROADWELL(dev)) {
> gen8_enable_rps(dev);
> - gen6_update_ring_freq(dev);
> + __gen6_update_ring_freq(dev);
> } else {
> gen6_enable_rps(dev);
> - gen6_update_ring_freq(dev);
> + __gen6_update_ring_freq(dev);
> }
> dev_priv->rps.enabled = true;
> mutex_unlock(&dev_priv->rps.hw_lock);
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V3 6/6] drm/i915: Use the coarse ping-pong mechanism based on drm fd to dispatch the BSD command on BDW GT3
On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote:
> The BDW GT3 has two independent BSD rings, which can be used to process the
> video commands. To be simpler, it is transparent to user-space driver/middle.
> Instead the kernel driver will decide which ring is to dispatch the BSD video
> command.
>
> As every BSD ring is powerful, it is enough to dispatch the BSD video command
> based on the drm fd. In such case it can play back video stream while encoding
> another video stream. The coarse ping-pong mechanism is used to determine
> which BSD ring is used to dispatch the BSD video command.
>
> V1->V2: Follow Daniel's comment and use the simple ping-pong mechanism.
> This is only to add the support of dual BSD rings on BDW GT3 machine.
> The further optimization will be considered in another patch set.
>
> V2->V3: Follow Daniel's comment to use the struct_mutext instead of
> atomic_t during determining which ring can be used to dispatch Video command.
>
> Signed-off-by: Zhao Yakui
Reviewed-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_dma.c|3 +++
> drivers/gpu/drm/i915/i915_drv.h|3 +++
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 40
> +++-
> 3 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 0b38f88..f7558f5 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1572,6 +1572,7 @@ int i915_driver_load(struct drm_device *dev, unsigned
> long flags)
> spin_lock_init(&dev_priv->backlight_lock);
> spin_lock_init(&dev_priv->uncore.lock);
> spin_lock_init(&dev_priv->mm.object_stat_lock);
> + dev_priv->ring_index = 0;
> mutex_init(&dev_priv->dpio_lock);
> mutex_init(&dev_priv->modeset_restore_lock);
>
> @@ -1929,6 +1930,8 @@ void i915_driver_postclose(struct drm_device *dev,
> struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> + if (file_priv && file_priv->bsd_ring)
> + file_priv->bsd_ring = NULL;
> kfree(file_priv);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 74aef6a..032f992 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1472,6 +1472,8 @@ struct drm_i915_private {
> struct i915_dri1_state dri1;
> /* Old ums support infrastructure, same warning applies. */
> struct i915_ums_state ums;
> + /* the indicator for dispatch video commands on two BSD rings */
> + int ring_index;
> };
>
> static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> @@ -1679,6 +1681,7 @@ struct drm_i915_file_private {
>
> struct i915_hw_context *private_default_ctx;
> atomic_t rps_wait_boost;
> + struct intel_ring_buffer *bsd_ring;
> };
>
> /*
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 341ec68..1dc6f03 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -999,6 +999,37 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
> return 0;
> }
>
> +/**
> + * Find one BSD ring to dispatch the corresponding BSD command.
> + * The Ring ID is returned.
> + */
> +static int gen8_dispatch_bsd_ring(struct drm_device *dev,
> + struct drm_file *file)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_file_private *file_priv = file->driver_priv;
> +
> + /* Check whether the file_priv is using one ring */
> + if (file_priv->bsd_ring)
> + return file_priv->bsd_ring->id;
> + else {
> + /* If no, use the ping-pong mechanism to select one ring */
> + int ring_id;
> +
> + mutex_lock(&dev->struct_mutex);
> + if (dev_priv->ring_index == 0) {
> + ring_id = VCS;
> + dev_priv->ring_index = 1;
> + } else {
> + ring_id = VCS2;
> + dev_priv->ring_index = 0;
> + }
> + file_priv->bsd_ring = &dev_priv->ring[ring_id];
> + mutex_unlock(&dev->struct_mutex);
> + return ring_id;
> + }
> +}
> +
> static int
> i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> struct drm_file *file,
> @@ -1043,7 +1074,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
> *data,
>
> if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT)
> ring = &dev_priv->ring[RCS];
> - else
> + else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) {
> + if (HAS_BSD2(dev)) {
> + int ring_id;
> + ring_id = gen8_dispatch_bsd_ring(dev, file);
> + ring = &dev_p
Re: [Intel-gfx] [PATCH V3 5/6] drm/i915: Update the restrict check to filter out wrong Ring ID passed by user-space
On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote:
> One extra ring is added in the kernel driver but it is transparent to the
> user-space application/middleware. In such case the number of the rings
> in kernel driver is bigger than that exported to the user-space. So
> it needs to filter out the wrong Ring ID passed by user-space.
>
> Signed-off-by: Zhao Yakui
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c |2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.h|1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 3491402..341ec68 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1035,7 +1035,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
> *data,
> if (args->flags & I915_EXEC_IS_PINNED)
> flags |= I915_DISPATCH_PINNED;
>
> - if ((args->flags & I915_EXEC_RING_MASK) > I915_NUM_RINGS) {
> + if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) {
> DRM_DEBUG("execbuf with unknown ring: %d\n",
> (int)(args->flags & I915_EXEC_RING_MASK));
> return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 8ca4285..59f4cdd 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -64,6 +64,7 @@ struct intel_ring_buffer {
> VCS2,
> } id;
> #define I915_NUM_RINGS 5
> +#define LAST_USER_RING (VECS + 1)
Defining this in the enum is cleaner IMO. Also this patch should ideally
be applied before 2/6 for bisectability. In any case:
Reviewed-by: Imre Deak
> u32 mmio_base;
> void__iomem *virtual_start;
> struct drm_device *dev;
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] Reduce intel_display.c
On Wed, Apr 16, 2014 at 07:37:16PM +0300, Ville Syrjälä wrote: > On Tue, Apr 15, 2014 at 09:25:44PM +0200, Daniel Vetter wrote: > > On Fri, Apr 11, 2014 at 10:21:59AM +0300, Jani Nikula wrote: > > > On Fri, 11 Apr 2014, Ben Widawsky wrote: > > > > On Wed, Apr 09, 2014 at 06:44:29PM -0300, Paulo Zanoni wrote: > > > >> From: Paulo Zanoni > > > >> > > > >> Hi > > > >> > > > >> We always talk about how intel_display.c is a giant file and how we > > > >> would like > > > >> to reduce it, so this is my attempt. Currently the file has 12090 > > > >> lines, and > > > >> after my patch series it has 8850 lines. > > > >> > > > >> I don't know if right now is the appropriate time to merge patches > > > >> like this. I > > > >> don't remember seeing too many patches on the list touching > > > >> cursor/fdi/eld/pll > > > >> functions, but I know there is never an appropriate time for huge > > > >> changes. > > > >> > > > >> Also, this change will obviously make the lives of people who backport > > > >> our > > > >> patches more complicated. So if we don't want this series at all, feel > > > >> free to > > > >> NACK it. > > > > > > > > I am only responding because it seems nobody else really said much. I > > > > never touch this code, and I shouldn't be the authority. I really > > > > quickly glanced at the patches. > > > > > > > > 1. +LOC: It sucks that you ended up adding 220 lines. I assume half of > > > > it is the > > > > copyright header, but still, considering there are no actual refactors, > > > > cleanups, or functional changes - adding lines makes me unhappy. > > > > > > > > 2. necessary? I personally haven't heard from anyone that we need to > > > > shrink > > > > intel_display.c (again, I am the furthest from being an expert). I doubt > > > > anyone isn't using some form of tags, or grep to navigate anyway. My > > > > problem has never been the file size itself, but just the structure of > > > > the display code interacting with the core KMS was hard to follow. > > > > > > > > 3. conflicts: Like you said, it's likely nobody touches this code, but > > > > we should > > > > keep in mind we do have several people working on older branches, and > > > > this kind of thing makes any sort of backport hard. > > > > > > > > On the other hand: > > > > 1. If more than one person finds the results more readable/consumable, I > > > > think it's worth it, and probably mostly justifies doing it. You've also > > > > shrunk the file by quite a bit, so it's somewhat useful churn. > > > > > > > > 2. intel_pll.c sounds like a good idea > > > > > > I'm in favour of reducing the size of intel_display.c. I did not look at > > > the patches though, because I've sent a few patches to this effect in > > > the past (limits/pll and quirks at least), but they were stalled because > > > they were in the collision course with the Grand Plans Daniel has. So > > > Daniel, I expect you to chime in on this one too. ;) > > > > Chiming in now ;-) > > > > I've seen a few "extract stuff" patches float by and occasionally also > > merged some, but thus far I' haven't been terribly convinced. I don't mind > > the conflicts these patches cause, but if we want to reorg the driver the > > goal shouldn't be to just make files smaller (cscope can cope) but > > actually improve the organization of all this. > > > > Often these patches just grab a bag of functions with related names, throw > > them all into a new file and then add forward and header declaration until > > the damn thing compiles again. What we want instead are real code modules > > where interactions within one file are high and the number of exported > > functions fairly low. > > > > Two examples: > > - Extracting the pageflip helpers and related code would mean that we also > > should extract a new pageflip_init functions, so that all the platform > > functions don't need to be exported. We've done similar things when > > creating intel_unocore.c. > > > > - I've just stumbled around in the haswell code and noticed that pretty > > much all the lpt and hsw fdi code could be moved into intel_crt.c with a > > new hsw specific crt encoder. In the pre_enable and post_disable hooks > > we could then do the ddi setup, fdi link training. And all the ltp > > handling code could mostly be moved away too. With this we could also > > remove a lot (if not all) of the has_pch_encoder checks and I think also > > many type == INTEL_OUTPUT_ANALOG checks in the haswell code. > > > > I haven't actually checked whether it'll all nicely work out, but my gut > > feeling says it'll be fewer forward declarations than just shoveling all > > the fdi related functions (including the lpt stuff) into a new > > intel_fdi.c. > > I'm not sure I'd like the code for one pch platform to have a totally > different structure than the other platforms. I guess I can't really say > w/o seeing the result. And this is hsw after all where the code likes be > different just because,
Re: [Intel-gfx] [PATCH V3 4/6] drm/i915:Add the VCS2 switch in Intel_ring_setup_status_page for Gen7 to remove the switch check warning
On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote: > The Gen7 doesn't have the second BSD ring. But it will complain the switch > check > warning message during compilation. So just add it to remove the > switch check warning. > > V1->V2: Follow Daniel's comment to update the comment > > Signed-off-by: Zhao Yakui > --- > drivers/gpu/drm/i915/intel_ringbuffer.c |5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 8b9b89080..2c89525 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -988,6 +988,11 @@ void intel_ring_setup_status_page(struct > intel_ring_buffer *ring) > case BCS: > mmio = BLT_HWS_PGA_GEN7; > break; > + /* > + * VCS2 actually doesn't exist on Gen7. Only shut up > + * gcc switch check warning > + */ > + case VCS2: A WARN would've been better here, but in any case: Reviewed-by: Imre Deak > case VCS: > mmio = BSD_HWS_PGA_GEN7; > break; signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V3 3/6] drm/i915:Handle the irq interrupt for the second BSD ring
On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote:
> Signed-off-by: Zhao Yakui
Reviewed-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7a4d3ae..63bd5de 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1347,13 +1347,16 @@ static irqreturn_t gen8_gt_irq_handler(struct
> drm_device *dev,
> DRM_ERROR("The master control interrupt lied (GT0)!\n");
> }
>
> - if (master_ctl & GEN8_GT_VCS1_IRQ) {
> + if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
> tmp = I915_READ(GEN8_GT_IIR(1));
> if (tmp) {
> ret = IRQ_HANDLED;
> vcs = tmp >> GEN8_VCS1_IRQ_SHIFT;
> if (vcs & GT_RENDER_USER_INTERRUPT)
> notify_ring(dev, &dev_priv->ring[VCS]);
> + vcs = tmp >> GEN8_VCS2_IRQ_SHIFT;
> + if (vcs & GT_RENDER_USER_INTERRUPT)
> + notify_ring(dev, &dev_priv->ring[VCS2]);
> I915_WRITE(GEN8_GT_IIR(1), tmp);
> } else
> DRM_ERROR("The master control interrupt lied (GT1)!\n");
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] Reduce intel_display.c
On Tue, Apr 15, 2014 at 09:25:44PM +0200, Daniel Vetter wrote: > On Fri, Apr 11, 2014 at 10:21:59AM +0300, Jani Nikula wrote: > > On Fri, 11 Apr 2014, Ben Widawsky wrote: > > > On Wed, Apr 09, 2014 at 06:44:29PM -0300, Paulo Zanoni wrote: > > >> From: Paulo Zanoni > > >> > > >> Hi > > >> > > >> We always talk about how intel_display.c is a giant file and how we > > >> would like > > >> to reduce it, so this is my attempt. Currently the file has 12090 lines, > > >> and > > >> after my patch series it has 8850 lines. > > >> > > >> I don't know if right now is the appropriate time to merge patches like > > >> this. I > > >> don't remember seeing too many patches on the list touching > > >> cursor/fdi/eld/pll > > >> functions, but I know there is never an appropriate time for huge > > >> changes. > > >> > > >> Also, this change will obviously make the lives of people who backport > > >> our > > >> patches more complicated. So if we don't want this series at all, feel > > >> free to > > >> NACK it. > > > > > > I am only responding because it seems nobody else really said much. I > > > never touch this code, and I shouldn't be the authority. I really > > > quickly glanced at the patches. > > > > > > 1. +LOC: It sucks that you ended up adding 220 lines. I assume half of it > > > is the > > > copyright header, but still, considering there are no actual refactors, > > > cleanups, or functional changes - adding lines makes me unhappy. > > > > > > 2. necessary? I personally haven't heard from anyone that we need to > > > shrink > > > intel_display.c (again, I am the furthest from being an expert). I doubt > > > anyone isn't using some form of tags, or grep to navigate anyway. My > > > problem has never been the file size itself, but just the structure of > > > the display code interacting with the core KMS was hard to follow. > > > > > > 3. conflicts: Like you said, it's likely nobody touches this code, but we > > > should > > > keep in mind we do have several people working on older branches, and > > > this kind of thing makes any sort of backport hard. > > > > > > On the other hand: > > > 1. If more than one person finds the results more readable/consumable, I > > > think it's worth it, and probably mostly justifies doing it. You've also > > > shrunk the file by quite a bit, so it's somewhat useful churn. > > > > > > 2. intel_pll.c sounds like a good idea > > > > I'm in favour of reducing the size of intel_display.c. I did not look at > > the patches though, because I've sent a few patches to this effect in > > the past (limits/pll and quirks at least), but they were stalled because > > they were in the collision course with the Grand Plans Daniel has. So > > Daniel, I expect you to chime in on this one too. ;) > > Chiming in now ;-) > > I've seen a few "extract stuff" patches float by and occasionally also > merged some, but thus far I' haven't been terribly convinced. I don't mind > the conflicts these patches cause, but if we want to reorg the driver the > goal shouldn't be to just make files smaller (cscope can cope) but > actually improve the organization of all this. > > Often these patches just grab a bag of functions with related names, throw > them all into a new file and then add forward and header declaration until > the damn thing compiles again. What we want instead are real code modules > where interactions within one file are high and the number of exported > functions fairly low. > > Two examples: > - Extracting the pageflip helpers and related code would mean that we also > should extract a new pageflip_init functions, so that all the platform > functions don't need to be exported. We've done similar things when > creating intel_unocore.c. > > - I've just stumbled around in the haswell code and noticed that pretty > much all the lpt and hsw fdi code could be moved into intel_crt.c with a > new hsw specific crt encoder. In the pre_enable and post_disable hooks > we could then do the ddi setup, fdi link training. And all the ltp > handling code could mostly be moved away too. With this we could also > remove a lot (if not all) of the has_pch_encoder checks and I think also > many type == INTEL_OUTPUT_ANALOG checks in the haswell code. > > I haven't actually checked whether it'll all nicely work out, but my gut > feeling says it'll be fewer forward declarations than just shoveling all > the fdi related functions (including the lpt stuff) into a new > intel_fdi.c. I'm not sure I'd like the code for one pch platform to have a totally different structure than the other platforms. I guess I can't really say w/o seeing the result. And this is hsw after all where the code likes be different just because, which usually makes it the last platform I think about. And my last hsw related patches got stuck in limbo anyway, so I now prefer to keep my distance. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-
Re: [Intel-gfx] [PATCH V3 2/6] drm/i915:Initialize the second BSD ring on BDW GT3 machine
On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote:
> Based on the hardware spec, the BDW GT3 machine has two independent
> BSD ring that can be used to dispatch the video commands.
> So just initialize it.
>
> Signed-off-by: Zhao Yakui
A couple of nitpicks below, with or without those:
Reviewed-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.c |4 +--
> drivers/gpu/drm/i915/i915_drv.h |2 ++
> drivers/gpu/drm/i915/i915_gem.c |9 +-
> drivers/gpu/drm/i915/i915_gpu_error.c |1 +
> drivers/gpu/drm/i915/i915_reg.h |1 +
> drivers/gpu/drm/i915/intel_ringbuffer.c | 54
> +++
> drivers/gpu/drm/i915/intel_ringbuffer.h |4 ++-
> 7 files changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 17fbbe5..2a7842b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -282,7 +282,7 @@ static const struct intel_device_info
> intel_broadwell_m_info = {
> static const struct intel_device_info intel_broadwell_gt3d_info = {
> .gen = 8, .num_pipes = 3,
> .need_gfx_hws = 1, .has_hotplug = 1,
> - .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> + .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> .has_llc = 1,
> .has_ddi = 1,
> .has_fbc = 1,
> @@ -292,7 +292,7 @@ static const struct intel_device_info
> intel_broadwell_gt3d_info = {
> static const struct intel_device_info intel_broadwell_gt3m_info = {
> .gen = 8, .is_mobile = 1, .num_pipes = 3,
> .need_gfx_hws = 1, .has_hotplug = 1,
> - .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> + .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> .has_llc = 1,
> .has_ddi = 1,
> .has_fbc = 1,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 92c3095..74aef6a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1833,7 +1833,9 @@ struct drm_i915_cmd_table {
> #define BSD_RING (1< #define BLT_RING (1< #define VEBOX_RING (1< +#define BSD2_RING(1< #define HAS_BSD(dev)(INTEL_INFO(dev)->ring_mask & BSD_RING)
> +#define HAS_BSD2(dev)(INTEL_INFO(dev)->ring_mask & BSD2_RING)
> #define HAS_BLT(dev)(INTEL_INFO(dev)->ring_mask & BLT_RING)
> #define HAS_VEBOX(dev)(INTEL_INFO(dev)->ring_mask & VEBOX_RING)
> #define HAS_LLC(dev)(INTEL_INFO(dev)->has_llc)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 85c9cf0..b4dcf2a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4374,13 +4374,20 @@ static int i915_gem_init_rings(struct drm_device *dev)
> goto cleanup_blt_ring;
> }
>
> + if (HAS_BSD2(dev)) {
> + ret = intel_init_bsd2_ring_buffer(dev);
> + if (ret)
> + goto cleanup_vebox_ring;
> + }
>
> ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
> if (ret)
> - goto cleanup_vebox_ring;
> + goto cleanup_ring;
maybe cleanup_bsd2_ring?
>
> return 0;
>
> +cleanup_ring:
> + intel_cleanup_ring_buffer(&dev_priv->ring[VCS2]);
> cleanup_vebox_ring:
> intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
> cleanup_blt_ring:
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 4865ade..3cab7f9 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -42,6 +42,7 @@ static const char *ring_str(int ring)
> case VCS: return "bsd";
> case BCS: return "blt";
> case VECS: return "vebox";
> + case VCS2: return "second bsd";
"bsd2" would be more concise
> default: return "";
> }
> }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8f84555..0b88508 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -760,6 +760,7 @@ enum punit_power_well {
> #define RENDER_RING_BASE 0x02000
> #define BSD_RING_BASE0x04000
> #define GEN6_BSD_RING_BASE 0x12000
> +#define GEN8_BSD2_RING_BASE 0x1c000
> #define VEBOX_RING_BASE 0x1a000
> #define BLT_RING_BASE0x22000
> #define RING_TAIL(base) ((base)+0x30)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index eb3dd26..8b9b89080 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1924,10 +1924,12 @@ int intel_init_render_ring_buffer(struct drm_device
> *dev)
> ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_RV;
>
Re: [Intel-gfx] The whole round of i-g-t testing cost too long running time
On Wed, Apr 16, 2014 at 05:50:20PM +0200, Daniel Vetter wrote: > On 16/04/2014 17:42, Jesse Barnes wrote: > > On Tue, 15 Apr 2014 19:17:59 +0200 > > Daniel Vetter wrote: > >> Ok there are a few cases where we can indeed make tests faster, but it > >> will be work for us. And that won't really speed up much since we're > >> adding piles more testcases at a pretty quick rate. And many of these > >> new testcases are CRC based, so inheritely take some time to run. > > But each test should run very quickly in general; I think we have too > > many tests that take much longer than they need to. Adding some > > hooks to the driver via debugfs may let us trigger specific cases > > directly rather than trying to induce them through massive threading > > and memory pressure for example. > > > > And can you elaborate on the CRC tests? It doesn't seem like those > > should take more than a few frames to verify we're getting what we > > expect... > > Well they don't take more than a few frames, but we have a _lot_ of > them, and there's a lot of cominations to test. It adds up quickly. Iirc > we have over 150 kms_flip testcases alone ... Many kms_flips tests are more like "run this thing for a few (dozen) seconds and see if there's something unexpected reported". They don't really try to hit specific issues. Some of the more targeted crc tests do run much quicker, but I'm guessing there are still at least two factors that add up. The actual modesets that the tests have to do cost time. And then there's the num_crtcs * num_connectors factor that tends to lengthen the execution time considerably. If we could run more tests for each pipe/connector pair w/o doing intervening modesets could probably cut the time down somewhat. But that sounds like a fairly massive undertaking as all the tests would have to be in a library and get called from some generic test runner code that does the modeset part. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 25/25] drm/i915: vlv: add runtime PM support
On Wed, 2014-04-16 at 16:53 +0200, Daniel Vetter wrote:
> On Mon, Apr 14, 2014 at 08:24:46PM +0300, Imre Deak wrote:
> > Add runtime PM support for VLV, but leave it disabled. The next patch
> > enables it.
> >
> > The suspend/resume sequence used is based on [1] and [2]. In practice we
> > depend on the GT RC6 mechanism to save the HW context depending on the
> > render and media power wells. By the time we run the runtime suspend
> > callback the display side is also off and the HW context for that is
> > managed by the display power domain framework.
> >
> > Besides the above there are Gunit registers that depend on a system-wide
> > power well. This power well goes off once the device enters any of the
> > S0i[R123] states. To handle this scenario, save/restore these Gunit
> > registers. Note that this is not the complete register set dictated by
> > [2], to remove some overhead, registers that are known not to be used are
> > ignored. Also some registers are fully setup by initialization functions
> > called during resume, these are not saved either. The list of registers
> > can be further reduced, see the TODO note in the code.
> >
> > [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> > [2] VLV2_S0IXRegs
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 327
> >
> > drivers/gpu/drm/i915/i915_drv.h | 62
> > 2 files changed, 389 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 08e210c..bc206dd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -911,6 +911,198 @@ static int hsw_runtime_resume(struct drm_i915_private
> > *dev_priv)
> > return 0;
> > }
> >
> > +/*
> > + * Save all Gunit registers that may be lost after a D3 and a subsequent
> > + * S0i[R123] transition. The list of registers needing a save/restore is
> > + * defined in the VLV2_S0IXRegs document. This documents marks all Gunit
> > + * registers in the following way:
> > + * - Driver: saved/restored by the driver
> > + * - Punit : saved/restored by the Punit firmware
> > + * - No, w/o marking: no need to save/restore, since the register is R/O or
> > + *used internally by the HW in a way that doesn't
> > depend
> > + *keeping the content across a suspend/resume.
> > + * - Debug : used for debugging
> > + *
> > + * We save/restore all registers marked with 'Driver', with the following
> > + * exceptions:
> > + * - Registers out of use, including also registers marked with 'Debug'.
> > + * These have no effect on the driver's operation, so we don't
> > save/restore
> > + * them to reduce the overhead.
> > + * - Registers that are fully setup by an initialization function called
> > from
> > + * the resume path. For example many clock gating and RPS/RC6 registers.
> > + * - Registers that provide the right functionality with their reset
> > defaults.
> > + *
> > + * TODO: Except for registers that based on the above 3 criteria can be
> > safely
> > + * ignored, we save/restore all others, practically treating the HW
> > context as
> > + * a black-box for the driver. Further investigation is needed to reduce
> > the
> > + * saved/restored registers even further, by following the same 3 criteria.
> > + */
> > +static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> > +{
> > + struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
> > + int i;
> > +
> > + /* GAM 0x4000-0x4770 */
> > + s->wr_watermark = I915_READ(GEN7_WR_WATERMARK);
> > + s->gfx_prio_ctrl= I915_READ(GEN7_GFX_PRIO_CTRL);
> > + s->arb_mode = I915_READ(ARB_MODE);
> > + s->gfx_pend_tlb0= I915_READ(GEN7_GFX_PEND_TLB0);
> > + s->gfx_pend_tlb1= I915_READ(GEN7_GFX_PEND_TLB1);
> > +
> > + for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++)
> > + s->lra_limits[i] = I915_READ(GEN7_LRA_LIMITS_BASE + i * 4);
> > +
> > + s->media_max_req_count = I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
> > + s->gfx_max_req_count= I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
> > +
> > + s->render_hwsp = I915_READ(RENDER_HWS_PGA_GEN7);
> > + s->ecochk = I915_READ(GAM_ECOCHK);
> > + s->bsd_hwsp = I915_READ(BSD_HWS_PGA_GEN7);
> > + s->blt_hwsp = I915_READ(BLT_HWS_PGA_GEN7);
> > +
> > + s->tlb_rd_addr = I915_READ(GEN7_TLB_RD_ADDR);
> > +
> > + /* MBC 0x9024-0x91D0, 0x8500 */
> > + s->g3dctl = I915_READ(GEN7_G3DCTL);
> > + s->gsckgctl = I915_READ(GEN7_GSCKGCTL);
> > + s->mbctl= I915_READ(GEN6_MBCTL);
> > +
> > + /* GCP 0x9400-0x9424, 0x8100-0x810C */
> > + s->ucgctl1 = I915_READ(GEN6_UCGCTL1);
> > + s->ucgctl3 = I915_READ(GEN7_UCGCTL3);
> > + s->rcgctl1 = I915_READ(GEN7_RCGCTL1);
> > + s->rcgctl2
Re: [Intel-gfx] The whole round of i-g-t testing cost too long running time
On Wed, Apr 16, 2014 at 08:42:27AM -0700, Jesse Barnes wrote: > And can you elaborate on the CRC tests? It doesn't seem like those > should take more than a few frames to verify we're getting what we > expect... Indeed, if the CRC tests take a long time, that's a bug (for instance we may never receive the CRC interrupt and at the moment this means the test will be blocked into a read()). I'll fix this particular one (needs to implement poll() support for the CRC result file in debugfs) when I'm finished with more pressing tasks, if noone beats me to it, of course. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Remove vblank wait from haswell_write_eld
On Wed, Apr 16, 2014 at 04:56:09PM +0200, Daniel Vetter wrote:
> The pipe is off at that point in time, so a vblank wait is simply a
> 50ms wait. Caught by Jesse's verbose "make vblank wait timeouts WARN"
> patch. We've probably had a few versions of this float around already.
>
> To document assumptions put a pipe assert into the same place. And
> also add a posting read.
>
> If we ever decide to update the eld and infoframes while the pipe is
> already on (e.g. for fastboot) then there's lots of work to do. So
> better properly document all the hidden assumptions.
IIRC the docs might say that eld (or some audio stuff) can't be enabled
until one or two vblanks after the pipe has been enabled (or something
to that effect). So we're probably doing it all wrong and we should
move the audio stuff to some vblank work thingy eventually.
But as this is currently done while the pipe is off, the vblank wait
is just a nop, and so we should just kill it.
Reviewed-by: Ville Syrjälä
>
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/intel_display.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 8b623a623f9c..82ad84eefc8d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7371,7 +7371,6 @@ static void haswell_write_eld(struct drm_connector
> *connector,
> {
> struct drm_i915_private *dev_priv = connector->dev->dev_private;
> uint8_t *eld = connector->eld;
> - struct drm_device *dev = crtc->dev;
> uint32_t eldv;
> uint32_t i;
> int len;
> @@ -7388,9 +7387,9 @@ static void haswell_write_eld(struct drm_connector
> *connector,
> tmp = I915_READ(aud_cntrl_st2);
> tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> I915_WRITE(aud_cntrl_st2, tmp);
> + POSTING_READ(aud_cntrl_st2);
>
> - /* Wait for 1 vertical blank */
> - intel_wait_for_vblank(dev, pipe);
> + assert_pipe_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
>
> /* Set ELD valid state */
> tmp = I915_READ(aud_cntrl_st2);
> --
> 1.8.4.rc3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] The whole round of i-g-t testing cost too long running time
On 16/04/2014 17:42, Jesse Barnes wrote: On Tue, 15 Apr 2014 19:17:59 +0200 Daniel Vetter wrote: Ok there are a few cases where we can indeed make tests faster, but it will be work for us. And that won't really speed up much since we're adding piles more testcases at a pretty quick rate. And many of these new testcases are CRC based, so inheritely take some time to run. But each test should run very quickly in general; I think we have too many tests that take much longer than they need to. Adding some hooks to the driver via debugfs may let us trigger specific cases directly rather than trying to induce them through massive threading and memory pressure for example. And can you elaborate on the CRC tests? It doesn't seem like those should take more than a few frames to verify we're getting what we expect... Well they don't take more than a few frames, but we have a _lot_ of them, and there's a lot of cominations to test. It adds up quickly. Iirc we have over 150 kms_flip testcases alone ... Like I've said I agree that we could speed tests up, but besides me doing the occasional tuning and improvement in that regard I have seen 0 patches from developers in this area. Which lets me conclude that apparently it's not reallly that bad an isssue ;-) If people _really_ care about this I have a list of things to knock down. But first someone needs to find some time and resources for this. So I think longer-term we simply need to throw more machines at the problem and run testcases in parallel on identical machines. Wrt analyzing issues I think the right approach for moving forward is: a) switch to piglit to run tests, not just enumerate them. This will allow QA and developers to share testcase analysis. b) add automated analysis for time-consuming and error prone cases like dmesg warnings and backtraces. Thomas&I have just discussed a few ideas in this are in our 1:1 today. Reducing the set of igt tests we run is imo pointless: The goal of igt is to hit corner-cases, arbitrarily selecting which kinds of corner-cases we test just means that we have a nice illusion about our test coverage. My goal is still to get full test coverage before patches get committed, and that means having quick (<1hr) turnaround for testing from the automated patch test system. It seems like we'll need to approach that from all angles: speeding up tests, parallelizing execution, adding hooks to the driver, etc. Currently <1h and "full test coverage" are rather mutually exclusive unfortunately :( I agree that having it would be extremely useful for developers, but if this happens with any cost/service reduction for nightly testing on my branches I'm really opposed. Atm we already have a _really_ hard time keeping track of all the various regressions and bugs. -Daniel Intel Semiconductor AG Registered No. 020.30.913.786-7 Registered Office: Badenerstrasse 549, 8048 Zurich, Switzerland ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/crtc-helper: gc usless connectro loop in disable_unused_functions
On Sun, Apr 13, 2014 at 4:39 PM, Daniel Vetter wrote: > I've forgotten to clean this all up correctly in > > commit e3d6ddb35f6221859b6054879d186e13a3af351e > Author: Daniel Vetter > Date: Tue Apr 1 22:15:00 2014 +0200 > > drm/crtc-helper: don't disable disconnected outputs > > Reported-by: Russell King - ARM Linux > Cc: Russell King - ARM Linux > Signed-off-by: Daniel Vetter Typo in the Subject "useless connector" ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] The whole round of i-g-t testing cost too long running time
On Tue, 15 Apr 2014 19:17:59 +0200 Daniel Vetter wrote: > Ok there are a few cases where we can indeed make tests faster, but it > will be work for us. And that won't really speed up much since we're > adding piles more testcases at a pretty quick rate. And many of these > new testcases are CRC based, so inheritely take some time to run. But each test should run very quickly in general; I think we have too many tests that take much longer than they need to. Adding some hooks to the driver via debugfs may let us trigger specific cases directly rather than trying to induce them through massive threading and memory pressure for example. And can you elaborate on the CRC tests? It doesn't seem like those should take more than a few frames to verify we're getting what we expect... > So I think longer-term we simply need to throw more machines at the > problem and run testcases in parallel on identical machines. > > Wrt analyzing issues I think the right approach for moving forward is: > a) switch to piglit to run tests, not just enumerate them. This will > allow QA and developers to share testcase analysis. > b) add automated analysis for time-consuming and error prone cases like > dmesg warnings and backtraces. Thomas&I have just discussed a few ideas > in this are in our 1:1 today. > > Reducing the set of igt tests we run is imo pointless: The goal of igt > is to hit corner-cases, arbitrarily selecting which kinds of > corner-cases we test just means that we have a nice illusion about our > test coverage. My goal is still to get full test coverage before patches get committed, and that means having quick (<1hr) turnaround for testing from the automated patch test system. It seems like we'll need to approach that from all angles: speeding up tests, parallelizing execution, adding hooks to the driver, etc. Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Remove vblank wait from haswell_write_eld
The pipe is off at that point in time, so a vblank wait is simply a
50ms wait. Caught by Jesse's verbose "make vblank wait timeouts WARN"
patch. We've probably had a few versions of this float around already.
To document assumptions put a pipe assert into the same place. And
also add a posting read.
If we ever decide to update the eld and infoframes while the pipe is
already on (e.g. for fastboot) then there's lots of work to do. So
better properly document all the hidden assumptions.
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/i915/intel_display.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 8b623a623f9c..82ad84eefc8d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7371,7 +7371,6 @@ static void haswell_write_eld(struct drm_connector
*connector,
{
struct drm_i915_private *dev_priv = connector->dev->dev_private;
uint8_t *eld = connector->eld;
- struct drm_device *dev = crtc->dev;
uint32_t eldv;
uint32_t i;
int len;
@@ -7388,9 +7387,9 @@ static void haswell_write_eld(struct drm_connector
*connector,
tmp = I915_READ(aud_cntrl_st2);
tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
I915_WRITE(aud_cntrl_st2, tmp);
+ POSTING_READ(aud_cntrl_st2);
- /* Wait for 1 vertical blank */
- intel_wait_for_vblank(dev, pipe);
+ assert_pipe_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
/* Set ELD valid state */
tmp = I915_READ(aud_cntrl_st2);
--
1.8.4.rc3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 25/25] drm/i915: vlv: add runtime PM support
On Mon, Apr 14, 2014 at 08:24:46PM +0300, Imre Deak wrote:
> Add runtime PM support for VLV, but leave it disabled. The next patch
> enables it.
>
> The suspend/resume sequence used is based on [1] and [2]. In practice we
> depend on the GT RC6 mechanism to save the HW context depending on the
> render and media power wells. By the time we run the runtime suspend
> callback the display side is also off and the HW context for that is
> managed by the display power domain framework.
>
> Besides the above there are Gunit registers that depend on a system-wide
> power well. This power well goes off once the device enters any of the
> S0i[R123] states. To handle this scenario, save/restore these Gunit
> registers. Note that this is not the complete register set dictated by
> [2], to remove some overhead, registers that are known not to be used are
> ignored. Also some registers are fully setup by initialization functions
> called during resume, these are not saved either. The list of registers
> can be further reduced, see the TODO note in the code.
>
> [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> [2] VLV2_S0IXRegs
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.c | 327
>
> drivers/gpu/drm/i915/i915_drv.h | 62
> 2 files changed, 389 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 08e210c..bc206dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -911,6 +911,198 @@ static int hsw_runtime_resume(struct drm_i915_private
> *dev_priv)
> return 0;
> }
>
> +/*
> + * Save all Gunit registers that may be lost after a D3 and a subsequent
> + * S0i[R123] transition. The list of registers needing a save/restore is
> + * defined in the VLV2_S0IXRegs document. This documents marks all Gunit
> + * registers in the following way:
> + * - Driver: saved/restored by the driver
> + * - Punit : saved/restored by the Punit firmware
> + * - No, w/o marking: no need to save/restore, since the register is R/O or
> + *used internally by the HW in a way that doesn't depend
> + *keeping the content across a suspend/resume.
> + * - Debug : used for debugging
> + *
> + * We save/restore all registers marked with 'Driver', with the following
> + * exceptions:
> + * - Registers out of use, including also registers marked with 'Debug'.
> + * These have no effect on the driver's operation, so we don't save/restore
> + * them to reduce the overhead.
> + * - Registers that are fully setup by an initialization function called from
> + * the resume path. For example many clock gating and RPS/RC6 registers.
> + * - Registers that provide the right functionality with their reset
> defaults.
> + *
> + * TODO: Except for registers that based on the above 3 criteria can be
> safely
> + * ignored, we save/restore all others, practically treating the HW context
> as
> + * a black-box for the driver. Further investigation is needed to reduce the
> + * saved/restored registers even further, by following the same 3 criteria.
> + */
> +static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> +{
> + struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
> + int i;
> +
> + /* GAM 0x4000-0x4770 */
> + s->wr_watermark = I915_READ(GEN7_WR_WATERMARK);
> + s->gfx_prio_ctrl= I915_READ(GEN7_GFX_PRIO_CTRL);
> + s->arb_mode = I915_READ(ARB_MODE);
> + s->gfx_pend_tlb0= I915_READ(GEN7_GFX_PEND_TLB0);
> + s->gfx_pend_tlb1= I915_READ(GEN7_GFX_PEND_TLB1);
> +
> + for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++)
> + s->lra_limits[i] = I915_READ(GEN7_LRA_LIMITS_BASE + i * 4);
> +
> + s->media_max_req_count = I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
> + s->gfx_max_req_count= I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
> +
> + s->render_hwsp = I915_READ(RENDER_HWS_PGA_GEN7);
> + s->ecochk = I915_READ(GAM_ECOCHK);
> + s->bsd_hwsp = I915_READ(BSD_HWS_PGA_GEN7);
> + s->blt_hwsp = I915_READ(BLT_HWS_PGA_GEN7);
> +
> + s->tlb_rd_addr = I915_READ(GEN7_TLB_RD_ADDR);
> +
> + /* MBC 0x9024-0x91D0, 0x8500 */
> + s->g3dctl = I915_READ(GEN7_G3DCTL);
> + s->gsckgctl = I915_READ(GEN7_GSCKGCTL);
> + s->mbctl= I915_READ(GEN6_MBCTL);
> +
> + /* GCP 0x9400-0x9424, 0x8100-0x810C */
> + s->ucgctl1 = I915_READ(GEN6_UCGCTL1);
> + s->ucgctl3 = I915_READ(GEN7_UCGCTL3);
> + s->rcgctl1 = I915_READ(GEN7_RCGCTL1);
> + s->rcgctl2 = I915_READ(GEN7_RCGCTL2);
> + s->rstctl = I915_READ(GEN7_RSTCTL);
> + s->misccpctl= I915_READ(GEN7_MISCCPCTL);
> +
> + /* GPM 0xA000-0xAA84, 0x8000-0x80FC */
> + s->gfxpause
[Intel-gfx] [PATCH] drm/fb-helper: Fix hpd vs. initial config races
Some drivers need to be able to have a perfect race-free fbcon setup.
Current drivers only enable hotplug processing after the call to
drm_fb_helper_initial_config which leaves a tiny but important race.
This race is especially noticable on embedded platforms where the
driver itself enables the voltage for the hdmi output, since only then
will monitors (after a bit of delay, as usual) respond by asserting
the hpd pin.
Most of the infrastructure is already there with the split-out
drm_fb_helper_init. And drm_fb_helper_initial_config already has all
the required locking to handle concurrent hpd events since
commit 53f1904bced78d7c00f5d874c662ec3ac85d0f9f
Author: Daniel Vetter
Date: Thu Mar 20 14:26:35 2014 +0100
drm/fb-helper: improve drm_fb_helper_initial_config locking
The only missing bit is making drm_fb_helper_hotplug_event save
against concurrent calls of drm_fb_helper_initial_config. The only
unprotected bit is the check for fb_helper->fb.
With that drivers can first initialize the fb helper, then enabel
hotplug processing and then set up the initial config all in a
completely race-free manner. Update kerneldoc and convert i915 as a
proof of concept.
Feature requested by Thierry since his tegra driver atm reliably boots
slowly enough to misses the hotplug event for an external hdmi screen,
but also reliably boots to quickly for the hpd pin to be asserted when
the fb helper calls into the hdmi ->detect function.
Cc: Thierry Reding
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_fb_helper.c | 11 +--
drivers/gpu/drm/i915/i915_dma.c | 3 ---
drivers/gpu/drm/i915/i915_drv.c | 2 --
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/i915_irq.c | 4
5 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 04d3fd3658f3..9b775ddb8697 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1578,8 +1578,10 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
* either the output polling work or a work item launched from the driver's
* hotplug interrupt).
*
- * Note that the driver must ensure that this is only called _after_ the fb has
- * been fully set up, i.e. after the call to drm_fb_helper_initial_config.
+ * Note that drivers may call this even before calling
+ * drm_fb_helper_initial_config but only aftert drm_fb_helper_init. This allows
+ * for a race-free fbcon setup and will make sure that the fbdev emulation will
+ * not miss any hotplug events.
*
* RETURNS:
* 0 on success and a non-zero error code otherwise.
@@ -1589,11 +1591,8 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper
*fb_helper)
struct drm_device *dev = fb_helper->dev;
u32 max_width, max_height;
- if (!fb_helper->fb)
- return 0;
-
mutex_lock(&fb_helper->dev->mode_config.mutex);
- if (!drm_fb_helper_is_bound(fb_helper)) {
+ if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
fb_helper->delayed_hotplug = true;
mutex_unlock(&fb_helper->dev->mode_config.mutex);
return 0;
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 96177eec0a0e..8254056f22d8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1369,9 +1369,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
*/
intel_fbdev_initial_config(dev);
- /* Only enable hotplug handling once the fbdev is fully set up. */
- dev_priv->enable_hotplug_processing = true;
-
drm_kms_helper_poll_init(dev);
return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 82f4d1f47d3b..aba9552e95e2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -448,7 +448,6 @@ static int i915_drm_freeze(struct drm_device *dev)
cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
drm_irq_uninstall(dev);
- dev_priv->enable_hotplug_processing = false;
/*
* Disable CRTCs directly since we want to preserve sw state
* for _thaw.
@@ -589,7 +588,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
restore_gtt_mappings)
* notifications.
* */
intel_hpd_init(dev);
- dev_priv->enable_hotplug_processing = true;
/* Config may have changed between suspend and resume */
intel_resume_hotplug(dev);
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0905cd915589..646a1f9a0db9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1469,7 +1469,6 @@ typedef struct drm_i915_private {
u32 pipestat_irq_mask[I915_MAX_PIPES];
struct work_struct hotplug_work;
- bool enab
Re: [Intel-gfx] [PATCH V3 1/6] drm/i915: Split the BDW device definition to prepare for dual BSD rings on BDW GT3
On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote:
> Based on the hardware spec, the BDW GT3 has the different configuration
> with the BDW GT1/GT2. So split the BDW device info definition.
> This is to do the preparation for adding the Dual BSD rings on BDW GT3
> machine.
>
> V1->V2: Follow Daniel's comment to pay attention to the stolen check for BDW
> in kernel/early-quirks.c
>
> Signed-off-by: Zhao Yakui
Reviewed-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.c | 26 --
> include/drm/i915_pciids.h | 22 +-
> 2 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5d8250f..17fbbe5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -279,6 +279,26 @@ static const struct intel_device_info
> intel_broadwell_m_info = {
> GEN_DEFAULT_PIPEOFFSETS,
> };
>
> +static const struct intel_device_info intel_broadwell_gt3d_info = {
> + .gen = 8, .num_pipes = 3,
> + .need_gfx_hws = 1, .has_hotplug = 1,
> + .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> + .has_llc = 1,
> + .has_ddi = 1,
> + .has_fbc = 1,
> + GEN_DEFAULT_PIPEOFFSETS,
> +};
> +
> +static const struct intel_device_info intel_broadwell_gt3m_info = {
> + .gen = 8, .is_mobile = 1, .num_pipes = 3,
> + .need_gfx_hws = 1, .has_hotplug = 1,
> + .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> + .has_llc = 1,
> + .has_ddi = 1,
> + .has_fbc = 1,
> + GEN_DEFAULT_PIPEOFFSETS,
> +};
> +
> /*
> * Make sure any device matches here are from most specific to most
> * general. For example, since the Quanta match is based on the subsystem
> @@ -311,8 +331,10 @@ static const struct intel_device_info
> intel_broadwell_m_info = {
> INTEL_HSW_M_IDS(&intel_haswell_m_info), \
> INTEL_VLV_M_IDS(&intel_valleyview_m_info), \
> INTEL_VLV_D_IDS(&intel_valleyview_d_info), \
> - INTEL_BDW_M_IDS(&intel_broadwell_m_info), \
> - INTEL_BDW_D_IDS(&intel_broadwell_d_info)
> + INTEL_BDW_GT12M_IDS(&intel_broadwell_m_info), \
> + INTEL_BDW_GT12D_IDS(&intel_broadwell_d_info), \
> + INTEL_BDW_GT3M_IDS(&intel_broadwell_gt3m_info), \
> + INTEL_BDW_GT3D_IDS(&intel_broadwell_gt3d_info)
>
> static const struct pci_device_id pciidlist[] = {/* aka */
> INTEL_PCI_IDS,
> diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> index 940ece4..24f3cad 100644
> --- a/include/drm/i915_pciids.h
> +++ b/include/drm/i915_pciids.h
> @@ -223,14 +223,26 @@
> _INTEL_BDW_D(gt, 0x160A, info), /* Server */ \
> _INTEL_BDW_D(gt, 0x160D, info) /* Workstation */
>
> -#define INTEL_BDW_M_IDS(info) \
> +#define INTEL_BDW_GT12M_IDS(info) \
> _INTEL_BDW_M_IDS(1, info), \
> - _INTEL_BDW_M_IDS(2, info), \
> - _INTEL_BDW_M_IDS(3, info)
> + _INTEL_BDW_M_IDS(2, info)
>
> -#define INTEL_BDW_D_IDS(info) \
> +#define INTEL_BDW_GT12D_IDS(info) \
> _INTEL_BDW_D_IDS(1, info), \
> - _INTEL_BDW_D_IDS(2, info), \
> + _INTEL_BDW_D_IDS(2, info)
> +
> +#define INTEL_BDW_GT3M_IDS(info) \
> + _INTEL_BDW_M_IDS(3, info)
> +
> +#define INTEL_BDW_GT3D_IDS(info) \
> _INTEL_BDW_D_IDS(3, info)
>
> +#define INTEL_BDW_M_IDS(info) \
> + INTEL_BDW_GT12M_IDS(info), \
> + INTEL_BDW_GT3M_IDS(info)
> +
> +#define INTEL_BDW_D_IDS(info) \
> + INTEL_BDW_GT12D_IDS(info), \
> + INTEL_BDW_GT3D_IDS(info)
> +
> #endif /* _I915_PCIIDS_H */
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Enable PM Interrupts target via Display Interface.
On Mon, Apr 14, 2014 at 10:36:55PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 14, 2014 at 10:41:15PM +0530, deepa...@intel.com wrote:
> > From: Deepak S
> >
> > In BDW, Apart from unmasking up/down threshold interrupts. we need
> > to umask bit 32 of PM_INTRMASK to route interrupts to target via Display
> > Interface.
> >
> > Signed-off-by: Deepak S
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/intel_pm.c | 2 ++
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index c2dd436..8c7841b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5105,6 +5105,7 @@ enum punit_power_well {
> > #define GEN6_RC6p_THRESHOLD0xA0BC
> > #define GEN6_RC6pp_THRESHOLD 0xA0C0
> > #define GEN6_PMINTRMSK 0xA168
> > +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP 0x7FFF
>
> Defining is as (1<<31) would make more sense to me.
>
> >
> > #define GEN6_PMISR 0x44020
> > #define GEN6_PMIMR 0x44024 /* rps_lock */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 27b64ab..6b123cd 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3066,6 +3066,8 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private
> > *dev_priv, u8 val)
> > if (INTEL_INFO(dev_priv->dev)->gen <= 7 && !IS_HASWELL(dev_priv->dev))
> > mask |= GEN6_PM_RP_UP_EI_EXPIRED;
> >
> > + mask &= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
> > +
> > return ~mask;
Oh and just noticed this doesn't actually do anything.
& must come after ~ to get the expected result.
> > }
> >
> > --
> > 1.8.5.2
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Drop the excessive vblank waits from modeset codepaths
On Wed, Mar 12, 2014 at 09:25:42AM +, Chris Wilson wrote: > On Wed, Mar 12, 2014 at 11:16:59AM +0200, Ville Syrjälä wrote: > > On Wed, Mar 12, 2014 at 08:35:39AM +, Chris Wilson wrote: > > > On Tue, Mar 11, 2014 at 07:37:35PM +0200, ville.syrj...@linux.intel.com > > > wrote: > > > > From: Ville Syrjälä > > > > > > > > Now that we've plugged the mmio vs. ring flip race, we shouldn't need > > > > these vblank waits in the modeset codepaths anymore. So get rid of > > > > them. > > > > > > Hmm, could we not add an assert(DSPSURFLIVE == > > > intel_crtc->dspsurf)? > > > > Where would you want the assert? > > assert_plane_is_bound(new_fb) in crtc_enable() and > assert_plane_is_bound(old_fb) in crt_disable(). i.e. add them to our set > of plane/pipe checks through modeset. We can't really add live base address checks to such places w/o adding extra vblank waits. For example if we first do a set_base w/o waiting for vblank and then disable the crtc. By the time we get to crtc_disable() there's no guarantee that the mmio flip from set_base has completed, and hence the live surface address may still have the old value. For the atomic page flip stuff the live address can serve as a decent debug tool to make sure the flip has really completed or not completed when we expect. IIRC I had such a debug patch in my atomic branch. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 10/25] drm/i915: gen2: move error capture of IER to its correct place
On Wed, 2014-04-16 at 15:22 +0300, Ville Syrjälä wrote:
> On Mon, Apr 14, 2014 at 08:24:31PM +0300, Imre Deak wrote:
> > While checking the error capture path I noticed that this register is
> > read twice for GEN2, so fix this and also move the read where it's done
> > for other platforms.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/i915_gpu_error.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 4865ade..ba79b59 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1053,9 +1053,6 @@ static void i915_capture_reg_state(struct
> > drm_i915_private *dev_priv,
> > error->gfx_mode = I915_READ(GFX_MODE);
> > }
> >
> > - if (IS_GEN2(dev))
> > - error->ier = I915_READ16(IER);
> > -
> > /* 2: Registers which belong to multiple generations */
> > if (INTEL_INFO(dev)->gen >= 7)
> > error->forcewake = I915_READ(FORCEWAKE_MT);
> > @@ -1079,7 +1076,10 @@ static void i915_capture_reg_state(struct
> > drm_i915_private *dev_priv,
> > if (HAS_PCH_SPLIT(dev))
> > error->ier = I915_READ(DEIER) | I915_READ(GTIER);
> > else {
> > - error->ier = I915_READ(IER);
> > + if (IS_GEN2(dev))
> > + error->ier = I915_READ16(IER);
> > + else
> > + error->ier = I915_READ(IER);
> > for_each_pipe(pipe)
> > error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
> > }
>
> The IER handling seems fairly bogus all around. On VLV and PCH platforms
> we smash both the display and GT IER into the same u32. So probably no
> one can make any sense of the result.
>
> Also I don't know why we try to dump only these two interrupt registers
> but not the others.
>
> So seems like there's more work that needs to be done here.
Right, haven't noticed those.
This patch could be still applied as it's just one step towards fixing
the other issues you mentioned, but I think it should be anyway kept
separate (being just a cleanup patch).
--Imre
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 25/25] drm/i915: vlv: add runtime PM support
On Mon, Apr 14, 2014 at 08:24:46PM +0300, Imre Deak wrote:
> Add runtime PM support for VLV, but leave it disabled. The next patch
> enables it.
>
> The suspend/resume sequence used is based on [1] and [2]. In practice we
> depend on the GT RC6 mechanism to save the HW context depending on the
> render and media power wells. By the time we run the runtime suspend
> callback the display side is also off and the HW context for that is
> managed by the display power domain framework.
>
> Besides the above there are Gunit registers that depend on a system-wide
> power well. This power well goes off once the device enters any of the
> S0i[R123] states. To handle this scenario, save/restore these Gunit
> registers. Note that this is not the complete register set dictated by
> [2], to remove some overhead, registers that are known not to be used are
> ignored. Also some registers are fully setup by initialization functions
> called during resume, these are not saved either. The list of registers
> can be further reduced, see the TODO note in the code.
>
> [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> [2] VLV2_S0IXRegs
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.c | 327
>
> drivers/gpu/drm/i915/i915_drv.h | 62
> 2 files changed, 389 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 08e210c..bc206dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -911,6 +911,198 @@ static int hsw_runtime_resume(struct drm_i915_private
> *dev_priv)
> return 0;
> }
>
> +/*
> + * Save all Gunit registers that may be lost after a D3 and a subsequent
> + * S0i[R123] transition. The list of registers needing a save/restore is
> + * defined in the VLV2_S0IXRegs document. This documents marks all Gunit
> + * registers in the following way:
> + * - Driver: saved/restored by the driver
> + * - Punit : saved/restored by the Punit firmware
> + * - No, w/o marking: no need to save/restore, since the register is R/O or
> + *used internally by the HW in a way that doesn't depend
> + *keeping the content across a suspend/resume.
> + * - Debug : used for debugging
> + *
> + * We save/restore all registers marked with 'Driver', with the following
> + * exceptions:
> + * - Registers out of use, including also registers marked with 'Debug'.
> + * These have no effect on the driver's operation, so we don't save/restore
> + * them to reduce the overhead.
> + * - Registers that are fully setup by an initialization function called from
> + * the resume path. For example many clock gating and RPS/RC6 registers.
> + * - Registers that provide the right functionality with their reset
> defaults.
> + *
> + * TODO: Except for registers that based on the above 3 criteria can be
> safely
> + * ignored, we save/restore all others, practically treating the HW context
> as
> + * a black-box for the driver. Further investigation is needed to reduce the
> + * saved/restored registers even further, by following the same 3 criteria.
> + */
> +static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> +{
> + struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
> + int i;
> +
> + /* GAM 0x4000-0x4770 */
> + s->wr_watermark = I915_READ(GEN7_WR_WATERMARK);
> + s->gfx_prio_ctrl= I915_READ(GEN7_GFX_PRIO_CTRL);
> + s->arb_mode = I915_READ(ARB_MODE);
> + s->gfx_pend_tlb0= I915_READ(GEN7_GFX_PEND_TLB0);
> + s->gfx_pend_tlb1= I915_READ(GEN7_GFX_PEND_TLB1);
> +
> + for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++)
> + s->lra_limits[i] = I915_READ(GEN7_LRA_LIMITS_BASE + i * 4);
> +
> + s->media_max_req_count = I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
> + s->gfx_max_req_count= I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
> +
> + s->render_hwsp = I915_READ(RENDER_HWS_PGA_GEN7);
> + s->ecochk = I915_READ(GAM_ECOCHK);
> + s->bsd_hwsp = I915_READ(BSD_HWS_PGA_GEN7);
> + s->blt_hwsp = I915_READ(BLT_HWS_PGA_GEN7);
> +
> + s->tlb_rd_addr = I915_READ(GEN7_TLB_RD_ADDR);
> +
> + /* MBC 0x9024-0x91D0, 0x8500 */
> + s->g3dctl = I915_READ(GEN7_G3DCTL);
> + s->gsckgctl = I915_READ(GEN7_GSCKGCTL);
> + s->mbctl= I915_READ(GEN6_MBCTL);
> +
> + /* GCP 0x9400-0x9424, 0x8100-0x810C */
> + s->ucgctl1 = I915_READ(GEN6_UCGCTL1);
> + s->ucgctl3 = I915_READ(GEN7_UCGCTL3);
> + s->rcgctl1 = I915_READ(GEN7_RCGCTL1);
> + s->rcgctl2 = I915_READ(GEN7_RCGCTL2);
> + s->rstctl = I915_READ(GEN7_RSTCTL);
> + s->misccpctl= I915_READ(GEN7_MISCCPCTL);
> +
> + /* GPM 0xA000-0xAA84, 0x8000-0x80FC */
> + s->gfxpause
Re: [Intel-gfx] [PATCH v2 14/25] drm/i915: sanitize enable_rc6 option
On Wed, 2014-04-16 at 15:28 +0300, Ville Syrjälä wrote:
> On Mon, Apr 14, 2014 at 08:24:35PM +0300, Imre Deak wrote:
> > Atm, an invalid enable_rc6 module option will be silently ignored, so
> > emit an info message about it. Doing an early sanitization we can also
> > reuse intel_enable_rc6() in a follow-up patch to see if RC6 is actually
> > enabled. Currently the caller would have to filter a non-zero return
> > value based on the platform we are running on. For example on VLV with
> > i915.enable_rc6 set to 2, RC6 won't be enabled but atm
> > intel_enable_rc6() would still return 2 in this case.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 27 ---
> > 1 file changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index a56f6b1..89fe0a7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3262,15 +3262,29 @@ static void intel_print_rc6_info(struct drm_device
> > *dev, u32 mode)
> > (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
> > }
> >
> > -int intel_enable_rc6(const struct drm_device *dev)
> > +static int sanitize_rc6_option(const struct drm_device *dev, int
> > enable_rc6)
> > {
> > /* No RC6 before Ironlake */
> > if (INTEL_INFO(dev)->gen < 5)
> > return 0;
> >
> > /* Respect the kernel parameter if it is set */
> > - if (i915.enable_rc6 >= 0)
> > - return i915.enable_rc6;
> > + if (enable_rc6 >= 0) {
> > + int mask = 0;
> > +
> > + if (IS_BROADWELL(dev) || IS_HASWELL(dev) ||
> > + IS_VALLEYVIEW(dev) || IS_IRONLAKE_M(dev))
> > + mask = INTEL_RC6_ENABLE;
> > + else if (INTEL_INFO(dev)->gen >= 6)
> > + mask = INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE |
> > + INTEL_RC6pp_ENABLE;
>
> You forgot ILK.
For desktop ILK intel_enable_gt_powersave() will keep RC6 disabled, so
the above is correct based on that.
> Also this would seem simpler:
>
> if (SNB|IVB)
> mask = rc6 | rc6p | rc6pp;
> else
> mask = rc6;
Ok, can rewrite it with the above exception for desktop ILK.
--Imre
>
> > +
> > + if ((enable_rc6 & mask) != enable_rc6)
> > + DRM_INFO("Adjusting RC6 mask to %d (requested %d, valid
> > %d)\n",
> > +enable_rc6, enable_rc6 & mask, mask);
> > +
> > + return enable_rc6 & mask;
> > + }
> >
> > /* Disable RC6 on Ironlake */
> > if (INTEL_INFO(dev)->gen == 5)
> > @@ -3282,6 +3296,11 @@ int intel_enable_rc6(const struct drm_device *dev)
> > return INTEL_RC6_ENABLE;
> > }
> >
> > +int intel_enable_rc6(const struct drm_device *dev)
> > +{
> > + return i915.enable_rc6;
> > +}
> > +
> > static void gen6_enable_rps_interrupts(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -4496,6 +4515,8 @@ static void intel_init_emon(struct drm_device *dev)
> >
> > void intel_init_gt_powersave(struct drm_device *dev)
> > {
> > + i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
> > +
> > if (IS_VALLEYVIEW(dev))
> > valleyview_setup_pctx(dev);
> > }
> > --
> > 1.8.4
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/25] drm/i915: sanitize enable_rc6 option
On Mon, Apr 14, 2014 at 08:24:35PM +0300, Imre Deak wrote:
> Atm, an invalid enable_rc6 module option will be silently ignored, so
> emit an info message about it. Doing an early sanitization we can also
> reuse intel_enable_rc6() in a follow-up patch to see if RC6 is actually
> enabled. Currently the caller would have to filter a non-zero return
> value based on the platform we are running on. For example on VLV with
> i915.enable_rc6 set to 2, RC6 won't be enabled but atm
> intel_enable_rc6() would still return 2 in this case.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/intel_pm.c | 27 ---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a56f6b1..89fe0a7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3262,15 +3262,29 @@ static void intel_print_rc6_info(struct drm_device
> *dev, u32 mode)
>(mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
> }
>
> -int intel_enable_rc6(const struct drm_device *dev)
> +static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
> {
> /* No RC6 before Ironlake */
> if (INTEL_INFO(dev)->gen < 5)
> return 0;
>
> /* Respect the kernel parameter if it is set */
> - if (i915.enable_rc6 >= 0)
> - return i915.enable_rc6;
> + if (enable_rc6 >= 0) {
> + int mask = 0;
> +
> + if (IS_BROADWELL(dev) || IS_HASWELL(dev) ||
> + IS_VALLEYVIEW(dev) || IS_IRONLAKE_M(dev))
> + mask = INTEL_RC6_ENABLE;
> + else if (INTEL_INFO(dev)->gen >= 6)
> + mask = INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE |
> +INTEL_RC6pp_ENABLE;
You forgot ILK.
Also this would seem simpler:
if (SNB|IVB)
mask = rc6 | rc6p | rc6pp;
else
mask = rc6;
> +
> + if ((enable_rc6 & mask) != enable_rc6)
> + DRM_INFO("Adjusting RC6 mask to %d (requested %d, valid
> %d)\n",
> + enable_rc6, enable_rc6 & mask, mask);
> +
> + return enable_rc6 & mask;
> + }
>
> /* Disable RC6 on Ironlake */
> if (INTEL_INFO(dev)->gen == 5)
> @@ -3282,6 +3296,11 @@ int intel_enable_rc6(const struct drm_device *dev)
> return INTEL_RC6_ENABLE;
> }
>
> +int intel_enable_rc6(const struct drm_device *dev)
> +{
> + return i915.enable_rc6;
> +}
> +
> static void gen6_enable_rps_interrupts(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4496,6 +4515,8 @@ static void intel_init_emon(struct drm_device *dev)
>
> void intel_init_gt_powersave(struct drm_device *dev)
> {
> + i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
> +
> if (IS_VALLEYVIEW(dev))
> valleyview_setup_pctx(dev);
> }
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 10/25] drm/i915: gen2: move error capture of IER to its correct place
On Mon, Apr 14, 2014 at 08:24:31PM +0300, Imre Deak wrote:
> While checking the error capture path I noticed that this register is
> read twice for GEN2, so fix this and also move the read where it's done
> for other platforms.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_gpu_error.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 4865ade..ba79b59 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1053,9 +1053,6 @@ static void i915_capture_reg_state(struct
> drm_i915_private *dev_priv,
> error->gfx_mode = I915_READ(GFX_MODE);
> }
>
> - if (IS_GEN2(dev))
> - error->ier = I915_READ16(IER);
> -
> /* 2: Registers which belong to multiple generations */
> if (INTEL_INFO(dev)->gen >= 7)
> error->forcewake = I915_READ(FORCEWAKE_MT);
> @@ -1079,7 +1076,10 @@ static void i915_capture_reg_state(struct
> drm_i915_private *dev_priv,
> if (HAS_PCH_SPLIT(dev))
> error->ier = I915_READ(DEIER) | I915_READ(GTIER);
> else {
> - error->ier = I915_READ(IER);
> + if (IS_GEN2(dev))
> + error->ier = I915_READ16(IER);
> + else
> + error->ier = I915_READ(IER);
> for_each_pipe(pipe)
> error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
> }
The IER handling seems fairly bogus all around. On VLV and PCH platforms
we smash both the display and GT IER into the same u32. So probably no
one can make any sense of the result.
Also I don't know why we try to dump only these two interrupt registers
but not the others.
So seems like there's more work that needs to be done here.
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 11/25] drm/i915: add missing error capturing of the PIPESTAT reg
On Mon, Apr 14, 2014 at 08:24:32PM +0300, Imre Deak wrote:
> While checking the error capture path I noticed that we lacked the
> power domain-on check for PIPESTAT so fix this by moving that to where
> the rest of pipe registers are captured.
>
> The move also revealed that we actually don't include this register in
> the error report, so fix that too.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 -
> drivers/gpu/drm/i915/i915_gpu_error.c | 3 ---
> drivers/gpu/drm/i915/intel_display.c | 3 +++
> 3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7d6acb4..5254f4b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -325,7 +325,6 @@ struct drm_i915_error_state {
> u32 gab_ctl;
> u32 gfx_mode;
> u32 extra_instdone[I915_NUM_INSTDONE_REG];
> - u32 pipestat[I915_MAX_PIPES];
> u64 fence[I915_MAX_NUM_FENCES];
> struct intel_overlay_error_state *overlay;
> struct intel_display_error_state *display;
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index ba79b59..7b5cc08 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1028,7 +1028,6 @@ static void i915_capture_reg_state(struct
> drm_i915_private *dev_priv,
> struct drm_i915_error_state *error)
> {
> struct drm_device *dev = dev_priv->dev;
> - int pipe;
>
> /* General organization
>* 1. Registers specific to a single generation
> @@ -1080,8 +1079,6 @@ static void i915_capture_reg_state(struct
> drm_i915_private *dev_priv,
> error->ier = I915_READ16(IER);
> else
> error->ier = I915_READ(IER);
> - for_each_pipe(pipe)
> - error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
> }
>
> /* 4: Everything else */
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 1390ab5..4d8d875 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11925,6 +11925,7 @@ struct intel_display_error_state {
> struct intel_pipe_error_state {
> bool power_domain_on;
> u32 source;
> + u32 stat;
> } pipe[I915_MAX_PIPES];
>
> struct intel_plane_error_state {
> @@ -12006,6 +12007,7 @@ intel_display_capture_error_state(struct drm_device
> *dev)
> }
>
> error->pipe[i].source = I915_READ(PIPESRC(i));
> + error->pipe[i].stat = I915_READ(PIPESTAT(i));
This needs a !HAS_PCH_SPLIT check
> }
>
> error->num_transcoders = INTEL_INFO(dev)->num_pipes;
> @@ -12056,6 +12058,7 @@ intel_display_print_error_state(struct
> drm_i915_error_state_buf *m,
> err_printf(m, " Power: %s\n",
> error->pipe[i].power_domain_on ? "on" : "off");
> err_printf(m, " SRC: %08x\n", error->pipe[i].source);
> + err_printf(m, " STAT: %08x\n", error->pipe[i].stat);
>
> err_printf(m, "Plane [%d]:\n", i);
> err_printf(m, " CNTR: %08x\n", error->plane[i].control);
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 09/25] drm/i915: get a runtime PM ref for the deferred GPU reset work
On Mon, Apr 14, 2014 at 08:24:30PM +0300, Imre Deak wrote: > Atm we can end up in the GPU reset deferred work in D3 state if the last > runtime PM reference is dropped between detecting a hang/scheduling the > work and executing the work. At least one such case I could trigger is > the simulated reset via the i915_wedged debugfs entry. Fix this by > disabling RPM before scheduling the work until the end of the work. > > v2: > - Instead of getting/putting the RPM reference in the reset work itself, > get it already before scheduling the work. By this we also prevent > going to D3 before the work gets to run, in addition to making sure > that we run the work itself in D0. (Ville, Daniel) > > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_dma.c | 8 +++- > drivers/gpu/drm/i915/i915_irq.c | 21 - > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 0b38f88..6398280 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1823,7 +1823,13 @@ int i915_driver_unload(struct drm_device *dev) > > /* Free error state after interrupts are fully disabled. */ > del_timer_sync(&dev_priv->gpu_error.hangcheck_timer); > - cancel_work_sync(&dev_priv->gpu_error.work); > + if (!cancel_work_sync(&dev_priv->gpu_error.work)) '!' shouldn't be there > + /* > + * The following won't make any difference in the PM state, > + * since RPM is disabled already, but do it still for > + * consistency. > + */ > + intel_runtime_pm_put(dev_priv); > i915_destroy_error_state(dev); > > if (dev->pdev->msi_enabled) > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a651d0d..5e079d8 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2210,6 +2210,9 @@ static void i915_error_work_func(struct work_struct > *work) >*/ > i915_error_wake_up(dev_priv, true); > } > + > + /* Drop the ref we took when scheduling this work. */ > + intel_runtime_pm_put(dev_priv); > } > > static void i915_report_and_clear_eir(struct drm_device *dev) > @@ -2353,8 +2356,24 @@ void i915_handle_error(struct drm_device *dev, bool > wedged, >* state of outstanding pagelips). Hence it must not be run on our own >* dev-priv->wq work queue for otherwise the flush_work in the pageflip >* code will deadlock. > + * > + * It's guaranteed that here we are in D0 state, since we can only get > + * here via one of the following paths: > + * - From an IRQ handler's error detection. -> The driver must make > + * sure that IRQs are unmasked only while holding an RPM ref. > + * - From hang-check due to a blocked request. -> The request holds an > + * RPM ref, that's only released in i915_gpu_idle() which in turn > + * won't be called until the request is finished. > + * - From hang-check due to a flip hang. -> We have an RPM ref because > + * of the active modeset. > + * - From debugfs i915_wedged_set(). -> The caller takes an explicit > + * RPM ref. > + * Take here an atomic RPM ref still to make sure that we don't > + * re-enter D3 until the error work gets to run and completes the > + * reset. >*/ > - schedule_work(&dev_priv->gpu_error.work); > + if (schedule_work(&dev_priv->gpu_error.work)) > + intel_runtime_pm_get_noresume(dev_priv); > } > > static void __always_unused i915_pageflip_stall_check(struct drm_device > *dev, int pipe) > -- > 1.8.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Break encoder->crtc link separately in intel_sanitize_crtc()
On Wed, Apr 16, 2014 at 11:16:45AM +0200, Egbert Eich wrote:
> Depending on the SDVO output_flags SDVO may have multiple connectors
> linking to the same encoder (in intel_connector->encoder->base).
> Only one of those connectors should be active (ie link to the encoder
> thru drm_connector->encoder).
> If intel_connector_break_all_links() is called from intel_sanitize_crtc()
> we may break the crtc connection of an encoder thru an inactive connector
> in which case intel_connector_break_all_links() will not be called again
> for the active connector if this happens to come later in the list due to:
> if (connector->encoder->base.crtc != &crtc->base)
> continue;
> in intel_sanitize_crtc().
> This will however leave the drm_connector->encoder linkage for this
> active connector in place. Subsequently this will cause multiple
> warnings in intel_connector_check_state() to trigger and the driver
> will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).
>
> To avoid this remove intel_connector_break_all_links() and move its
> code to its two calling functions: intel_sanitize_crtc() and
> intel_sanitize_encoder().
> This allows to implement the link breaking more flexibly matching
> the surrounding code: ie. in intel_sanitize_crtc() we can break the
> crtc link separatly after the links to the encoders have been
> broken which avoids above problem.
>
> Signed-off-by: Egbert Eich
>
> v2: This patch takes care of the concernes voiced by Chris Wilson
> and Daniel Vetter that only breaking links if the drm_connector
> is linked to an encoder may miss some links.
> ---
> drivers/gpu/drm/i915/intel_display.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 1390ab5..c276733 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11386,15 +11386,6 @@ void intel_modeset_init(struct drm_device *dev)
> }
> }
>
> -static void
> -intel_connector_break_all_links(struct intel_connector *connector)
> -{
> - connector->base.dpms = DRM_MODE_DPMS_OFF;
> - connector->base.encoder = NULL;
> - connector->encoder->connectors_active = false;
> - connector->encoder->base.crtc = NULL;
> -}
> -
> static void intel_enable_pipe_a(struct drm_device *dev)
> {
> struct intel_connector *connector;
> @@ -11476,8 +11467,16 @@ static void intel_sanitize_crtc(struct intel_crtc
> *crtc)
> if (connector->encoder->base.crtc != &crtc->base)
> continue;
>
> - intel_connector_break_all_links(connector);
> + connector->base.dpms = DRM_MODE_DPMS_OFF;
> + connector->encoder->connectors_active = false;
I'd move the encoder->connectors_active = false down into the encoder
loop, but doesn't really matter. Jani can frob this when applying for
3.15-fixes.
This regression has been introduced in
commit 24929352481f085c5f85d4d4cbc919ddf106d381
Author: Daniel Vetter
Date: Mon Jul 2 20:28:59 2012 +0200
drm/i915: read out the modeset hw state at load and resume time
so goes back to the very beginning of the modeset rework.
Reviewed-by: Daniel Vetter
Cc: sta...@vger.kernel.org
Thanks, Daniel
> + connector->base.encoder = NULL;
> }
> + /* multiple connectors may have the same encoder:
> + * break crtc link separately */
> + list_for_each_entry(connector, &dev->mode_config.connector_list,
> + base.head)
> + if (connector->encoder->base.crtc == &crtc->base)
> + connector->encoder->base.crtc = NULL;
>
> WARN_ON(crtc->active);
> crtc->base.enabled = false;
> @@ -11559,6 +11558,8 @@ static void intel_sanitize_encoder(struct
> intel_encoder *encoder)
> drm_get_encoder_name(&encoder->base));
> encoder->disable(encoder);
> }
> + encoder->base.crtc = NULL;
> + encoder->connectors_active = false;
>
> /* Inconsistent output/port/pipe state happens presumably due to
>* a bug in one of the get_hw_state functions. Or someplace else
> @@ -11569,8 +11570,8 @@ static void intel_sanitize_encoder(struct
> intel_encoder *encoder)
> base.head) {
>
Re: [Intel-gfx] [PATCH] drm/i915: Resurrect warning from intel_encoder_crtc_ok()
On Wed, Apr 16, 2014 at 11:46 AM, Egbert Eich wrote:
> Bail out if crtc is NULL to keep the driver from crashing.
>
> Signed-off-by: Egbert Eich
> ---
> drivers/gpu/drm/i915/intel_display.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c276733..dfebced 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10257,6 +10257,11 @@ intel_modeset_stage_output_state(struct drm_device
> *dev,
> new_crtc = set->crtc;
> }
>
> + if (!new_crtc) {
> + WARN(1, "crtc not set!");
> + return -EINVAL;
> + }
This looks really ugly. Can you please supply some context about
where/how this blows up? We really shouldn't ever attempt a modeset
with a NULL crtc ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Resurrect warning from intel_encoder_crtc_ok()
Bail out if crtc is NULL to keep the driver from crashing.
Signed-off-by: Egbert Eich
---
drivers/gpu/drm/i915/intel_display.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index c276733..dfebced 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10257,6 +10257,11 @@ intel_modeset_stage_output_state(struct drm_device
*dev,
new_crtc = set->crtc;
}
+ if (!new_crtc) {
+ WARN(1, "crtc not set!");
+ return -EINVAL;
+ }
+
/* Make sure the new CRTC will work with the encoder */
if (!drm_encoder_crtc_ok(&connector->new_encoder->base,
new_crtc)) {
--
1.8.4.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] The whole round of i-g-t testing cost too long running time
> I think stopping the tests after 10 minutes is ok, but in general the point of > stress tests is to beat on the kernel for corner cases. E.g. > even with todays extensive set of stress tests some spurious OOM bugs can > only be reproduced in 1 out of 5 runs. Reducing the test time could severely > impact the testing power of a test, so I'm vary for doing that. [Guang YANG] Agree with you, but whether stop these stress testing after 10M if it not finished will impact the effort? > But there are tricks to speed up some tests which shouldn't affect the power > of > the testcase to find bugs, and we should definitely look into those. > > Does this mean that due to PRTS we now have fewer machines running tests > on drm-intel-nightly? I've thought the idea is to share machines on an > as-needed basis, with -nightly testing getting priority? [Guang YANG] No, we haven’t changed the scale for existed nightly testing resources ,and the PRTS can supplement our nightly well with more old platforms(ILK,PNV), but we still lack of BDW/BYT resources in both PRTS and nightly. These new platforms are important and most of the bug reported on them. Running tests, checking patches, doing the bisect work keep the machine resources always busy. Shuang still hard work on sharing machines between PRTS and nightly. > > > Yeah keeping your overall test-runner infrastructure makes sense. The idea > behind my proposal to use piglit to execute the individual tests is to share > analysis scripts. That won't make the tests run any faster, but it should (in > the > long term at least) speed up the triaging a lot. And the high amount of time > required for bug triaging also seems to be an issue for you guys. [Guang YANG] Great, improve the analysis script or tool will be comfortable for QA, We can catch the regression more accurately for such as BackTrace. Do you have any plans? QA can also take over some tasks. > I agree that for a quick sanity test a reduced test set makes sense. > Which is why we have a testcase naming convention which can be used > together with the piglit -x and -t flags. I do that a lot when developing > things. > > But for regression testing imo only the full test suite makes sense, otherwise > we just have a false sense of security. I.e. if the full set means we can > only run > it every 2 days then I prefer that over running only a subset. Also very often > there are other issues delaying the time between when a buggy patch was > committed and when the full bug report is available, so imo the 10h runtime > isn't too bad from my pov really. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch[Guang YANG] ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Break encoder->crtc link separately in intel_sanitize_crtc()
Depending on the SDVO output_flags SDVO may have multiple connectors
linking to the same encoder (in intel_connector->encoder->base).
Only one of those connectors should be active (ie link to the encoder
thru drm_connector->encoder).
If intel_connector_break_all_links() is called from intel_sanitize_crtc()
we may break the crtc connection of an encoder thru an inactive connector
in which case intel_connector_break_all_links() will not be called again
for the active connector if this happens to come later in the list due to:
if (connector->encoder->base.crtc != &crtc->base)
continue;
in intel_sanitize_crtc().
This will however leave the drm_connector->encoder linkage for this
active connector in place. Subsequently this will cause multiple
warnings in intel_connector_check_state() to trigger and the driver
will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).
To avoid this remove intel_connector_break_all_links() and move its
code to its two calling functions: intel_sanitize_crtc() and
intel_sanitize_encoder().
This allows to implement the link breaking more flexibly matching
the surrounding code: ie. in intel_sanitize_crtc() we can break the
crtc link separatly after the links to the encoders have been
broken which avoids above problem.
Signed-off-by: Egbert Eich
v2: This patch takes care of the concernes voiced by Chris Wilson
and Daniel Vetter that only breaking links if the drm_connector
is linked to an encoder may miss some links.
---
drivers/gpu/drm/i915/intel_display.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..c276733 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11386,15 +11386,6 @@ void intel_modeset_init(struct drm_device *dev)
}
}
-static void
-intel_connector_break_all_links(struct intel_connector *connector)
-{
- connector->base.dpms = DRM_MODE_DPMS_OFF;
- connector->base.encoder = NULL;
- connector->encoder->connectors_active = false;
- connector->encoder->base.crtc = NULL;
-}
-
static void intel_enable_pipe_a(struct drm_device *dev)
{
struct intel_connector *connector;
@@ -11476,8 +11467,16 @@ static void intel_sanitize_crtc(struct intel_crtc
*crtc)
if (connector->encoder->base.crtc != &crtc->base)
continue;
- intel_connector_break_all_links(connector);
+ connector->base.dpms = DRM_MODE_DPMS_OFF;
+ connector->encoder->connectors_active = false;
+ connector->base.encoder = NULL;
}
+ /* multiple connectors may have the same encoder:
+* break crtc link separately */
+ list_for_each_entry(connector, &dev->mode_config.connector_list,
+ base.head)
+ if (connector->encoder->base.crtc == &crtc->base)
+ connector->encoder->base.crtc = NULL;
WARN_ON(crtc->active);
crtc->base.enabled = false;
@@ -11559,6 +11558,8 @@ static void intel_sanitize_encoder(struct intel_encoder
*encoder)
drm_get_encoder_name(&encoder->base));
encoder->disable(encoder);
}
+ encoder->base.crtc = NULL;
+ encoder->connectors_active = false;
/* Inconsistent output/port/pipe state happens presumably due to
* a bug in one of the get_hw_state functions. Or someplace else
@@ -11569,8 +11570,8 @@ static void intel_sanitize_encoder(struct intel_encoder
*encoder)
base.head) {
if (connector->encoder != encoder)
continue;
-
- intel_connector_break_all_links(connector);
+ connector->base.dpms = DRM_MODE_DPMS_OFF;
+ connector->base.encoder = NULL;
}
}
/* Enabled encoders without active connectors will be fixed in
--
1.8.4.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Return -ENOENT for unknown contexts
From: Mika Kuoppala
If hw_contexts are disabled, we always return the per file
descriptor default context stats. Make sure that the context
is correctly given and fail accordingly if not.
Testcase: igt/gem_reset_stats/params
Signed-off-by: Mika Kuoppala
---
drivers/gpu/drm/i915/i915_gem_context.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 30b355a..85c890c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -582,8 +582,12 @@ i915_gem_context_get(struct drm_i915_file_private
*file_priv, u32 id)
{
struct i915_hw_context *ctx;
- if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
+ if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev)) {
+ if (id != DEFAULT_CONTEXT_ID)
+ return ERR_PTR(-ENOENT);
+
return file_priv->private_default_ctx;
+ }
ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
if (!ctx)
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
On Tue, Apr 15, 2014 at 07:43:07PM -0700, Ben Widawsky wrote:
> On Mon, Apr 14, 2014 at 10:55:53PM +0300, Ville Syrjälä wrote:
> > On Mon, Apr 14, 2014 at 10:41:14PM +0530, deepa...@intel.com wrote:
> > > From: Ben Widawsky
> > >
> > > Almost all of it is reusable from the existing code. The primary
> > > difference is we need to do even less in the interrupt handler, since
> > > interrupts are not shared in the same way.
> > >
> > > The patch is mostly a copy-paste of the existing snb+ code, with updates
> > > to the relevant parts requiring changes to the interrupt handling. As
> > > such it /should/ be relatively trivial. It's highly likely that I missed
> > > some places where I need a gen8 version of the PM interrupts, but it has
> > > become invisible to me by now.
> > >
> > > This patch could probably be split into adding the new functions,
> > > followed by actually handling the interrupts. Since the code is
> > > currently disabled (and broken) I think the patch stands better by
> > > itself.
> > >
> > > v2: Move the commit about not touching the ringbuffer interrupt to the
> > > snb_* function where it belongs (Rodrigo)
> > >
> > > v3: Rebased on Paulo's runtime PM changes
> > >
> > > v4: Not well validated, but rebase on
> > > commit 730488b2eddded4497f63f70867b1256cd9e117c
> > > Author: Paulo Zanoni
> > > Date: Fri Mar 7 20:12:32 2014 -0300
> > >
> > > drm/i915: kill dev_priv->pm.regsave
> > >
> > > v5: Rebased on latest code base. (Deepak)
> > >
> > > Signed-off-by: Ben Widawsky
> > >
> > > Conflicts:
> > > drivers/gpu/drm/i915/i915_irq.c
> >
> > IIRC Daniel doesn't like these conflict markers. So should be dropped.
> >
>
> I like the conflict markers generally. Daniel can kill it if he likes,
> but thanks for the input. I've killed it this time around, but I don't
> plan on it for the future.
>
> > > ---
> > > drivers/gpu/drm/i915/i915_irq.c | 81
> > > +---
> > > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > drivers/gpu/drm/i915/intel_pm.c | 38 ++-
> > > 4 files changed, 115 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index 7a4d3ae..96c459a 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device
> > > *dev)
> > > return true;
> > > }
> > >
> > > +/**
> > > + * bdw_update_pm_irq - update GT interrupt 2
> > > + * @dev_priv: driver private
> > > + * @interrupt_mask: mask of interrupt bits to update
> > > + * @enabled_irq_mask: mask of interrupt bits to enable
> > > + *
> > > + * Copied from the snb function, updated with relevant register offsets
> > > + */
> > > +static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
> > > + uint32_t interrupt_mask,
> > > + uint32_t enabled_irq_mask)
> > > +{
> > > + uint32_t new_val;
> > > +
> > > + assert_spin_locked(&dev_priv->irq_lock);
> > > +
> > > + if (dev_priv->pm.irqs_disabled) {
> > > + WARN(1, "IRQs disabled\n");
> > > + return;
> > > + }
> > > +
> > > + new_val = dev_priv->pm_irq_mask;
> > > + new_val &= ~interrupt_mask;
> > > + new_val |= (~enabled_irq_mask & interrupt_mask);
> > > +
> > > + if (new_val != dev_priv->pm_irq_mask) {
> > > + dev_priv->pm_irq_mask = new_val;
> > > + I915_WRITE(GEN8_GT_IMR(2), I915_READ(GEN8_GT_IMR(2)) |
> > > +dev_priv->pm_irq_mask);
> > > + POSTING_READ(GEN8_GT_IMR(2));
> > > + }
> > > +}
> > > +
> > > +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > > +{
> > > + bdw_update_pm_irq(dev_priv, mask, mask);
> > > +}
> > > +
> > > +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > > +{
> > > + bdw_update_pm_irq(dev_priv, mask, 0);
> > > +}
> > > +
> > > +
> >
> > Unnecessary empty line.
> >
>
> Got it, thanks.
>
> > > static bool cpt_can_enable_serr_int(struct drm_device *dev)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct
> > > *work)
> > > spin_lock_irq(&dev_priv->irq_lock);
> > > pm_iir = dev_priv->rps.pm_iir;
> > > dev_priv->rps.pm_iir = 0;
> > > - /* Make sure not to corrupt PMIMR state used by ringbuffer code */
> > > - snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > > + if (IS_BROADWELL(dev_priv->dev))
> > > + bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > > + else {
> > > + /* Make sure not to corrupt PMIMR state used by ringbuffer */
> > > + snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > > + /* Make sure we didn't queue anything we're not going to
> > > + * process. */
> > > + WARN_ON(pm_ii
Re: [Intel-gfx] The whole round of i-g-t testing cost too long running time
On Wed, Apr 16, 2014 at 7:47 AM, Yang, Guang A wrote: > Ok there are a few cases where we can indeed make tests faster, but it will > be work for us. And that won't really speed up much since we're adding piles > more testcases at a pretty quick rate. And many of these new testcases are > CRC based, so inheritely take some time to run. > > [He, Shuang] OK, so it takes at least n/60 in usual case to have result > detected plus additional execution time, depending on how many rounds of > testing. We will be absolutely happy to see more tests coming that is useful > > [Guang YANG] Except these CRC case, some stress case may also cost a bit of > time, especially on some old platforms. Maybe can reduce the loop in that > kind of stress case? I think stopping the tests after 10 minutes is ok, but in general the point of stress tests is to beat on the kernel for corner cases. E.g. even with todays extensive set of stress tests some spurious OOM bugs can only be reproduced in 1 out of 5 runs. Reducing the test time could severely impact the testing power of a test, so I'm vary for doing that. But there are tricks to speed up some tests which shouldn't affect the power of the testcase to find bugs, and we should definitely look into those. > So I think longer-term we simply need to throw more machines at the problem > and run testcases in parallel on identical machines. > > [He, Shuang] This would be the perfect way to go if all tests are really > feasible to take long time to run. If we get more identical test machines, > then problem solved > > [Guang YANG] shuang’s PRTS can cover some work for i-g-t testing and catch > some regressions. Most of the i-g-t bugs are from HSW+, so I hope keep focus > on these new platforms. but now we don’t have enough free machine resource > (such as BYT,BDW)to support one machine only run i-g-t in nightly. Does this mean that due to PRTS we now have fewer machines running tests on drm-intel-nightly? I've thought the idea is to share machines on an as-needed basis, with -nightly testing getting priority? > Wrt analyzing issues I think the right approach for moving forward is: > a) switch to piglit to run tests, not just enumerate them. This will allow > QA and developers to share testcase analysis. > > [He, Shuang] Yes, though this could not actually accelerate the test. We > could directly wrap over piglit to run testing (have other control process > to monitor and collecting test results) > > [Guang YANG] Yeah, Shuang said is what we did. Piglit have been improved > more powerful, but our infrastructure have better remote control and result > collecting. If it will be comfortable for Developers to see the case result > from running piglit, we can discuss how to match these two framework > together. Yeah keeping your overall test-runner infrastructure makes sense. The idea behind my proposal to use piglit to execute the individual tests is to share analysis scripts. That won't make the tests run any faster, but it should (in the long term at least) speed up the triaging a lot. And the high amount of time required for bug triaging also seems to be an issue for you guys. > b) add automated analysis for time-consuming and error prone cases like > dmesg warnings and backtraces. Thomas&I have just discussed a few ideas in > this are in our 1:1 today. > > Reducing the set of igt tests we run is imo pointless: The goal of igt is to > hit corner-cases, arbitrarily selecting which kinds of corner-cases we test > just means that we have a nice illusion about our test coverage. > > [He, Shuang] I don’t think select a subset of test cases to run is > pointless. It’s a trade-off between speed and correctness. For our nightly > testing it’s not so useful to run only a small set of testing. But for fast > sanity testing, it should be easier, which is supposed to catch regression > in major/critical functionality (So other developers and QA could continue > their work). I agree that for a quick sanity test a reduced test set makes sense. Which is why we have a testcase naming convention which can be used together with the piglit -x and -t flags. I do that a lot when developing things. But for regression testing imo only the full test suite makes sense, otherwise we just have a false sense of security. I.e. if the full set means we can only run it every 2 days then I prefer that over running only a subset. Also very often there are other issues delaying the time between when a buggy patch was committed and when the full bug report is available, so imo the 10h runtime isn't too bad from my pov really. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect
On Wed, Apr 16, 2014 at 07:58:48AM +0200, Egbert Eich wrote: > Daniel Vetter writes: > > On Mon, Apr 14, 2014 at 07:26:09PM +0200, Egbert Eich wrote: > > > Depending on the SDVO output_flags SDVO may have multiple connectors. > > > These are all initialized in intel_sdvo_output_setup(). The connector > > > that is initialized later will override the encoder_type that has been > > > set up by an earlier connector type initialization. Eventually the > > > one that comes last wins. > > > Eventually when intel_sdvo_detect() is called the active connector is > > > determined. > > > Delay encoder type initialization until the active connector is known > > > and set it to the type that corresponds to this connector. > > > > > > Signed-off-by: Egbert Eich > > > > Hm, has this any effect on the code itself? I think if we want to fix this > > just for optics we should add a new DRM_MODE_ENCODER_MULTI. At least I > > have an sdvo here which has a dac, hdmi and tv-out ... > > With the present logic the last connector in the list matching a flag bit > will win the encoder type. The encoder type is presently just used for > (debug) messages, so it is cosmetic. I'm vary of changing the encoder type at runtime tbh. We use the same hack in intel_ddi.c to switch between hdmi and dp mode, and the results aren't pretty imo. For debug output imo adding a new "multi" encoder type is better. And if we need the type somehow to set up the right connector for sdvo encoders with more than one detected output, then we need to track this piece of state somewhere in our modeset state (either with the connector->encoder links or with a bit of state in the pipe config structure). Which also means we need state read-out and cross-check support to make sure it really does what we want. Storing such state in global structure tends to lead to subtile bugs and prevent proper pre-compute/commit semantics for modeset changes. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
On Tue, Apr 15, 2014 at 07:43:07PM -0700, Ben Widawsky wrote: > On Mon, Apr 14, 2014 at 10:55:53PM +0300, Ville Syrjälä wrote: > > On Mon, Apr 14, 2014 at 10:41:14PM +0530, deepa...@intel.com wrote: > > > From: Ben Widawsky > > > > > > Almost all of it is reusable from the existing code. The primary > > > difference is we need to do even less in the interrupt handler, since > > > interrupts are not shared in the same way. > > > > > > The patch is mostly a copy-paste of the existing snb+ code, with updates > > > to the relevant parts requiring changes to the interrupt handling. As > > > such it /should/ be relatively trivial. It's highly likely that I missed > > > some places where I need a gen8 version of the PM interrupts, but it has > > > become invisible to me by now. > > > > > > This patch could probably be split into adding the new functions, > > > followed by actually handling the interrupts. Since the code is > > > currently disabled (and broken) I think the patch stands better by > > > itself. > > > > > > v2: Move the commit about not touching the ringbuffer interrupt to the > > > snb_* function where it belongs (Rodrigo) > > > > > > v3: Rebased on Paulo's runtime PM changes > > > > > > v4: Not well validated, but rebase on > > > commit 730488b2eddded4497f63f70867b1256cd9e117c > > > Author: Paulo Zanoni > > > Date: Fri Mar 7 20:12:32 2014 -0300 > > > > > > drm/i915: kill dev_priv->pm.regsave > > > > > > v5: Rebased on latest code base. (Deepak) > > > > > > Signed-off-by: Ben Widawsky > > > > > > Conflicts: > > > drivers/gpu/drm/i915/i915_irq.c > > > > IIRC Daniel doesn't like these conflict markers. So should be dropped. > > > > I like the conflict markers generally. Daniel can kill it if he likes, > but thanks for the input. I've killed it this time around, but I don't > plan on it for the future. Imo leaving them in is just lazy. Either there was some real conflict that required real work, and then you should rev the patch revision and take note about the changes in the in-patch changelog. Or the conflict was trivial, in which case the left-behind marker is meaningless and doesn't add anything useful to the commit message. So please remove them and if it makes sense augment the patch revision log. See e.g. the rebase notes (where I mention the upstream changes that required the rebase) I add for -internal patches. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
