Re: [Intel-gfx] [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl
On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick wrote: > On 10/27/2013 05:30 AM, Daniel Vetter wrote: >> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote: >>> Since the Mesa merge window is closing soon, I'm finally getting back on >>> this. I've pushed a rebase of my old Mesa branch to my fd.o repo >>> >>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3 >>> >>> I have a couple questions... >>> >>> 1. Has any of this landed an a kernel tree anywhere? >> >> Afaik everything but the actual ioctl and i-g-t testcase has landed. > > And that stuff will land once my patches hit the Mesa list or ... ? Yup. >>> 2. Has any support code landed in a libdrm tree anywhere? >> >> Dunno whether Mika has libdrm patches. Since mesa is the only expected >> user I'd just go with putting the ioctl wrapper (using the drmIoctl >> helper) into mesa itself, that get rids of a dep for merging this support. > > What's the right way to get the ctx_id out of the drm_intel_context? > That struct is private to libdrm, but the ioctl needs it. Hm, I guess then we need something in libdrm. I've just figured for stuff that only mesa really cares about like the hw contexts here libdrm wrappers are a bit pointless since it adds a 2nd ABI barrier with no real gain. -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] drm/i915: Capture batchbuffer state upon GPU hang
On Tue, Oct 29, 2013 at 08:09:04PM +, Chris Wilson wrote:
> The bbstate contains useful bits of debugging information such as
> whether the batch is being read from GTT or PPGTT, or whether it is
> allowed to execute privileged instructions.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gpu_error.c | 2 ++
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3d65b511e320..ca1b223356eb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -301,6 +301,7 @@ struct drm_i915_error_state {
> u32 cpu_ring_tail[I915_NUM_RINGS];
> u32 error; /* gen6+ */
> u32 err_int; /* gen7 */
> + u32 bbstate[I915_NUM_RINGS];
> u32 instpm[I915_NUM_RINGS];
> u32 instps[I915_NUM_RINGS];
> u32 extra_instdone[I915_NUM_INSTDONE_REG];
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 918d978dce4a..cdd9121a28bb 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -251,6 +251,7 @@ static void i915_ring_error_state(struct
> drm_i915_error_state_buf *m,
> if (ring == RCS && INTEL_INFO(dev)->gen >= 4)
> err_printf(m, " BBADDR: 0x%08llx\n", error->bbaddr);
>
> + err_printf(m, " BB_STATE: 0x%08x\n", error->bbstate[ring]);
This seems to only exist on gen4+
-Daniel
> if (INTEL_INFO(dev)->gen >= 4)
> err_printf(m, " INSTPS: 0x%08x\n", error->instps[ring]);
> err_printf(m, " INSTPM: 0x%08x\n", error->instpm[ring]);
> @@ -735,6 +736,7 @@ static void i915_record_ring_state(struct drm_device *dev,
> }
>
> error->waiting[ring->id] = waitqueue_active(&ring->irq_queue);
> + error->bbstate[ring->id] = I915_READ(RING_BBSTATE(ring->mmio_base));
> error->instpm[ring->id] = I915_READ(RING_INSTPM(ring->mmio_base));
> error->seqno[ring->id] = ring->get_seqno(ring, false);
> error->acthd[ring->id] = intel_ring_get_active_head(ring);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dbad19f4a983..9d79d28decb6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -722,6 +722,7 @@
> #define NOPID0x02094
> #define HWSTAM 0x02098
> #define DMA_FADD_I8XX0x020d0
> +#define RING_BBSTATE(base) ((base)+0x110)
>
> #define ERROR_GEN6 0x040a0
> #define GEN7_ERR_INT 0x44040
> --
> 1.8.4.rc3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Capture batchbuffer state upon GPU hang
The bbstate contains useful bits of debugging information such as
whether the batch is being read from GTT or PPGTT, or whether it is
allowed to execute privileged instructions.
v2: Only record BB_STATE for gen4+
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gpu_error.c | 4 +++-
drivers/gpu/drm/i915/i915_reg.h | 1 +
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3d65b511e320..ca1b223356eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -301,6 +301,7 @@ struct drm_i915_error_state {
u32 cpu_ring_tail[I915_NUM_RINGS];
u32 error; /* gen6+ */
u32 err_int; /* gen7 */
+ u32 bbstate[I915_NUM_RINGS];
u32 instpm[I915_NUM_RINGS];
u32 instps[I915_NUM_RINGS];
u32 extra_instdone[I915_NUM_INSTDONE_REG];
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 918d978dce4a..c5aee63a3a11 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -250,7 +250,8 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, " INSTDONE: 0x%08x\n", error->instdone[ring]);
if (ring == RCS && INTEL_INFO(dev)->gen >= 4)
err_printf(m, " BBADDR: 0x%08llx\n", error->bbaddr);
-
+ if (INTEL_INFO(dev)->gen >= 4)
+ err_printf(m, " BB_STATE: 0x%08x\n", error->bbstate[ring]);
if (INTEL_INFO(dev)->gen >= 4)
err_printf(m, " INSTPS: 0x%08x\n", error->instps[ring]);
err_printf(m, " INSTPM: 0x%08x\n", error->instpm[ring]);
@@ -727,6 +728,7 @@ static void i915_record_ring_state(struct drm_device *dev,
error->instps[ring->id] =
I915_READ(RING_INSTPS(ring->mmio_base));
if (ring->id == RCS)
error->bbaddr = I915_READ64(BB_ADDR);
+ error->bbstate[ring->id] =
I915_READ(RING_BBSTATE(ring->mmio_base));
} else {
error->faddr[ring->id] = I915_READ(DMA_FADD_I8XX);
error->ipeir[ring->id] = I915_READ(IPEIR);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dbad19f4a983..9d79d28decb6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -722,6 +722,7 @@
#define NOPID 0x02094
#define HWSTAM 0x02098
#define DMA_FADD_I8XX 0x020d0
+#define RING_BBSTATE(base) ((base)+0x110)
#define ERROR_GEN6 0x040a0
#define GEN7_ERR_INT 0x44040
--
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] drm/i915: handle faked missed interrupts as simulated hangs, too
On Mon, Oct 28, 2013 at 09:24:13AM +0100, Daniel Vetter wrote:
> Otherwise QA will report this as a real hang when running igt
> ZZ_missed_irq.
>
> v2: Actually test the right stuff and really shut up the DRM_ERROR
> output ...
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70747
> Tested-by: lu hua
> Signed-off-by: Daniel Vetter
Merged with Chris' irc-ack.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_irq.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1a7dc7754e2f..68936da7c25d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2258,8 +2258,12 @@ static void i915_hangcheck_elapsed(unsigned long data)
> if (waitqueue_active(&ring->irq_queue)) {
> /* Issue a wake-up to catch stuck h/w.
> */
> if (!test_and_set_bit(ring->id,
> &dev_priv->gpu_error.missed_irq_rings)) {
> - DRM_ERROR("Hangcheck timer
> elapsed... %s idle\n",
> - ring->name);
> + if
> (!(dev_priv->gpu_error.test_irq_rings & intel_ring_flag(ring)))
> + DRM_ERROR("Hangcheck
> timer elapsed... %s idle\n",
> + ring->name);
> + else
> + DRM_INFO("Fake missed
> irq on %s\n",
> + ring->name);
> wake_up_all(&ring->irq_queue);
> }
> /* Safeguard against driver failure */
> --
> 1.8.4.rc3
>
--
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] drm/i915: Capture batchbuffer state upon GPU hang
On Wed, Oct 30, 2013 at 09:28:22AM +, Chris Wilson wrote: > The bbstate contains useful bits of debugging information such as > whether the batch is being read from GTT or PPGTT, or whether it is > allowed to execute privileged instructions. > > v2: Only record BB_STATE for gen4+ > > Signed-off-by: Chris Wilson Queued for -next, thanks for the patch. -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] drm/i915: Capture batchbuffer state upon GPU hang
On Wed, Oct 30, 2013 at 09:28:22AM +, Chris Wilson wrote: > The bbstate contains useful bits of debugging information such as > whether the batch is being read from GTT or PPGTT, or whether it is > allowed to execute privileged instructions. > > v2: Only record BB_STATE for gen4+ > > Signed-off-by: Chris Wilson Queued for -next, thanks for the patch. -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: refactor ilk display interrupt handling
On Mon, Oct 21, 2013 at 07:48:58PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 21, 2013 at 06:04:36PM +0200, Daniel Vetter wrote:
> > - Use a for_each_loop and add the corresponding #defines.
> > - Drop the _ILK postfix on the existing DE_PIPE_VBLANK macro for
> > consistency with everything else.
> > - Also use macros (and add the missing one for plane flips) for the
> > ivb display interrupt handler.
> >
> > Cc: Ville Syrjälä
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 50
> > +
> > drivers/gpu/drm/i915/i915_reg.h | 7 --
> > 2 files changed, 26 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index ce94afc..062a6d6 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1535,6 +1535,7 @@ static void cpt_irq_handler(struct drm_device *dev,
> > u32 pch_iir)
> > static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + enum pipe pipe;
> >
> > if (de_iir & DE_AUX_CHANNEL_A)
> > dp_aux_irq_handler(dev);
> > @@ -1542,37 +1543,26 @@ static void ilk_display_irq_handler(struct
> > drm_device *dev, u32 de_iir)
> > if (de_iir & DE_GSE)
> > intel_opregion_asle_intr(dev);
> >
> > - if (de_iir & DE_PIPEA_VBLANK)
> > - drm_handle_vblank(dev, 0);
> > -
> > - if (de_iir & DE_PIPEB_VBLANK)
> > - drm_handle_vblank(dev, 1);
> > -
> > if (de_iir & DE_POISON)
> > DRM_ERROR("Poison interrupt\n");
> >
> > - if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
> > - if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_A, false))
> > - DRM_DEBUG_DRIVER("Pipe A FIFO underrun\n");
> > -
> > - if (de_iir & DE_PIPEB_FIFO_UNDERRUN)
> > - if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_B, false))
> > - DRM_DEBUG_DRIVER("Pipe B FIFO underrun\n");
> > -
> > - if (de_iir & DE_PIPEA_CRC_DONE)
> > - i9xx_pipe_crc_irq_handler(dev, PIPE_A);
> > + for_each_pipe(pipe) {
> > + if (de_iir & DE_PIPE_VBLANK(pipe))
> > + drm_handle_vblank(dev, pipe);
> >
> > - if (de_iir & DE_PIPEB_CRC_DONE)
> > - i9xx_pipe_crc_irq_handler(dev, PIPE_B);
> > + if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> > + if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
> > false))
> > + DRM_DEBUG_DRIVER("Pipe %c FIFO underrun\n",
> > +pipe_name(pipe));
> >
> > - if (de_iir & DE_PLANEA_FLIP_DONE) {
> > - intel_prepare_page_flip(dev, 0);
> > - intel_finish_page_flip_plane(dev, 0);
> > - }
> > + if (de_iir & DE_PIPE_CRC_DONE(pipe))
> > + i9xx_pipe_crc_irq_handler(dev, pipe);
> >
> > - if (de_iir & DE_PLANEB_FLIP_DONE) {
> > - intel_prepare_page_flip(dev, 1);
> > - intel_finish_page_flip_plane(dev, 1);
> > + /* plane/pipes map 1:1 on ilk+ */
> > + if (de_iir & DE_PLANE_FLIP_DONE(pipe)) {
> > + intel_prepare_page_flip(dev, pipe);
> > + intel_finish_page_flip_plane(dev, pipe);
> > + }
> > }
> >
> > /* check event from PCH */
> > @@ -1607,9 +1597,11 @@ static void ivb_display_irq_handler(struct
> > drm_device *dev, u32 de_iir)
> > intel_opregion_asle_intr(dev);
> >
> > for_each_pipe(i) {
> > - if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
> > + if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> ^ ^
>
> > drm_handle_vblank(dev, i);
> > - if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
> > +
> > + /* plane/pipes map 1:1 on ilk+ */
> > + if (de_iir & (DE_PLANE_FLIP_DONE_IVB(i))) {
> ^ ^
>
> Just a minor complaint about the useless parens.
Fixed and both merged, thanks for the review.
-Daniel
>
> But otherwise both patches look good, so:
> Reviewed-by: Ville Syrjälä
>
> > intel_prepare_page_flip(dev, i);
> > intel_finish_page_flip_plane(dev, i);
> > }
> > @@ -2012,7 +2004,7 @@ static int ironlake_enable_vblank(struct drm_device
> > *dev, int pipe)
> > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > unsigned long irqflags;
> > uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
> > -DE_PIPE_VBLANK_ILK(pipe);
> > +DE_PIPE_VBLANK(pipe);
> >
> > if (!i915_pipe_enabled(dev, pipe))
> > return -EINVAL;
> > @@ -2070,7 +2062,7 @@ static void ironlake_disable_vb
Re: [Intel-gfx] [Draft] Testing Requirements for drm/i915 Patches
On Wed, Oct 30, 2013 at 12:38 AM, Jesse Barnes wrote: > Since a number of people internally are also involved in i915 > development but not on the mailing list, I think we'll need to have an > internal meeting or two to cover this stuff and get buy in. Yeah I'll do that. I simply didn't get around to spaming internal mailing lists with this yet yesterday evening. > Overall, developing tests along with code is a good goal. A few > comments below. > > On Tue, 29 Oct 2013 20:00:49 +0100 > Daniel Vetter wrote: > >> - Tests must fully cover userspace interfaces. By this I mean exercising all >> the > [snip] >> - Tests need to provide a reasonable baseline coverage of the internal driver >> state. The idea here isn't to aim for full coverage, that's an impossible >> and > [snip] > > What you've described here is basically full validation. Something > that most groups at Intel have large teams to dedicate all their time > to. I'm not sure how far we can go down this path with just the > development resources we have today (though maybe we'll get some help > from the validation teams in the product groups) I guess you can call it validation, I'd go with test driven development instead. Also my main focus is on userspace interfaces (due to the security relevance and that we need to keep them working forever) and bugs that review/testing/users caught. The general coverage is just to avoid people getting royally pissed off since starting to write tests once everything is essentially ready for merging (like we've done with ppgtt) is just too late. >> Finally the short lists of excuses that don't count as proper test coverage >> for >> a feature. >> >> - Manual testing. We are ridiculously limited on our QA manpower. Every time >> we >> drop something onto the "manual testing" plate something else _will_ drop >> off. >> Which means in the end that we don't really have any test coverage. So >> if patches don't come with automated tests, in-kernel cross-checking or >> some other form of validation attached they need to have really good >> reasons for doing so. > > Some things are only testable manually at this point, since we don't > have a sophisticated webcam structure set up for everything (and in > fact, the webcam tests we do have are fairly manual at this point, in > that they have to be set up specially each time). I agree that not everything is testable in an automated way. And I also think that anything which requires special setup like webcams isn't really useful, since it's a pain to replicate and for developers it's easer to just look at the screen. But if you look at the past 2 years of i-g-t progress we've come up with some neat tricks: - fake gpu hang injection. Before that our reset code was essentially untested, after that we've spent 1+ years thus far fixing bugs. Nowadays gpu hang handling actually works, which is great for developers and also improves the user experience a lot. - exercising gem slowpath: We've added tricks like using gtt mmaps to precisely provoke pagefaults and then added prefault disabling and Chris' drop_caches to debugfs to exercise more cornercases. Nowadays i-g-t exercise the full slowpath-in-slowpath madness in execbuf. - CRC checksums: It's still very early, but Ville has already pushed a testcase for all the cursor bugs he's fixed in the past (and discovered yet another one on top). - fifo underrun reporting: Unfortunately not yet enabled by default (since our watermark code is still buggy), but it seems like Ville and Paulo have found this to be a really powerful tool for catching watermark bugs. - modeset state checker in the kernel: I don't think we could have pushed through all the big reworks we've done in the past year in our modeset code without this. There's more, and I already have crazy ideas for more infrastructure to make more bugs and features testable (like all the ugly 3 pipe resource sharing issues we have on ivb/hsw). Essentially I just want people to spend a bit of their brain power on coming up with new ideas for stuff that's currently untestable. That doesn't mean that it's a hard requirement since very often all the ideas just suck (or are way too much effort to be able to inject the required state and then check it). But occasionally something really good will come out of this. >> - Testing by product teams. The entire point of Intel OTC's "upstream first" >> strategy is to have a common codebase for everyone. If we break product >> trees >> every time we feed an update into them because we can't properly regression >> test a given feature then the value of upstreaming features is greatly >> diminished in my opinion and could potentially doom collaborations with >> product teams. We just can't have that. >> >> This means that when products teams submit patches upstream they also need >> to submit the relevant testcases to i-g-t. > > So what I'm hearing here is that even if someone submits a tested > patch, wit
[Intel-gfx] [PATCH 1/2] drm/i915: add i915_reset_count
reset_counter will be incremented twice per successful
reset. Odd values mean reset is in progress and even values
mean that reset has completed.
Reset status ioctl introduced in following commit
needs to deliver global reset count to userspace so
use reset_counter to derive the actual reset count
for the gpu
Note that reset in progress is enough to increment
the counter.
v2: wedged equals reset in progress (Daniel Vetter)
Signed-off-by: Mika Kuoppala
---
drivers/gpu/drm/i915/i915_drv.h | 11 ---
drivers/gpu/drm/i915/i915_irq.c |2 +-
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0a886fd..9fd716d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1082,7 +1082,7 @@ struct i915_gpu_error {
* being true.
*/
#define I915_RESET_IN_PROGRESS_FLAG1
-#define I915_WEDGED0x
+#define I915_WEDGED(1 << 31)
/**
* Waitqueue to signal when the reset has completed. Used by clients
@@ -2031,12 +2031,17 @@ int __must_check i915_gem_check_wedge(struct
i915_gpu_error *error,
static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
{
return unlikely(atomic_read(&error->reset_counter)
- & I915_RESET_IN_PROGRESS_FLAG);
+ & (I915_RESET_IN_PROGRESS_FLAG | I915_WEDGED));
}
static inline bool i915_terminally_wedged(struct i915_gpu_error *error)
{
- return atomic_read(&error->reset_counter) == I915_WEDGED;
+ return atomic_read(&error->reset_counter) & I915_WEDGED;
+}
+
+static inline u32 i915_reset_count(struct i915_gpu_error *error)
+{
+ return ((atomic_read(&error->reset_counter) & ~I915_WEDGED) + 1) / 2;
}
void i915_gem_reset(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a228176..80800d2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1783,7 +1783,7 @@ static void i915_error_work_func(struct work_struct *work)
kobject_uevent_env(&dev->primary->kdev.kobj,
KOBJ_CHANGE, reset_done_event);
} else {
- atomic_set(&error->reset_counter, I915_WEDGED);
+ atomic_set_mask(I915_WEDGED, &error->reset_counter);
}
/*
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: add i915_get_reset_stats_ioctl
This ioctl returns reset stats for specified context.
The struct returned contains context loss counters.
reset_count:all resets across all contexts
batch_active: active batches lost on resets
batch_pending: pending batches lost on resets
v2: get rid of state tracking completely and deliver only counts. Idea
from Chris Wilson.
v3: fix commit message
v4: default context handled inside i915_gem_context_get_hang_stats
v5: reset_count only for priviledged process
v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
v7: context hang stats never returns NULL
v8: rebased on top of reworked context hang stats
DRM_RENDER_ALLOW for ioctl
v9: use DEFAULT_CONTEXT_ID. Improve comments for ioctl struct members
Signed-off-by: Mika Kuoppala
Cc: Ian Romanick
Cc: Chris Wilson
Cc: Daniel Vetter
---
drivers/gpu/drm/i915/i915_dma.c |1 +
drivers/gpu/drm/i915/i915_drv.h |2 ++
drivers/gpu/drm/i915/intel_uncore.c | 34 ++
include/uapi/drm/i915_drm.h | 19 +++
4 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 6eecce7..f2cdeb2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1921,6 +1921,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE,
i915_gem_context_create_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY,
i915_gem_context_destroy_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl,
DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl,
DRM_UNLOCKED|DRM_RENDER_ALLOW),
};
int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fd716d..8870804 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2369,6 +2369,8 @@ extern int intel_enable_rc6(const struct drm_device *dev);
extern bool i915_semaphore_is_enabled(struct drm_device *dev);
int i915_reg_read_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
+int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file);
/* overlay */
extern struct intel_overlay_error_state
*intel_overlay_capture_error_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index f6fae35..21cf951 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -633,6 +633,40 @@ int i915_reg_read_ioctl(struct drm_device *dev,
return 0;
}
+int i915_get_reset_stats_ioctl(struct drm_device *dev,
+ void *data, struct drm_file *file)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_reset_stats *args = data;
+ struct i915_ctx_hang_stats *hs;
+ int ret;
+
+ if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ ret = mutex_lock_interruptible(&dev->struct_mutex);
+ if (ret)
+ return ret;
+
+ hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
+ if (IS_ERR(hs)) {
+ mutex_unlock(&dev->struct_mutex);
+ return PTR_ERR(hs);
+ }
+
+ if (capable(CAP_SYS_ADMIN))
+ args->reset_count = i915_reset_count(&dev_priv->gpu_error);
+ else
+ args->reset_count = 0;
+
+ args->batch_active = hs->batch_active;
+ args->batch_pending = hs->batch_pending;
+
+ mutex_unlock(&dev->struct_mutex);
+
+ return 0;
+}
+
static int i965_reset_complete(struct drm_device *dev)
{
u8 gdrst;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 3a4e97b..52aed89 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -222,6 +222,7 @@ typedef struct _drm_i915_sarea {
#define DRM_I915_GEM_SET_CACHING 0x2f
#define DRM_I915_GEM_GET_CACHING 0x30
#define DRM_I915_REG_READ 0x31
+#define DRM_I915_GET_RESET_STATS 0x32
#define DRM_IOCTL_I915_INITDRM_IOW( DRM_COMMAND_BASE +
DRM_I915_INIT, drm_i915_init_t)
#define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE +
DRM_I915_FLUSH)
@@ -271,6 +272,7 @@ typedef struct _drm_i915_sarea {
#define DRM_IOCTL_I915_GEM_CONTEXT_CREATE DRM_IOWR (DRM_COMMAND_BASE +
DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
#define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY DRM_IOW (DRM_COMMAND_BASE +
DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
#define DRM_IOCTL_I915_REG_READDRM_IOWR
(DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_rea
[Intel-gfx] [RFC] drm/i915: Clean up display pipe register accesses
Upcoming hardware will not have the various display pipe register
ranges evenly spaced in memory. Change register address calculations
into array lookups.
Tested on SandyBridge.
I left the UMS cruft untouched.
Signed-off-by: Antti Koskipaa
---
drivers/gpu/drm/i915/dvo_ns2501.c | 6 ++--
drivers/gpu/drm/i915/i915_dma.c | 16 +++
drivers/gpu/drm/i915/i915_drv.h | 10 ++-
drivers/gpu/drm/i915/i915_reg.h | 59 +++
4 files changed, 69 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c
b/drivers/gpu/drm/i915/dvo_ns2501.c
index c4a255b..ce6a11b 100644
--- a/drivers/gpu/drm/i915/dvo_ns2501.c
+++ b/drivers/gpu/drm/i915/dvo_ns2501.c
@@ -99,12 +99,12 @@ static void enable_dvo(struct intel_dvo_device *dvo)
DRM_DEBUG_KMS("%s: Trying to re-enable the DVO\n", __FUNCTION__);
ns->dvoc = I915_READ(DVO_C);
- ns->pll_a = I915_READ(_DPLL_A);
+ ns->pll_a = I915_READ(DPLL(0));
ns->srcdim = I915_READ(DVOC_SRCDIM);
ns->fw_blc = I915_READ(FW_BLC);
I915_WRITE(DVOC, 0x10004084);
- I915_WRITE(_DPLL_A, 0xd082);
+ I915_WRITE(DPLL(0), 0xd082);
I915_WRITE(DVOC_SRCDIM, 0x400300); // 1024x768
I915_WRITE(FW_BLC, 0x1080304);
@@ -125,7 +125,7 @@ static void restore_dvo(struct intel_dvo_device *dvo)
struct ns2501_priv *ns = (struct ns2501_priv *)(dvo->dev_priv);
I915_WRITE(DVOC, ns->dvoc);
- I915_WRITE(_DPLL_A, ns->pll_a);
+ I915_WRITE(DPLL(0), ns->pll_a);
I915_WRITE(DVOC_SRCDIM, ns->srcdim);
I915_WRITE(FW_BLC, ns->fw_blc);
}
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0cab2d0..64fbfbb1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1465,6 +1465,7 @@ int i915_driver_load(struct drm_device *dev, unsigned
long flags)
struct drm_i915_private *dev_priv;
struct intel_device_info *info;
int ret = 0, mmio_bar, mmio_size;
+ u32 mmio;
uint32_t aperture_size;
info = (struct intel_device_info *) flags;
@@ -1484,6 +1485,21 @@ int i915_driver_load(struct drm_device *dev, unsigned
long flags)
dev_priv->dev = dev;
dev_priv->info = info;
+ /* Set addresses for various pipe registers */
+ mmio = dev_priv->info->display_mmio_offset;
+ dev_priv->pipe_offsets[0] = mmio + PIPE_A_OFFSET;
+ dev_priv->pipe_offsets[1] = mmio + PIPE_B_OFFSET;
+ dev_priv->trans_offsets[0] = mmio + TRANSCODER_A_OFFSET;
+ dev_priv->trans_offsets[1] = mmio + TRANSCODER_B_OFFSET;
+ dev_priv->trans_offsets[2] = mmio + TRANSCODER_C_OFFSET;
+ dev_priv->trans_offsets[3] = mmio + TRANSCODER_EDP_OFFSET;
+ dev_priv->dpll_offsets[0] = mmio + DPLL_A_OFFSET;
+ dev_priv->dpll_offsets[1] = mmio + DPLL_B_OFFSET;
+ dev_priv->dpll_md_offsets[0] = mmio + DPLL_A_MD_OFFSET;
+ dev_priv->dpll_md_offsets[1] = mmio + DPLL_B_MD_OFFSET;
+ dev_priv->palette_offsets[0] = mmio + PALETTE_A_OFFSET;
+ dev_priv->palette_offsets[1] = mmio + PALETTE_B_OFFSET;
+
spin_lock_init(&dev_priv->irq_lock);
spin_lock_init(&dev_priv->gpu_error.lock);
spin_lock_init(&dev_priv->backlight.lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c1921d..7ea469a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -65,7 +65,8 @@ enum transcoder {
TRANSCODER_A = 0,
TRANSCODER_B,
TRANSCODER_C,
- TRANSCODER_EDP = 0xF,
+ TRANSCODER_EDP,
+ I915_MAX_TRANSCODERS
};
#define transcoder_name(t) ((t) + 'A')
@@ -1473,6 +1474,13 @@ typedef struct drm_i915_private {
struct i915_package_c8 pc8;
+ /* Register offsets for the various display pipes */
+ int pipe_offsets[I915_MAX_PIPES];
+ int trans_offsets[I915_MAX_TRANSCODERS];
+ int dpll_offsets[I915_MAX_PIPES];
+ int dpll_md_offsets[I915_MAX_PIPES];
+ int palette_offsets[I915_MAX_PIPES];
+
/* Old dri1 support infrastructure, beware the dragons ya fools entering
* here! */
struct i915_dri1_state dri1;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4d2db59..0afc9fa 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1187,6 +1187,10 @@
* Clock control & power management
*/
+#define DPLL_A_OFFSET 0x6014
+#define DPLL_B_OFFSET 0x6018
+#define DPLL(pipe) (dev_priv->dpll_offsets[pipe])
+
#define VGA0 0x6000
#define VGA1 0x6004
#define VGA_PD 0x6010
@@ -1200,7 +1204,6 @@
#define VGA1_PD_P1_MASK (0x1f << 8)
#define _DPLL_A(dev_priv->info->display_mmio_offset + 0x6014)
#define _DPLL_B(dev_priv->info->display_mmio_offset + 0x6018)
-#define DPLL(pipe) _PIPE(pipe, _DPLL_A, _DPLL_B)
#define DPLL_VCO_ENABLE (1 << 31)
#define
[Intel-gfx] [PATCH 1/1] tests: add gem_reset_stats
Signed-off-by: Mika Kuoppala
---
tests/Makefile.am |1 +
tests/gem_reset_stats.c | 716 +++
2 files changed, 717 insertions(+)
create mode 100644 tests/gem_reset_stats.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 4ba1d43..e3bf835 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -182,6 +182,7 @@ HANG = \
gem_bad_blit \
gem_bad_address \
gem_non_secure_batch \
+ gem_reset_stats \
$(NULL)
scripts = \
diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c
new file mode 100644
index 000..f87e246
--- /dev/null
+++ b/tests/gem_reset_stats.c
@@ -0,0 +1,716 @@
+/*
+ * Copyright (c) 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Mika Kuoppala
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "i915_drm.h"
+#include "intel_bufmgr.h"
+#include "intel_batchbuffer.h"
+#include "intel_gpu_tools.h"
+#include "rendercopy.h"
+
+#define RS_NO_ERROR 0
+#define RS_BATCH_ACTIVE (1 << 0)
+#define RS_BATCH_PENDING (1 << 1)
+#define RS_UNKNOWN (1 << 2)
+
+struct local_drm_i915_reset_stats {
+ __u32 ctx_id;
+ __u32 flags;
+ __u32 reset_count;
+ __u32 batch_active;
+ __u32 batch_pending;
+ __u32 pad;
+};
+
+struct local_drm_i915_gem_context_create {
+ __u32 ctx_id;
+ __u32 pad;
+};
+
+struct local_drm_i915_gem_context_destroy {
+ __u32 ctx_id;
+ __u32 pad;
+};
+
+#define MAX_FD 32
+
+#define CONTEXT_CREATE_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2d, struct
local_drm_i915_gem_context_create)
+#define CONTEXT_DESTROY_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2e, struct
local_drm_i915_gem_context_destroy)
+#define GET_RESET_STATS_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x32, struct
local_drm_i915_reset_stats)
+
+static uint32_t context_create(int fd)
+{
+ struct local_drm_i915_gem_context_create create;
+ int ret;
+
+ create.ctx_id = rand();
+ create.pad = rand();
+
+ ret = drmIoctl(fd, CONTEXT_CREATE_IOCTL, &create);
+ igt_assert(ret == 0);
+
+ return create.ctx_id;
+}
+
+static int context_destroy(int fd, uint32_t ctx_id)
+{
+ int ret;
+ struct local_drm_i915_gem_context_destroy destroy;
+
+ destroy.ctx_id = ctx_id;
+ destroy.pad = rand();
+
+ ret = drmIoctl(fd, CONTEXT_DESTROY_IOCTL, &destroy);
+ if (ret != 0)
+ return -errno;
+
+ return 0;
+}
+
+static int gem_reset_stats(int fd, int ctx_id,
+ struct local_drm_i915_reset_stats *rs)
+{
+ int ret;
+
+ rs->ctx_id = ctx_id;
+ rs->flags = rand();
+ rs->reset_count = rand();
+ rs->batch_active = rand();
+ rs->batch_pending = rand();
+ rs->pad = rand();
+
+ do {
+ ret = ioctl(fd, GET_RESET_STATS_IOCTL, rs);
+ } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
+
+ if (ret < 0)
+ return -errno;
+
+ return 0;
+}
+
+static int gem_reset_status(int fd, int ctx_id)
+{
+ int ret;
+ struct local_drm_i915_reset_stats rs;
+
+ ret = gem_reset_stats(fd, ctx_id, &rs);
+ if (ret)
+ return ret;
+
+ if (rs.batch_active)
+ return RS_BATCH_ACTIVE;
+ if (rs.batch_pending)
+ return RS_BATCH_PENDING;
+
+ return RS_NO_ERROR;
+}
+
+static int gem_exec(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
+{
+ int ret;
+
+ ret = ioctl(fd,
+ DRM_IOCTL_I915_GEM_EXECBUFFER2,
+ execbuf);
+
+ if (ret < 0)
+ return -errno;
+
+ return 0;
+}
+
+static int exec_valid(int fd, int ctx)
+{
+ struct drm_i915_gem_execbuffer2 execbuf;
+ struct drm_i915_ge
[Intel-gfx] [RFC/Draft] Testing requirements for upstream drm/i915 patches
[This is cross-posted to the public intel-gfx mailing list at http://lists.freedesktop.org/archives/intel-gfx/2013-October/035268.html I'll also present a quick overview of this at Gavin's kernel PDT next week.] Hi all, So in the past half year we've had tons of sometimes rather heated discussions about getting patches merged. Often these discussions have been in the context of specific patch series, which meant that people are already invested. Which contributed to the boiling emotions. I'd like to avoid that here by making this a free-standing discussion. There's a bunch of smaller process tuning going on, but the big thing I'd like to instate henceforth is that automated test coverage is a primary consideration for anything going upstream. In this write up I'll detail my reasons, considerations and expectations. My plan is to solicit feedback over the next few days and then publish an edited and polished version to my blog. After that I'll put down my foot on this process so that we can go back to coding and stop blowing through so much time and energy on waging flamewars. Feedback and critique highly welcome. Cheers, Daniel Testing Requirements for Upstreaming (Draft) I want to make automated test coverage an integral part of our feature and bufix development process. For features this means that starting with the design phase testability needs to be considered an integral part of any feature. This needs to go up through the entire development process until when the implementation is submitted together with the proposed tests. For bugfixes that means the fix is only complete once the automated testcase for it is also done, if we need a new one. This specifically excludes testing with humans somewhere in the loop. We are extremely limited in our validation resources, every time we put something new onto the "manual testing" plate something else _will_ fall off. Why? - More predictability. Right now test coverage often only comes up as a topic when I drop my maintainer review onto a patch series. Which is too late, since it'll delay the otherwise working patches and so massively frustrates people. I hope by making test requirements clear and up-front we can make the upstreaming process more predictable. Also, if we have good tests from the get-go there should be much less need for me to drop patches from my trees after having them merged. - Less bikeshedding. In my opinion test cases are an excellent means to settle bikesheds - we've had in the past seen cases of endless back&forths where writing a simple testcase would have shown that _all_ proposed color flavours are actually broken. The even more important thing is that fully automated tests allow us to legitimately postpone cleanups. If the only testing we have is manual testing then we have only one shot at a feature tested, namely when the developer tests it. So it better be perfect. But with automated tests we can postpone cleanups with too high risks of regressions until a really clear need is established. And since that need often never materializes we'll save work. - Better review. For me it's often helps a lot to review tests than the actual code in-depth. This is especially true for reviewing userspace interface additions. - Actionable regression reports. Only if we have a fully automated testcase do we have a good chance that QA reports a regression within just a few days. Everything else can easily take weeks (for platforms and features which are explicitly tested) to months (for stuff only users from the community notice). And especially now that much more shipping products depend upon a working i915.ko driver we just can't do this any more. - Better tests. A lot of our code is really hard to test in an automated fashion, and pushing the frontier of what is testable often requires a lot of work. I hope that by making tests an integral part of any feature work and so forcing more people to work on them and think about testing we'll advance the state of the art at a brisker pace. Risks and Buts -- - Bikeshedding on tests. This plan is obviously not too useful if we just replace massive bikeshedding on patches with massive bikeshedding on testcases. But right now we do almost no review on i-g-t patches so the risk is small. Long-term the review requirements for testcases will certainly increase, but as with everything else we simply need to strive for a good balance to strike for just the right amount of review. Also if we really start discussing tests _before_ having written massive patch series we'll do the bikeshedding while there's no real rebase pain. So even if the bikeshedding just shifts we'll benefit I think, especially for really big features. - Technical debt in test coverage. We have a lot of old code which still completely lacks
Re: [Intel-gfx] [Draft] Testing Requirements for drm/i915 Patches
On Tue, 29 Oct 2013 20:00:49 +0100 Daniel Vetter wrote: > Since the point here is to make the actual test requirements known up-front we > need to settle on clear expectations. Since this is the part that actually > matters in practice I'll really welcome close scrutiny and comments here. Another thought on these lines. The expectations come from more than just the test side of things, but also from the design of new features, or how code is refactored for a new platform or to fix a bug. So it may make sense, before starting big work, to propose both the tests required for the feature/refactor/bug fix, as well as the proposed design of the change. That would let us get any potential test & feature bikeshedding out of the way before tons of time is invested in the code, hopefully saving a lot of rage, especially on the big features. Note that this runs contrary to a lot of other subsystems, which are sometimes allergic to up front design thinking, and prefer to churn through dozens of implementations to settle on something. Both approaches have value, and some combination is probably best. Some well thought out discussion up front, followed by testing, review, and adjustment of an actual implementation. Getting back to your points: - tests must cover userspace interfaces - tests need to provide coverage of internal driver state - tests need to be written following review for any bugs caught in review (I don't fully agree here; we can look at this on a case by case basis; e.g. in some cases an additional BUG_ON or state check may be all that's needed) - test infrastructure and scope should increase over time I think we can discuss the above points as part of any new proposed feature or rework. For reworks in particular, we can probably start to address some of the "technical debt" you mentioned, where the bar for a rework is relatively high, requiring additional tests and infrastructure scope. -- Jesse Barnes, 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] [Draft] Testing Requirements for drm/i915 Patches
On Wed, Oct 30, 2013 at 6:30 PM, Jesse Barnes wrote: > On Tue, 29 Oct 2013 20:00:49 +0100 > Daniel Vetter wrote: > >> Since the point here is to make the actual test requirements known up-front >> we >> need to settle on clear expectations. Since this is the part that actually >> matters in practice I'll really welcome close scrutiny and comments here. > > Another thought on these lines. > > The expectations come from more than just the test side of things, but > also from the design of new features, or how code is refactored for a > new platform or to fix a bug. > > So it may make sense, before starting big work, to propose both the > tests required for the feature/refactor/bug fix, as well as the > proposed design of the change. That would let us get any potential > test & feature bikeshedding out of the way before tons of time is > invested in the code, hopefully saving a lot of rage, especially on the > big features. Yeah, that's actually very much the core idea I have. Using ppgtt as an example I think a reasonable test coverage we could have discussed upfront would have been: - make sure all the slowpaths in execbuf are really covered with faulting testcases (maybe even concurrent thrashing ones), iirc we've still had a bunch of gaping holes there - filling in testcases for specific corner cases we've had to fix in the execbuf/gem code in the past (e.g. the gen6 ppgtt w/a) - a generic "let's thrash the crap out of gem" test as a baseline, which will be fleshed out while developing the patches to hit the low-mem corner-cases when vma setup fails midway through an execbuf somewhere. Later on (and this actually happened since I've totally forgotten about it) we'd discover that we need a testcase for the secure dispatch stuff. But since we already had a few nice testcase to throw crazy execbufs at the kernel that was just little work on top. > Note that this runs contrary to a lot of other subsystems, which are > sometimes allergic to up front design thinking, and prefer to churn > through dozens of implementations to settle on something. Both > approaches have value, and some combination is probably best. Some > well thought out discussion up front, followed by testing, review, and > adjustment of an actual implementation. Yeah, I agree that we need more planing than what's the usual approach. > Getting back to your points: > - tests must cover userspace interfaces > - tests need to provide coverage of internal driver state > - tests need to be written following review for any bugs caught in > review (I don't fully agree here; we can look at this on a case by > case basis; e.g. in some cases an additional BUG_ON or state check > may be all that's needed) I guess I need to clarify this: Tests don't need to be special i-g-ts, in-kernel self-checks are also good (and often the best approach). For WARN_ONs we just need try to make them as orthogonal as possible to the code itself (refcount checks are always great at that) to minimize the chances of both the code and the check being broken in the same way. > - test infrastructure and scope should increase over time > > I think we can discuss the above points as part of any new proposed > feature or rework. For reworks in particular, we can probably start to > address some of the "technical debt" you mentioned, where the bar for a > rework is relatively high, requiring additional tests and > infrastructure scope. Yeah I think having a upfront discussion about testing and what should be done for big features/reworks would be really useful. Both to make sure that we have an optimal set of tests (not too much or too little) to get the patches merged as smoothly as possible, but also to evenly distribute plugging test gaps for existing features. Especially when people add new test infrastructure (like EDID injection or pipe CRC support) we should be really lenient about the coverage for new features. Otherwise doing the feature, the test infrastructure _and_ all the tests for the feature is too much. So for completely new test approaches I think it's more than good enough to just deliver that plus a small proof-of-concept testcase - advancing the testable scope itself is extermely valuable. Later on extension work can then start to fill in the gaps. -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: add i915_get_reset_stats_ioctl
On 10/30/2013 06:44 AM, Mika Kuoppala wrote:
> This ioctl returns reset stats for specified context.
>
> The struct returned contains context loss counters.
>
> reset_count:all resets across all contexts
> batch_active: active batches lost on resets
> batch_pending: pending batches lost on resets
>
> v2: get rid of state tracking completely and deliver only counts. Idea
> from Chris Wilson.
>
> v3: fix commit message
>
> v4: default context handled inside i915_gem_context_get_hang_stats
>
> v5: reset_count only for priviledged process
>
> v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
>
> v7: context hang stats never returns NULL
>
> v8: rebased on top of reworked context hang stats
> DRM_RENDER_ALLOW for ioctl
>
> v9: use DEFAULT_CONTEXT_ID. Improve comments for ioctl struct members
>
> Signed-off-by: Mika Kuoppala
> Cc: Ian Romanick
> Cc: Chris Wilson
> Cc: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_dma.c |1 +
> drivers/gpu/drm/i915/i915_drv.h |2 ++
> drivers/gpu/drm/i915/intel_uncore.c | 34 ++
> include/uapi/drm/i915_drm.h | 19 +++
> 4 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 6eecce7..f2cdeb2 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1921,6 +1921,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE,
> i915_gem_context_create_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY,
> i915_gem_context_destroy_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> };
>
> int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9fd716d..8870804 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2369,6 +2369,8 @@ extern int intel_enable_rc6(const struct drm_device
> *dev);
> extern bool i915_semaphore_is_enabled(struct drm_device *dev);
> int i915_reg_read_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> +int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file);
>
> /* overlay */
> extern struct intel_overlay_error_state
> *intel_overlay_capture_error_state(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index f6fae35..21cf951 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -633,6 +633,40 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> return 0;
> }
>
> +int i915_get_reset_stats_ioctl(struct drm_device *dev,
> +void *data, struct drm_file *file)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_reset_stats *args = data;
> + struct i915_ctx_hang_stats *hs;
> + int ret;
> +
> + if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + ret = mutex_lock_interruptible(&dev->struct_mutex);
> + if (ret)
> + return ret;
> +
> + hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
> + if (IS_ERR(hs)) {
> + mutex_unlock(&dev->struct_mutex);
> + return PTR_ERR(hs);
> + }
> +
> + if (capable(CAP_SYS_ADMIN))
> + args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> + else
> + args->reset_count = 0;
We're having some additional debate about issues related to this. Eric
(added to CC so he'll notice) believes that we may encounter memory
corruption around a reset (most likely causing the reset instead of the
other way around). This means that we may need to deliver a reset
notification to an otherwise unaffected GL context after all. :(
If we decided that this is possible, we should deliver a single bit to
user mode that says "there was a reset after this context was created."
I assume that could be returned to user space in the flags field?
I don't think this provides the same potential information leak as
directly exposing the global reset count, but I could be wrong.
I don't think we need to change anything /yet/, but we may need to soon.
> +
> + args->batch_active = hs->batch_active;
> + args->batch_pending = hs->batch_pending;
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> + return 0;
> +}
> +
> static int i965_reset_complete(struct drm_device *dev)
> {
> u8 gdrst;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 3a
Re: [Intel-gfx] [RFC/Draft] Testing requirements for upstream drm/i915 patches
On 10/30/2013 09:00 AM, Daniel Vetter wrote: > [This is cross-posted to the public intel-gfx mailing list at > http://lists.freedesktop.org/archives/intel-gfx/2013-October/035268.html > I'll also present a quick overview of this at Gavin's kernel PDT next > week.] > > Hi all, > > So in the past half year we've had tons of sometimes rather heated > discussions > about getting patches merged. Often these discussions have been in the > context > of specific patch series, which meant that people are already invested. > Which > contributed to the boiling emotions. I'd like to avoid that here by > making this > a free-standing discussion. > > There's a bunch of smaller process tuning going on, but the big thing > I'd like > to instate henceforth is that automated test coverage is a primary > consideration > for anything going upstream. In this write up I'll detail my reasons, > considerations and expectations. My plan is to solicit feedback over the > next > few days and then publish an edited and polished version to my blog. > > After that I'll put down my foot on this process so that we can go back to > coding and stop blowing through so much time and energy on waging > flamewars. > > Feedback and critique highly welcome. > > Cheers, Daniel > > Testing Requirements for Upstreaming (Draft) > > > I want to make automated test coverage an integral part of our feature > and bufix ^ bugfix > development process. For features this means that starting with the > design phase > testability needs to be considered an integral part of any feature. This > needs > to go up through the entire development process until when the > implementation is > submitted together with the proposed tests. For bugfixes that means the > fix is > only complete once the automated testcase for it is also done, if we > need a new > one. > > This specifically excludes testing with humans somewhere in the loop. We > are > extremely limited in our validation resources, every time we put > something new > onto the "manual testing" plate something else _will_ fall off. > > Why? > > > - More predictability. Right now test coverage often only comes up as a > topic > when I drop my maintainer review onto a patch series. Which is too > late, since > it'll delay the otherwise working patches and so massively frustrates > people. > I hope by making test requirements clear and up-front we can make the > upstreaming process more predictable. Also, if we have good tests from > the get-go > there should be much less need for me to drop patches from my trees > after having them merged. > > - Less bikeshedding. In my opinion test cases are an excellent means to > settle > bikesheds - we've had in the past seen cases of endless back&forths > where writing a simple testcase would have shown that _all_ proposed > color flavours are actually broken. > > The even more important thing is that fully automated tests allow us to > legitimately postpone cleanups. If the only testing we have is manual > testing > then we have only one shot at a feature tested, namely when the developer > tests it. So it better be perfect. But with automated tests we can > postpone > cleanups with too high risks of regressions until a really clear need is > established. And since that need often never materializes we'll save > work. > > - Better review. For me it's often helps a lot to review tests than the > actual > code in-depth. This is especially true for reviewing userspace interface > additions. > > - Actionable regression reports. Only if we have a fully automated > testcase do > we have a good chance that QA reports a regression within just a few > days. > Everything else can easily take weeks (for platforms and features > which are > explicitly tested) to months (for stuff only users from the community > notice). > And especially now that much more shipping products depend upon a working > i915.ko driver we just can't do this any more. > > - Better tests. A lot of our code is really hard to test in an automated > fashion, and pushing the frontier of what is testable often requires a > lot of > work. I hope that by making tests an integral part of any feature work > and so > forcing more people to work on them and think about testing we'll > advance the state of the art at a brisker pace. > > Risks and Buts > -- > > - Bikeshedding on tests. This plan is obviously not too useful if we just > replace massive bikeshedding on patches with massive bikeshedding on > testcases. But right now we do almost no review on i-g-t patches so > the risk > is small. Long-term the review requirements for testcases will certainly > increase, but as with everything else we simply need to strive for a good > balance to strike for just the right amount of review. > > Also if we really start discussing tests _before_ having written > massive patch >
Re: [Intel-gfx] [RFC/Draft] Testing requirements for upstream drm/i915 patches
On Wed, Oct 30, 2013 at 7:11 PM, Ian Romanick wrote: >> test coverage of the existing code _before_ starting a feature instead >> of when the patches are ready for merging should help a lot, before >> everyone is invested into patches already and mounting rebase pain looms >> large. > > This is actually an opportunity in disguise. Once you have identified > some of the places that are really lacking coverage, you give new people > the task to create tests for some small areas. This gives the new > person a way to learn the code and make a contribution without hurting > themselves. Yeah, that would be a great way to bring up new people. The problem is a bit that on the kernel side we have a few disadvantages compared to mesa: We don't have a nice spec and we also don't have a fairly decent reference implementation (the nvidia blob). So ime writing kernel tests is much more open-ended than reading a gl extension spec and just nocking off all the new enums and api interface points. The other issue is that some of the bugs (especially in gem) are ridiculously hard to reproduce. E.g. a late -rc regression in 3.11 took me a full day of head against the wall banging until I've had a testcase for it. And that was with the bugfix already at hand and the issue seemingly understood. A great learning experience in carefully ordering operations and laying out objects in the gtt, but probably not something to get started. >> - Tests need to provide a reasonable baseline coverage of the internal >> driver >> state. The idea here isn't to aim for full coverage, that's an >> impossible and >> pointless endeavor. The goal is to have a good starting point of tests >> so that >> when a tricky corner case pops up in review or validation that it's not a >> terribly big effort to add a specific testcase for it. > > In Mesa we've started adding tests in Mesa (run with 'make check') in > addition to the piglit tests. These allow us to poke at internal > interfaces in ways that are difficult or impossible to do directly from > a test that lives on the other side of the API. The hope is that this > will help prevent "revealed" bugs. It's those bugs where a change in an > unrelated piece of code exposes a bug that had previously existed. > > Is something like that sensible (or even possible) in the kernel? Actual unit tests are hard to impossible, since the driver (and the kernel in general) is a spaghetti monster with complete lack of mock objects. And often bugs happen at the interface between sw and hw, so we need some way to inject (fake) hw events and state. I think runtime checks otoh are really great, and especially for slow operations like modeset the overhead doesn't matter at all. Everywhere else where treating the kernel as a blackbox isn't good enough we've resorted to exposing internals and special knobs through sysfs. That includes stuff like fake hang injection, cache thrashing or the newly added pipe CRC stuff. But it's all for runtime tests. >> - Issues discovered in review and final validation need automated test >> coverage. >> The reasoning is that anything which slipped the developer's attention is >> tricky enough to warrant an explicit testcase, since in a later >> refactoring >> there's a good chance that it'll be missed again. This has a bit a risk >> to delay patches, but if the basic test coverage is good enough as per >> the previous point it really shouln't be an issue. > > These are the bugs most likely to need the previously mentioned > "internal" tests. "Function X will crash in situation Y. The code that > calls X won't cause Y, but..." Ime the really tricky stuff is concurrency issues.They're hard to understand and often really hard to reproduce. We're trying though. The other ones are bugs depending in complex ways upon the aggregate driver state (especially for modesetting issue). Both are not easily done as unit tests which are executed with make check. There's also a the issue of making the kernel code compile as a userspace executable. The usually way to solve that is to create a loadable kernel module with all the unit tests, and there's definitely a bit of that around already in the kernel (e.g. lockdep has a good set of self-tests that isolate the internal lockdep state for these tests). That approach might be useful for some parts of the i915 driver like testing the edid parser, but those tend to not be high-priority items for testing. -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: dp aux irq support for g4x/vlv
Now we have this everywhere. Next up would be to wire up the DP hotplug pin to speed up panel power sequencing for eDP panels ... I've decided to leave the has_aux_irq logic in the code, it should come handy for hw bringup. For testing/fail-safety the dp aux code already has a timeout when waiting for interrupts to signal completion and screams rather loud if they don't arrive in time. Given that we need a real piece of hw to talk to anyway this is probably as good as it gets. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_irq.c | 6 ++ drivers/gpu/drm/i915/i915_reg.h | 4 drivers/gpu/drm/i915/intel_dp.c | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 2a44816..7c075a2 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1375,6 +1375,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_i915); + if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X) + dp_aux_irq_handler(dev); + I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status); I915_READ(PORT_HOTPLUG_STAT); } @@ -3256,6 +3259,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg) intel_hpd_irq_handler(dev, hotplug_trigger, IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i915); + if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X) + dp_aux_irq_handler(dev); + I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status); I915_READ(PORT_HOTPLUG_STAT); } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 4d2db59..447fd83 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2116,6 +2116,10 @@ #define CRT_HOTPLUG_MONITOR_COLOR(3 << 8) #define CRT_HOTPLUG_MONITOR_MONO (2 << 8) #define CRT_HOTPLUG_MONITOR_NONE (0 << 8) +#define DP_AUX_CHANNEL_D_INT_STATUS_G4X (1 << 6) +#define DP_AUX_CHANNEL_C_INT_STATUS_G4X (1 << 5) +#define DP_AUX_CHANNEL_B_INT_STATUS_G4X (1 << 4) +#define DP_AUX_CHANNEL_MASK_INT_STATUS_G4X (1 << 4) /* SDVO is different across gen3/4 */ #define SDVOC_HOTPLUG_INT_STATUS_G4X (1 << 3) #define SDVOB_HOTPLUG_INT_STATUS_G4X (1 << 2) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b3cc333..7fa4518 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -404,7 +404,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, int i, ret, recv_bytes; uint32_t status; int try, precharge, clock = 0; - bool has_aux_irq = INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev); + bool has_aux_irq = true; /* dp aux is extremely sensitive to irq latency, hence request the * lowest possible wakeup latency and so prevent the cpu from going into -- 1.8.4.rc3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: reuse WRPLL when possible
From: Paulo Zanoni
It seems we do have machines with 3 HDMI/DVI outputs, so sharing
WRPLLs is the only way to get 3 pipes working.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68485
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_ddi.c | 37 ++---
1 file changed, 22 insertions(+), 15 deletions(-)
It is that easy because I already planned to enable PLL sharing when I wrote the
original code :)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 31f4fe2..f2144b2 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -636,8 +636,6 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
uint32_t reg, val;
int clock = intel_crtc->config.port_clock;
- /* TODO: reuse PLLs when possible (compare values) */
-
intel_ddi_put_crtc_pll(crtc);
if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
@@ -665,31 +663,40 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
} else if (type == INTEL_OUTPUT_HDMI) {
unsigned p, n2, r2;
- if (plls->wrpll1_refcount == 0) {
+ intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p);
+
+ val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 |
+ WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) |
+ WRPLL_DIVIDER_POST(p);
+
+ if (val == I915_READ(WRPLL_CTL1)) {
+ DRM_DEBUG_KMS("Reusing WRPLL 1 on pipe %c\n",
+ pipe_name(pipe));
+ reg = WRPLL_CTL1;
+ } else if (val == I915_READ(WRPLL_CTL2)) {
+ DRM_DEBUG_KMS("Reusing WRPLL 2 on pipe %c\n",
+ pipe_name(pipe));
+ reg = WRPLL_CTL2;
+ } else if (plls->wrpll1_refcount == 0) {
DRM_DEBUG_KMS("Using WRPLL 1 on pipe %c\n",
pipe_name(pipe));
- plls->wrpll1_refcount++;
reg = WRPLL_CTL1;
- intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1;
} else if (plls->wrpll2_refcount == 0) {
DRM_DEBUG_KMS("Using WRPLL 2 on pipe %c\n",
pipe_name(pipe));
- plls->wrpll2_refcount++;
reg = WRPLL_CTL2;
- intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL2;
} else {
DRM_ERROR("No WRPLLs available!\n");
return false;
}
- WARN(I915_READ(reg) & WRPLL_PLL_ENABLE,
-"WRPLL already enabled\n");
-
- intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p);
-
- val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 |
- WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) |
- WRPLL_DIVIDER_POST(p);
+ if (reg == WRPLL_CTL1) {
+ plls->wrpll1_refcount++;
+ intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1;
+ } else {
+ plls->wrpll2_refcount++;
+ intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL2;
+ }
} else if (type == INTEL_OUTPUT_ANALOG) {
if (plls->spll_refcount == 0) {
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces
From: Paulo Zanoni
The first piece, intel_ddi_pll_select, finds a PLL and assigns it to
the CRTC, but doesn't write any register. It can also fail in case it
doesn't find a PLL.
The second piece, intel_ddi_pll_enable, uses the information stored by
intel_ddi_pll_select to actually enable the PLL by writing to its
register. This function can't fail. We also have some refcount sanity
checks here.
The idea is that one day we'll remove all the functions that touch
registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll
call intel_ddi_pll_select at haswell_crtc_mode_set and then call
intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already
touching this code, let's take care of this particular split today.
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_ddi.c | 101 ---
drivers/gpu/drm/i915/intel_display.c | 3 +-
drivers/gpu/drm/i915/intel_drv.h | 3 +-
3 files changed, 87 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f2144b2..9d9ed18 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -619,21 +619,21 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
*n2_out = best.n2;
*p_out = best.p;
*r2_out = best.r2;
-
- DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n",
- clock, *p_out, *n2_out, *r2_out);
}
-bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
+/* Tries to find a PLL for the CRTC. If it finds, it increases the refcount and
+ * stores it in intel_crtc->ddi_pll_sel, so other mode sets won't be able to
+ * steal the selected PLL. You need to call intel_ddi_pll_enable to actually
+ * enable the PLL. */
+bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct drm_crtc *crtc = &intel_crtc->base;
struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
struct drm_encoder *encoder = &intel_encoder->base;
struct drm_i915_private *dev_priv = crtc->dev->dev_private;
struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
int type = intel_encoder->type;
enum pipe pipe = intel_crtc->pipe;
- uint32_t reg, val;
int clock = intel_crtc->config.port_clock;
intel_ddi_put_crtc_pll(crtc);
@@ -657,10 +657,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
return false;
}
- /* We don't need to turn any PLL on because we'll use LCPLL. */
- return true;
-
} else if (type == INTEL_OUTPUT_HDMI) {
+ uint32_t reg, val;
unsigned p, n2, r2;
intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p);
@@ -690,6 +688,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
return false;
}
+ DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d
r2=%d\n",
+ clock, p, n2, r2);
+
if (reg == WRPLL_CTL1) {
plls->wrpll1_refcount++;
intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1;
@@ -703,29 +704,93 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
DRM_DEBUG_KMS("Using SPLL on pipe %c\n",
pipe_name(pipe));
plls->spll_refcount++;
- reg = SPLL_CTL;
intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
} else {
DRM_ERROR("SPLL already in use\n");
return false;
}
- WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
-"SPLL already enabled\n");
-
- val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
-
} else {
WARN(1, "Invalid DDI encoder type %d\n", type);
return false;
}
- I915_WRITE(reg, val);
- udelay(20);
-
return true;
}
+/* To be called after intel_ddi_pll_select(). That one selects the PLL to be
+ * used, this one actually enables the PLL. */
+void intel_ddi_pll_enable(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
+ int clock = crtc->config.port_clock;
+ uint32_t reg, cur_val, new_val;
+ int refcount;
+ const char *pll_name;
+ uint32_t enable_bit = (1 << 31);
+ unsigned int p, n2, r2;
+
+ BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE);
+ BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE);
+
+ switch (crtc->ddi_pll_sel) {
+ case PORT_CLK_SEL_LCPLL_2700:
+ case PORT_CLK_SEL_LCPLL_1350:
+ case PORT_CLK_SEL_LCPLL_810:
+
Re: [Intel-gfx] [RFC/Draft] Testing requirements for upstream drm/i915 patches
On 10/30/2013 12:05 PM, Daniel Vetter wrote: On Wed, Oct 30, 2013 at 7:11 PM, Ian Romanick wrote: test coverage of the existing code _before_ starting a feature instead of when the patches are ready for merging should help a lot, before everyone is invested into patches already and mounting rebase pain looms large. Yeah, that would be a great way to bring up new people. The problem is a bit that on the kernel side we have a few disadvantages compared to mesa: We don't have a nice spec and we also don't have a fairly decent reference implementation (the nvidia blob). So ime writing kernel tests is much more open-ended than reading a gl extension spec and just nocking off all the new enums and api interface points. Writing *meaningful* GL tests is much more open-ended than reading a gl spec and knocking off each item. To really test some GL features, you must be exceedingly clever and even have an understanding of the underlying hardware implementation to test the significant corner cases. In that sense, it's not too different from writing a kernel test case. My comment is intended to be positive rather than a negative correction. The Mesa team frequently succeeds at creating good test coverage of new GL features despite the difficulty. That fact hopefully confirms it will be possible for the kernel team too. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/4] PC8 fixes
From: Paulo Zanoni Hi Patch 1 was sent by Chris a few weeks ago, and patch 2 is my bikeshed to his original patch. They shouldn't fix any particular bug, and patch 2 will WARN in case we hit the bug that should not happen. Patches 3 and 4 fix problems that I discovered while working on the runtime D3. Since these bugs affect PC8 even without D3, I think they should be merged before D3. I already have subtests for these problems on the pc8 test, so we should be fine. Thanks, Paulo Chris Wilson (1): drm/i915: Do not enable package C8 on unsupported hardware Paulo Zanoni (3): drm/i915: WARN if !HAS_PC8 when enabling/disabling PC8 drm/i915: get a PC8 reference when enabling the power well drm/i915: get/put PC8 when we get/put a CRTC drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_display.c | 48 +--- drivers/gpu/drm/i915/intel_pm.c | 14 +-- 3 files changed, 57 insertions(+), 6 deletions(-) -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Do not enable package C8 on unsupported hardware
From: Chris Wilson
If the hardware does not support package C8, then do not even schedule
work to enable it. Thereby we can eliminate a bunch of dangerous work.
Signed-off-by: Chris Wilson
Cc: Paulo Zanoni
Reviewed-by: Paulo Zanoni
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 15 +++
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c1921d..a28a676 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1790,6 +1790,7 @@ struct drm_i915_file_private {
#define HAS_POWER_WELL(dev)(IS_HASWELL(dev))
#define HAS_FPGA_DBG_UNCLAIMED(dev)(INTEL_INFO(dev)->has_fpga_dbg)
#define HAS_PSR(dev) (IS_HASWELL(dev))
+#define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */
#define INTEL_PCH_DEVICE_ID_MASK 0xff00
#define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index f0b42bc..14327ff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6473,6 +6473,9 @@ static void __hsw_disable_package_c8(struct
drm_i915_private *dev_priv)
void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
{
+ if (!HAS_PC8(dev_priv->dev))
+ return;
+
mutex_lock(&dev_priv->pc8.lock);
__hsw_enable_package_c8(dev_priv);
mutex_unlock(&dev_priv->pc8.lock);
@@ -6480,6 +6483,9 @@ void hsw_enable_package_c8(struct drm_i915_private
*dev_priv)
void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
{
+ if (!HAS_PC8(dev_priv->dev))
+ return;
+
mutex_lock(&dev_priv->pc8.lock);
__hsw_disable_package_c8(dev_priv);
mutex_unlock(&dev_priv->pc8.lock);
@@ -6517,6 +6523,9 @@ static void hsw_update_package_c8(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev->dev_private;
bool allow;
+ if (!HAS_PC8(dev_priv->dev))
+ return;
+
if (!i915_enable_pc8)
return;
@@ -6540,6 +6549,9 @@ done:
static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
{
+ if (!HAS_PC8(dev_priv->dev))
+ return;
+
if (!dev_priv->pc8.gpu_idle) {
dev_priv->pc8.gpu_idle = true;
hsw_enable_package_c8(dev_priv);
@@ -6548,6 +6560,9 @@ static void hsw_package_c8_gpu_idle(struct
drm_i915_private *dev_priv)
static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
{
+ if (!HAS_PC8(dev_priv->dev))
+ return;
+
if (dev_priv->pc8.gpu_idle) {
dev_priv->pc8.gpu_idle = false;
hsw_disable_package_c8(dev_priv);
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: WARN if !HAS_PC8 when enabling/disabling PC8
From: Paulo Zanoni We already have some checks and shouldn't be reaching these places on !HAS_PC8 platforms, but add a WARN, just in case. Signed-off-by: Paulo Zanoni --- drivers/gpu/drm/i915/intel_display.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 14327ff..c226f4d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6402,6 +6402,8 @@ void hsw_enable_pc8_work(struct work_struct *__work) struct drm_device *dev = dev_priv->dev; uint32_t val; + WARN_ON(!HAS_PC8(dev)); + if (dev_priv->pc8.enabled) return; @@ -6447,6 +6449,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) if (dev_priv->pc8.disable_count != 1) return; + WARN_ON(!HAS_PC8(dev)); + cancel_delayed_work_sync(&dev_priv->pc8.enable_work); if (!dev_priv->pc8.enabled) return; -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] drm/i915: get/put PC8 when we get/put a CRTC
From: Paulo Zanoni
Currently, PC8 is enabled at modeset_global_resources, which is called
after intel_modeset_update_state. Due to this, there's a small race
condition on the case where we start enabling PC8, then do a modeset
while PC8 is still being enabled. The racing condition triggers a WARN
because intel_modeset_update_state will mark the CRTC as enabled, then
the thread that's still enabling PC8 might look at the data structure
and think that PC8 is being enabled while a pipe is enabled. Despite
the WARN, this is not really a bug since we'll wait for the
PC8-enabling thread to finish when we call modeset_global_resources.
So this patch makes sure we get/put PC8 before we update
drm_crtc->enabled, because this will grab the PC8 lock, which will
wait for the PC8-enable task to finish.
The side-effect benefit of this patch is that we have a nice place to
track enabled/disabled CRTCs, so we may want to move some code from
modeset_global_resources to intel_crtc_set_state in the future.
The problem fixed by this patch can be reproduced by the
modeset-lpsp-stress-no-wait subtest from the pc8 test of
intel-gpu-tools.
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_display.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index c226f4d..e841cd7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6409,6 +6409,7 @@ void hsw_enable_pc8_work(struct work_struct *__work)
DRM_DEBUG_KMS("Enabling package C8+\n");
+ mutex_lock(&dev_priv->pc8.lock);
dev_priv->pc8.enabled = true;
if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
@@ -6420,6 +6421,7 @@ void hsw_enable_pc8_work(struct work_struct *__work)
lpt_disable_clkout_dp(dev);
hsw_pc8_disable_interrupts(dev);
hsw_disable_lcpll(dev_priv, true, true);
+ mutex_unlock(&dev_priv->pc8.lock);
}
static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
@@ -8880,6 +8882,24 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
return false;
}
+/* Sets crtc->base.enabled and gets/puts whatever resources are needed by the
+ * CRTC. */
+static void intel_crtc_set_state(struct intel_crtc *crtc, bool enabled)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ if (enabled == crtc->base.enabled)
+ return;
+
+ if (enabled)
+ hsw_disable_package_c8(dev_priv);
+ else
+ hsw_enable_package_c8(dev_priv);
+
+ crtc->base.enabled = enabled;
+}
+
static void
intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
{
@@ -8903,7 +8923,8 @@ intel_modeset_update_state(struct drm_device *dev,
unsigned prepare_pipes)
/* Update computed state. */
list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
base.head) {
- intel_crtc->base.enabled = intel_crtc_in_use(&intel_crtc->base);
+ intel_crtc_set_state(intel_crtc,
+intel_crtc_in_use(&intel_crtc->base));
}
list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -10695,7 +10716,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
}
WARN_ON(crtc->active);
- crtc->base.enabled = false;
+ intel_crtc_set_state(crtc, false);
}
if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
@@ -10722,7 +10743,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
crtc->base.enabled ? "enabled" : "disabled",
crtc->active ? "enabled" : "disabled");
- crtc->base.enabled = crtc->active;
+ intel_crtc_set_state(crtc, crtc->active);
/* Because we only establish the connector -> encoder ->
* crtc links if something is active, this means the
@@ -10819,7 +10840,7 @@ static void intel_modeset_readout_hw_state(struct
drm_device *dev)
crtc->active = dev_priv->display.get_pipe_config(crtc,
&crtc->config);
- crtc->base.enabled = crtc->active;
+ intel_crtc_set_state(crtc, crtc->active);
crtc->primary_enabled = crtc->active;
DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915: get a PC8 reference when enabling the power well
From: Paulo Zanoni
In the current code, at haswell_modeset_global_resources, first we
decide if we want to enable/disable the power well, then we decide if
we want to enable/disable PC8. On the case where we're enabling PC8
this works fine, but on the case where we disable PC8 due to a non-eDP
monitor being enabled, we first enable the power well and then disable
PC8. Although wrong, this doesn't seem to be causing any problems now,
and we don't even see anything in dmesg. But the patches for runtime
D3 turn this problem into a real bug, so we need to fix it.
This fixes the "modeset-non-lpsp" test from both "pc8" and
"runtime_pm" tests from intel-gpu-tools.
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_pm.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a0c907f..5b50083 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5547,6 +5547,8 @@ static void __intel_set_power_well(struct drm_device
*dev, bool enable)
bool is_enabled, enable_requested;
uint32_t tmp;
+ WARN_ON(dev_priv->pc8.enabled);
+
tmp = I915_READ(HSW_PWR_WELL_DRIVER);
is_enabled = tmp & HSW_PWR_WELL_STATE_ENABLED;
enable_requested = tmp & HSW_PWR_WELL_ENABLE_REQUEST;
@@ -5591,16 +5593,24 @@ static void __intel_set_power_well(struct drm_device
*dev, bool enable)
static void __intel_power_well_get(struct drm_device *dev,
struct i915_power_well *power_well)
{
- if (!power_well->count++)
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ if (!power_well->count++) {
+ hsw_disable_package_c8(dev_priv);
__intel_set_power_well(dev, true);
+ }
}
static void __intel_power_well_put(struct drm_device *dev,
struct i915_power_well *power_well)
{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
WARN_ON(!power_well->count);
- if (!--power_well->count)
+ if (!--power_well->count) {
__intel_set_power_well(dev, false);
+ hsw_enable_package_c8(dev_priv);
+ }
}
void intel_display_power_get(struct drm_device *dev,
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: turn the eDP VDD on for any i2c transactions
From: Paulo Zanoni
If the eDP output is disabled, then we try to use /dev/i2c-X file to
do i2c transations, we get a WARN from intel_dp_check_edp() saying
we're trying to do AUX communication with the panel off. So this
commit reorganizes the code so we enable the VDD at
intel_dp_i2c_aux_ch() instead of just the callers inside i915.ko.
This fixes the i2c subtest from the pc8 test of intel-gpu-tools on
machines that have eDP panels.
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_dp.c | 29 +
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3cc333..05d0424 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -623,6 +623,7 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
int reply_bytes;
int ret;
+ ironlake_edp_panel_vdd_on(intel_dp);
intel_dp_check_edp(intel_dp);
/* Set up the command byte */
if (mode & MODE_I2C_READ)
@@ -665,7 +666,7 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
reply, reply_bytes);
if (ret < 0) {
DRM_DEBUG_KMS("aux_ch failed %d\n", ret);
- return ret;
+ goto out;
}
switch (reply[0] & AUX_NATIVE_REPLY_MASK) {
@@ -676,7 +677,8 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
break;
case AUX_NATIVE_REPLY_NACK:
DRM_DEBUG_KMS("aux_ch native nack\n");
- return -EREMOTEIO;
+ ret = -EREMOTEIO;
+ goto out;
case AUX_NATIVE_REPLY_DEFER:
/*
* For now, just give more slack to branch devices. We
@@ -694,7 +696,8 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
default:
DRM_ERROR("aux_ch invalid native reply 0x%02x\n",
reply[0]);
- return -EREMOTEIO;
+ ret = -EREMOTEIO;
+ goto out;
}
switch (reply[0] & AUX_I2C_REPLY_MASK) {
@@ -702,22 +705,29 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
if (mode == MODE_I2C_READ) {
*read_byte = reply[1];
}
- return reply_bytes - 1;
+ ret = reply_bytes - 1;
+ goto out;
case AUX_I2C_REPLY_NACK:
DRM_DEBUG_KMS("aux_i2c nack\n");
- return -EREMOTEIO;
+ ret = -EREMOTEIO;
+ goto out;
case AUX_I2C_REPLY_DEFER:
DRM_DEBUG_KMS("aux_i2c defer\n");
udelay(100);
break;
default:
DRM_ERROR("aux_i2c invalid reply 0x%02x\n", reply[0]);
- return -EREMOTEIO;
+ ret = -EREMOTEIO;
+ goto out;
}
}
DRM_ERROR("too many retries, giving up\n");
- return -EREMOTEIO;
+ ret = -EREMOTEIO;
+
+out:
+ ironlake_edp_panel_vdd_off(intel_dp, false);
+ return ret;
}
static int
@@ -739,9 +749,7 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
intel_dp->adapter.algo_data = &intel_dp->algo;
intel_dp->adapter.dev.parent = intel_connector->base.kdev;
- ironlake_edp_panel_vdd_on(intel_dp);
ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
- ironlake_edp_panel_vdd_off(intel_dp, false);
return ret;
}
@@ -3498,7 +3506,6 @@ static bool intel_edp_init_connector(struct intel_dp
*intel_dp,
intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
&power_seq);
- ironlake_edp_panel_vdd_on(intel_dp);
edid = drm_get_edid(connector, &intel_dp->adapter);
if (edid) {
if (drm_add_edid_modes(connector, edid)) {
@@ -3530,8 +3537,6 @@ static bool intel_edp_init_connector(struct intel_dp
*intel_dp,
fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
}
- ironlake_edp_panel_vdd_off(intel_dp, false);
-
intel_panel_init(&intel_connector->panel, fixed_mode);
intel_panel_setup_backlight(connector);
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: reduce eDP VDD message verbose
From: Paulo Zanoni
Now we only print messages when we actually enable VDD and when we
actually disable VDD.
The changes in the last commit triggered a big number of messages
while the driver was being initialized, and I thought we were toggling
things on/off too many times, but that was not really true: we were
just being too verbose.
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_dp.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 05d0424..8db1fda 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1077,17 +1077,16 @@ void ironlake_edp_panel_vdd_on(struct intel_dp
*intel_dp)
if (!is_edp(intel_dp))
return;
- DRM_DEBUG_KMS("Turn eDP VDD on\n");
WARN(intel_dp->want_panel_vdd,
"eDP VDD already requested on\n");
intel_dp->want_panel_vdd = true;
- if (ironlake_edp_have_panel_vdd(intel_dp)) {
- DRM_DEBUG_KMS("eDP VDD already on\n");
+ if (ironlake_edp_have_panel_vdd(intel_dp))
return;
- }
+
+ DRM_DEBUG_KMS("Turning eDP VDD on\n");
if (!ironlake_edp_have_panel_power(intel_dp))
ironlake_wait_panel_power_cycle(intel_dp);
@@ -1121,6 +1120,8 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp
*intel_dp)
WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
if (!intel_dp->want_panel_vdd && ironlake_edp_have_panel_vdd(intel_dp))
{
+ DRM_DEBUG_KMS("Turning eDP VDD off\n");
+
pp = ironlake_get_pp_control(intel_dp);
pp &= ~EDP_FORCE_VDD;
@@ -1153,7 +1154,6 @@ void ironlake_edp_panel_vdd_off(struct intel_dp
*intel_dp, bool sync)
if (!is_edp(intel_dp))
return;
- DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->want_panel_vdd);
WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
intel_dp->want_panel_vdd = false;
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/i915: cancel the panel VDD work when we do it manually
From: Paulo Zanoni
After I reorganized the panel VDD debug messages I was able to spot we
were disabling it one extra time. The problem is that we're missing
the call to cancel the delayed work in one of the instances where we
manually call ironlake_panel_vdd_off_sync().
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_dp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8db1fda..f2280b4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1159,6 +1159,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp
*intel_dp, bool sync)
intel_dp->want_panel_vdd = false;
if (sync) {
+ cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
ironlake_panel_vdd_off_sync(intel_dp);
} else {
/*
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i810 forgets configured rows & columns on ttys on startx shutdown
On 2013-10-29 19:19 (GMT-0400) Felix Miata composed: On 2013-10-29 13:54 (GMT+0100) Daniel Vetter composed: Userspace modesetting is known to be racy and broken, so people don't really want to waste time on digging into bugs which are most likely flukes. But if you can bisect this issue to a specific patch a fix should be possible. Again I got no clear answer to the question asked, so I guess now it's fair to assume this must be the right place for discussion after all. I've installed openSUSE 12.3. Its 3.7.10-1.16-default and server 1.13.2 behave nicely. Beyond installing Mageia 3 to get 3.8.13 and 1.13.4, and as one who does not build software even via proxy, I'm open to suggestions how to get kernels and/or other xorg versions needed to perform bisection. This problem is differently present with Mageia 3's desktop kernel 3.8.13.4. The ttys are left usable on startx exit, but the exit log X prints to screen is scrambled from mismatch between character size and line length. I don't know whether to try older kernels, or newer, even if I knew where to find any. -- "The wise are known for their understanding, and pleasant words are persuasive." Proverbs 16:21 (New Living Translation) Team OS/2 ** Reg. Linux User #211409 ** a11y rocks! Felix Miata *** http://fm.no-ip.com/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
