[PATCH 1/3] drm/i915: Add kerneldoc for intel_pipe_update_{start, end}

[PATCH 1/3] drm/i915: Add kerneldoc for intel_pipe_update_{start, end}

[Ander Conselvan de Oliveira]

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8b80d68..f9ddedc 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
 	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
 }
 
+/**
+ * intel_pipe_update_start() - start update of a set of display registers
+ * @crtc: the crtc of which the registers are going to be updated
+ * @start_vbl_count: vblank counter return pointer used for error checking
+ *
+ * Mark the start of an update to pipe registers that should be updated
+ * atomically regarding vblank. If the next vblank will happens within
+ * the next 100 us, this function waits until the vblank passes.
+ *
+ * After a successful call to this function, interrupts will be disabled
+ * until a subsequent call to intel_pipe_update_end(). That is done to
+ * avoid random delays. The value written to @start_vbl_count should be
+ * supplied to intel_pipe_update_end() for error checking.
+ *
+ * Return: true if the call was successful
+ */
 static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -112,6 +128,15 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
 	return true;
 }
 
+/**
+ * intel_pipe_update_end() - end update of a set of display registers
+ * @crtc: the crtc of which the registers were updated
+ * @start_vbl_count: start vblank counter (used for error checking)
+ *
+ * Mark the end of an update started with intel_pipe_update_start(). This
+ * re-enables interrupts and verifies the update was actually completed
+ * before a vblank using the value of @start_vbl_count.
+ */
 static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Remove modeset lock check from intel_pipe_update_start()

[Ander Conselvan de Oliveira]

A follow up patch will call this funcion from a work context for the
mmio flip, in which case we cannot acquire the modeset locks. That's
not a problem though, since the check is there to protect vblank and
the mode, but the code that changes that waits for pending flips
first.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f9ddedc..c1d9547 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -72,8 +72,6 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
 	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
 	DEFINE_WAIT(wait);
 
-	WARN_ON(!drm_modeset_is_locked(&crtc->base.mutex));
-
 	vblank_start = mode->crtc_vblank_start;
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		vblank_start = DIV_ROUND_UP(vblank_start, 2);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Ander Conselvan de Oliveira]

Currently we program just DPSCNTR and DSPSTRIDE directly from the ring
interrupt handler, which is fine since the hardware guarantees that
those are update atomically. When we have atomic page flips we'll want
to be able to update also the offset registers, and then we need to use
the vblank evade mechanism to guarantee atomicity. Since that mechanism
introduces a wait, we need to do the actual register write from a work
when it is triggered by the ring interrupt.

v2: Explain the need for mmio_flip.work in the commit message (Paulo)
    Initialize the mmio_flip work in intel_crtc_init() (Paulo)
    Prevent new flips the previous flip work finishes (Paulo)
    Don't acquire modeset locks for mmio flip work

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     | 12 +++++++++++-
 drivers/gpu/drm/i915/intel_sprite.c  |  4 ++--
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8965f2d..86c2051 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9314,11 +9314,15 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 	struct intel_framebuffer *intel_fb =
 		to_intel_framebuffer(intel_crtc->base.primary->fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
+	bool atomic_update;
+	u32 start_vbl_count;
 	u32 dspcntr;
 	u32 reg;
 
 	intel_mark_page_flip_active(intel_crtc);
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	reg = DSPCNTR(intel_crtc->plane);
 	dspcntr = I915_READ(reg);
 
@@ -9332,6 +9336,21 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 	I915_WRITE(DSPSURF(intel_crtc->plane),
 		   intel_crtc->unpin_work->gtt_offset);
 	POSTING_READ(DSPSURF(intel_crtc->plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
+	spin_lock_irq(&dev_priv->mmio_flip_lock);
+	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE;
+	spin_unlock_irq(&dev_priv->mmio_flip_lock);
+}
+
+static void intel_mmio_flip_work_func(struct work_struct *work)
+{
+	struct intel_crtc *intel_crtc =
+		container_of(work, struct intel_crtc, mmio_flip.work);
+
+	intel_do_mmio_flip(intel_crtc);
 }
 
 static int intel_postpone_flip(struct drm_i915_gem_object *obj)
@@ -9374,15 +9393,15 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
 		struct intel_mmio_flip *mmio_flip;
 
 		mmio_flip = &intel_crtc->mmio_flip;
-		if (mmio_flip->seqno == 0)
+		if (mmio_flip->status != INTEL_MMIO_FLIP_WAIT_RING)
 			continue;
 
 		if (ring->id != mmio_flip->ring_id)
 			continue;
 
 		if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
-			intel_do_mmio_flip(intel_crtc);
-			mmio_flip->seqno = 0;
+			schedule_work(&intel_crtc->mmio_flip.work);
+			mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED;
 			ring->irq_put(ring);
 		}
 	}
@@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
 
-	if (WARN_ON(intel_crtc->mmio_flip.seqno))
+	if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
 		return -EBUSY;
 
 	ret = intel_postpone_flip(obj);
@@ -9412,6 +9431,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
 	}
 
 	spin_lock_irq(&dev_priv->mmio_flip_lock);
+	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_WAIT_RING;
 	intel_crtc->mmio_flip.seqno = obj->last_write_seqno;
 	intel_crtc->mmio_flip.ring_id = obj->ring->id;
 	spin_unlock_irq(&dev_priv->mmio_flip_lock);
@@ -11883,6 +11903,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
+	INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_func);
+
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d53ac23..ff4af6b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -399,9 +399,17 @@ struct intel_pipe_wm {
 	bool sprites_scaled;
 };
 
+enum intel_mmio_flip_status {
+	INTEL_MMIO_FLIP_IDLE = 0,
+	INTEL_MMIO_FLIP_WAIT_RING,
+	INTEL_MMIO_FLIP_WORK_SCHEDULED,
+};
+
 struct intel_mmio_flip {
 	u32 seqno;
 	u32 ring_id;
+	enum intel_mmio_flip_status status;
+	struct work_struct work;
 };
 
 struct intel_crtc {
@@ -1168,7 +1176,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
-
+bool intel_pipe_update_start(struct intel_crtc *crtc,
+			     uint32_t *start_vbl_count);
+void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c1d9547..9e2a5e2 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -62,7 +62,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
  *
  * Return: true if the call was successful
  */
-static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
 	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
@@ -135,7 +135,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
  * re-enables interrupts and verifies the update was actually completed
  * before a vblank using the value of @start_vbl_count.
  */
-static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
 	enum pipe pipe = crtc->pipe;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Damien Lespiau]

On Tue, Oct 28, 2014 at 03:10:14PM +0200, Ander Conselvan de Oliveira wrote:
> Currently we program just DPSCNTR and DSPSTRIDE directly from the ring
> interrupt handler, which is fine since the hardware guarantees that
> those are update atomically. When we have atomic page flips we'll want
> to be able to update also the offset registers, and then we need to use
> the vblank evade mechanism to guarantee atomicity. Since that mechanism
> introduces a wait, we need to do the actual register write from a work
> when it is triggered by the ring interrupt.
> 
> v2: Explain the need for mmio_flip.work in the commit message (Paulo)
>     Initialize the mmio_flip work in intel_crtc_init() (Paulo)
>     Prevent new flips the previous flip work finishes (Paulo)
>     Don't acquire modeset locks for mmio flip work

So that means you can replace the content of intel_do_mmio() by a call
to dev_priv->display.update_primary_plane() on top of this patch? That'd
do the right thing on SKL, which would make me happy.

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Add kerneldoc for intel_pipe_update_{start, end}

[Daniel Vetter]

On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote:
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8b80d68..f9ddedc 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>  	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
>  }
>  
> +/**
> + * intel_pipe_update_start() - start update of a set of display registers
> + * @crtc: the crtc of which the registers are going to be updated
> + * @start_vbl_count: vblank counter return pointer used for error checking
> + *
> + * Mark the start of an update to pipe registers that should be updated
> + * atomically regarding vblank. If the next vblank will happens within
> + * the next 100 us, this function waits until the vblank passes.
> + *
> + * After a successful call to this function, interrupts will be disabled
> + * until a subsequent call to intel_pipe_update_end(). That is done to
> + * avoid random delays. The value written to @start_vbl_count should be
> + * supplied to intel_pipe_update_end() for error checking.
> + *
> + * Return: true if the call was successful
> + */

It's nice that people now go overboard with kerneldoc, but I think we need
to strike a good balance. And in general I think documenting static inline
functions isn't worth it - they really should be self-explanatory as-is.

Documentation is imo only really useful for the bigger stuff, which
usually means it's used in a few places all over. So non-static functions.

This one here is a bit a corner-case since it's more of a generic piece of
infrastructure. But otoh I think in the end we'll have exactly one caller
of these functions for all atomic plane updates we'll need. Everyone else
(legacy code, modeset code, ...) will just call that higher-level
function. Not sure what to do here.
-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: [PATCH 1/3] drm/i915: Add kerneldoc for intel_pipe_update_{start, end}

[Ander Conselvan de Oliveira]

On 11/03/2014 02:33 PM, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote:
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 8b80d68..f9ddedc 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>>   	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
>>   }
>>
>> +/**
>> + * intel_pipe_update_start() - start update of a set of display registers
>> + * @crtc: the crtc of which the registers are going to be updated
>> + * @start_vbl_count: vblank counter return pointer used for error checking
>> + *
>> + * Mark the start of an update to pipe registers that should be updated
>> + * atomically regarding vblank. If the next vblank will happens within
>> + * the next 100 us, this function waits until the vblank passes.
>> + *
>> + * After a successful call to this function, interrupts will be disabled
>> + * until a subsequent call to intel_pipe_update_end(). That is done to
>> + * avoid random delays. The value written to @start_vbl_count should be
>> + * supplied to intel_pipe_update_end() for error checking.
>> + *
>> + * Return: true if the call was successful
>> + */
>
> It's nice that people now go overboard with kerneldoc, but I think we need
> to strike a good balance. And in general I think documenting static inline
> functions isn't worth it - they really should be self-explanatory as-is.
>
> Documentation is imo only really useful for the bigger stuff, which
> usually means it's used in a few places all over. So non-static functions.
>
> This one here is a bit a corner-case since it's more of a generic piece of
> infrastructure. But otoh I think in the end we'll have exactly one caller
> of these functions for all atomic plane updates we'll need. Everyone else
> (legacy code, modeset code, ...) will just call that higher-level
> function. Not sure what to do here.

s/**/*/ ?

Cheers,
Ander
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Add kerneldoc for intel_pipe_update_{start, end}

[Paulo Zanoni]

2014-11-03 10:33 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote:
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 8b80d68..f9ddedc 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>>       return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
>>  }
>>
>> +/**
>> + * intel_pipe_update_start() - start update of a set of display registers
>> + * @crtc: the crtc of which the registers are going to be updated
>> + * @start_vbl_count: vblank counter return pointer used for error checking
>> + *
>> + * Mark the start of an update to pipe registers that should be updated
>> + * atomically regarding vblank. If the next vblank will happens within
>> + * the next 100 us, this function waits until the vblank passes.
>> + *
>> + * After a successful call to this function, interrupts will be disabled
>> + * until a subsequent call to intel_pipe_update_end(). That is done to
>> + * avoid random delays. The value written to @start_vbl_count should be
>> + * supplied to intel_pipe_update_end() for error checking.
>> + *
>> + * Return: true if the call was successful
>> + */
>
> It's nice that people now go overboard with kerneldoc, but I think we need
> to strike a good balance. And in general I think documenting static inline
> functions isn't worth it - they really should be self-explanatory as-is.

But patch 3 exports these functions and uses them from another file.

>
> Documentation is imo only really useful for the bigger stuff, which
> usually means it's used in a few places all over. So non-static functions.

The comments he introduced are useful and helped me review patch 3
without having to look at the function implementation and waste 20
minutes wondering what it was supposed to do.

To me, this patch is an improvement to the codebase, so with or
without the extra '*' chars:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> This one here is a bit a corner-case since it's more of a generic piece of
> infrastructure. But otoh I think in the end we'll have exactly one caller
> of these functions for all atomic plane updates we'll need. Everyone else
> (legacy code, modeset code, ...) will just call that higher-level
> function. Not sure what to do here.
> -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



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Add kerneldoc for intel_pipe_update_{start, end}

[Daniel Vetter]

On Mon, Nov 03, 2014 at 03:26:36PM -0200, Paulo Zanoni wrote:
> 2014-11-03 10:33 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote:
> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++
> >>  1 file changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index 8b80d68..f9ddedc 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
> >>       return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> >>  }
> >>
> >> +/**
> >> + * intel_pipe_update_start() - start update of a set of display registers
> >> + * @crtc: the crtc of which the registers are going to be updated
> >> + * @start_vbl_count: vblank counter return pointer used for error checking
> >> + *
> >> + * Mark the start of an update to pipe registers that should be updated
> >> + * atomically regarding vblank. If the next vblank will happens within
> >> + * the next 100 us, this function waits until the vblank passes.
> >> + *
> >> + * After a successful call to this function, interrupts will be disabled
> >> + * until a subsequent call to intel_pipe_update_end(). That is done to
> >> + * avoid random delays. The value written to @start_vbl_count should be
> >> + * supplied to intel_pipe_update_end() for error checking.
> >> + *
> >> + * Return: true if the call was successful
> >> + */
> >
> > It's nice that people now go overboard with kerneldoc, but I think we need
> > to strike a good balance. And in general I think documenting static inline
> > functions isn't worth it - they really should be self-explanatory as-is.
> 
> But patch 3 exports these functions and uses them from another file.

Ah, I've missed that, comment retracted ...
> 
> >
> > Documentation is imo only really useful for the bigger stuff, which
> > usually means it's used in a few places all over. So non-static functions.
> 
> The comments he introduced are useful and helped me review patch 3
> without having to look at the function implementation and waste 20
> minutes wondering what it was supposed to do.
> 
> To me, this patch is an improvement to the codebase, so with or
> without the extra '*' chars:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

... and patch merged.

Thanks, 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: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Paulo Zanoni]

2014-10-28 11:10 GMT-02:00 Ander Conselvan de Oliveira
<ander.conselvan.de.oliveira@intel.com>:
> Currently we program just DPSCNTR and DSPSTRIDE directly from the ring
> interrupt handler, which is fine since the hardware guarantees that
> those are update atomically. When we have atomic page flips we'll want
> to be able to update also the offset registers, and then we need to use
> the vblank evade mechanism to guarantee atomicity. Since that mechanism
> introduces a wait, we need to do the actual register write from a work
> when it is triggered by the ring interrupt.
>
> v2: Explain the need for mmio_flip.work in the commit message (Paulo)
>     Initialize the mmio_flip work in intel_crtc_init() (Paulo)
>     Prevent new flips the previous flip work finishes (Paulo)
>     Don't acquire modeset locks for mmio flip work
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     | 12 +++++++++++-
>  drivers/gpu/drm/i915/intel_sprite.c  |  4 ++--
>  3 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8965f2d..86c2051 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9314,11 +9314,15 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>         struct intel_framebuffer *intel_fb =
>                 to_intel_framebuffer(intel_crtc->base.primary->fb);
>         struct drm_i915_gem_object *obj = intel_fb->obj;
> +       bool atomic_update;
> +       u32 start_vbl_count;
>         u32 dspcntr;
>         u32 reg;
>
>         intel_mark_page_flip_active(intel_crtc);
>
> +       atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);

Don't we want to print something (error message?) in case this returns false?

Besides this, my only worry is what is going to happen if we disable
the CRTC or plane (or even suspend - S3 or runtime - or something like
that) after we schedule the work, but before the work actually
happens. Isn't it possible to end up accidentally reenabling something
that was just disabled? We can't seem to queue a new flip before the
current one finishes, but maybe we can shut down the CRTC while the
work is scheduled? I don't know much about vblanks to properly answer
that, so I'll let Daniel/Ville take a look at this.

Everything else looks correct, so if someone guarantees the above
question is not a problem: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@intel.com>

> +
>         reg = DSPCNTR(intel_crtc->plane);
>         dspcntr = I915_READ(reg);
>
> @@ -9332,6 +9336,21 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>         I915_WRITE(DSPSURF(intel_crtc->plane),
>                    intel_crtc->unpin_work->gtt_offset);
>         POSTING_READ(DSPSURF(intel_crtc->plane));
> +
> +       if (atomic_update)
> +               intel_pipe_update_end(intel_crtc, start_vbl_count);
> +
> +       spin_lock_irq(&dev_priv->mmio_flip_lock);
> +       intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE;
> +       spin_unlock_irq(&dev_priv->mmio_flip_lock);
> +}
> +
> +static void intel_mmio_flip_work_func(struct work_struct *work)
> +{
> +       struct intel_crtc *intel_crtc =
> +               container_of(work, struct intel_crtc, mmio_flip.work);
> +
> +       intel_do_mmio_flip(intel_crtc);
>  }
>
>  static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> @@ -9374,15 +9393,15 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
>                 struct intel_mmio_flip *mmio_flip;
>
>                 mmio_flip = &intel_crtc->mmio_flip;
> -               if (mmio_flip->seqno == 0)
> +               if (mmio_flip->status != INTEL_MMIO_FLIP_WAIT_RING)
>                         continue;
>
>                 if (ring->id != mmio_flip->ring_id)
>                         continue;
>
>                 if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
> -                       intel_do_mmio_flip(intel_crtc);
> -                       mmio_flip->seqno = 0;
> +                       schedule_work(&intel_crtc->mmio_flip.work);
> +                       mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED;
>                         ring->irq_put(ring);
>                 }
>         }
> @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         int ret;
>
> -       if (WARN_ON(intel_crtc->mmio_flip.seqno))
> +       if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
>                 return -EBUSY;
>
>         ret = intel_postpone_flip(obj);
> @@ -9412,6 +9431,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>         }
>
>         spin_lock_irq(&dev_priv->mmio_flip_lock);
> +       intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_WAIT_RING;
>         intel_crtc->mmio_flip.seqno = obj->last_write_seqno;
>         intel_crtc->mmio_flip.ring_id = obj->ring->id;
>         spin_unlock_irq(&dev_priv->mmio_flip_lock);
> @@ -11883,6 +11903,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>         dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>         dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>
> +       INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_func);
> +
>         drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>
>         WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d53ac23..ff4af6b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -399,9 +399,17 @@ struct intel_pipe_wm {
>         bool sprites_scaled;
>  };
>
> +enum intel_mmio_flip_status {
> +       INTEL_MMIO_FLIP_IDLE = 0,
> +       INTEL_MMIO_FLIP_WAIT_RING,
> +       INTEL_MMIO_FLIP_WORK_SCHEDULED,
> +};
> +
>  struct intel_mmio_flip {
>         u32 seqno;
>         u32 ring_id;
> +       enum intel_mmio_flip_status status;
> +       struct work_struct work;
>  };
>
>  struct intel_crtc {
> @@ -1168,7 +1176,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>                               struct drm_file *file_priv);
>  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
>                               struct drm_file *file_priv);
> -
> +bool intel_pipe_update_start(struct intel_crtc *crtc,
> +                            uint32_t *start_vbl_count);
> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
>
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c1d9547..9e2a5e2 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -62,7 +62,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>   *
>   * Return: true if the call was successful
>   */
> -static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
> +bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> @@ -135,7 +135,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
>   * re-enables interrupts and verifies the update was actually completed
>   * before a vblank using the value of @start_vbl_count.
>   */
> -static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         enum pipe pipe = crtc->pipe;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Daniel Vetter]

On Mon, Nov 3, 2014 at 7:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Besides this, my only worry is what is going to happen if we disable
> the CRTC or plane (or even suspend - S3 or runtime - or something like
> that) after we schedule the work, but before the work actually
> happens. Isn't it possible to end up accidentally reenabling something
> that was just disabled? We can't seem to queue a new flip before the
> current one finishes, but maybe we can shut down the CRTC while the
> work is scheduled? I don't know much about vblanks to properly answer
> that, so I'll let Daniel/Ville take a look at this.

We do wait for pageflips to complete before disabling the crtc or
updating the framebuffer. But I don't think we actually lack the
corresponding code to do the same when disabling the pipe with dpms
off. We have extensive sets of testcases to race pageflips against
almost any combination of dpms calls, setCrtc changes, vblank waits or
any other nonsense, so if that still works we should be good I think.

Or maybe that's the reason why kms_flip has such a poor pass-rate?
-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: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Paulo Zanoni]

2014-11-03 16:53 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Nov 3, 2014 at 7:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Besides this, my only worry is what is going to happen if we disable
>> the CRTC or plane (or even suspend - S3 or runtime - or something like
>> that) after we schedule the work, but before the work actually
>> happens. Isn't it possible to end up accidentally reenabling something
>> that was just disabled? We can't seem to queue a new flip before the
>> current one finishes, but maybe we can shut down the CRTC while the
>> work is scheduled? I don't know much about vblanks to properly answer
>> that, so I'll let Daniel/Ville take a look at this.
>
> We do wait for pageflips to complete before disabling the crtc or
> updating the framebuffer.

But, as far as I can see, those functions do not take into
consideration the new workqueue added by this patch.

> But I don't think we actually lack the
> corresponding code to do the same when disabling the pipe with dpms
> off. We have extensive sets of testcases to race pageflips against
> almost any combination of dpms calls, setCrtc changes, vblank waits or
> any other nonsense, so if that still works we should be good I think.
>
> Or maybe that's the reason why kms_flip has such a poor pass-rate?
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Daniel Vetter]

On Mon, Nov 3, 2014 at 8:13 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-11-03 16:53 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Mon, Nov 3, 2014 at 7:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>> Besides this, my only worry is what is going to happen if we disable
>>> the CRTC or plane (or even suspend - S3 or runtime - or something like
>>> that) after we schedule the work, but before the work actually
>>> happens. Isn't it possible to end up accidentally reenabling something
>>> that was just disabled? We can't seem to queue a new flip before the
>>> current one finishes, but maybe we can shut down the CRTC while the
>>> work is scheduled? I don't know much about vblanks to properly answer
>>> that, so I'll let Daniel/Ville take a look at this.
>>
>> We do wait for pageflips to complete before disabling the crtc or
>> updating the framebuffer.
>
> But, as far as I can see, those functions do not take into
> consideration the new workqueue added by this patch.

Well we wait for the flip to happen. How that happens shouldn't really
matter at all - the flip only completes on the next vblank, which
might actaully be a lot later than when the work item has run. It's
guaranteed to be after the mmio writes though.
-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: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Ander Conselvan de Oliveira]

Hi Paulo,

Thanks for reviewing.

On 11/03/2014 08:37 PM, Paulo Zanoni wrote:
> 2014-10-28 11:10 GMT-02:00 Ander Conselvan de Oliveira
> <ander.conselvan.de.oliveira@intel.com>:
>> Currently we program just DPSCNTR and DSPSTRIDE directly from the ring
>> interrupt handler, which is fine since the hardware guarantees that
>> those are update atomically. When we have atomic page flips we'll want
>> to be able to update also the offset registers, and then we need to use
>> the vblank evade mechanism to guarantee atomicity. Since that mechanism
>> introduces a wait, we need to do the actual register write from a work
>> when it is triggered by the ring interrupt.
>>
>> v2: Explain the need for mmio_flip.work in the commit message (Paulo)
>>      Initialize the mmio_flip work in intel_crtc_init() (Paulo)
>>      Prevent new flips the previous flip work finishes (Paulo)
>>      Don't acquire modeset locks for mmio flip work
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++----
>>   drivers/gpu/drm/i915/intel_drv.h     | 12 +++++++++++-
>>   drivers/gpu/drm/i915/intel_sprite.c  |  4 ++--
>>   3 files changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8965f2d..86c2051 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9314,11 +9314,15 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>>          struct intel_framebuffer *intel_fb =
>>                  to_intel_framebuffer(intel_crtc->base.primary->fb);
>>          struct drm_i915_gem_object *obj = intel_fb->obj;
>> +       bool atomic_update;
>> +       u32 start_vbl_count;
>>          u32 dspcntr;
>>          u32 reg;
>>
>>          intel_mark_page_flip_active(intel_crtc);
>>
>> +       atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>
> Don't we want to print something (error message?) in case this returns false?

Good question. The function itself has a warn for one of the failure 
cases (when drm_vblank_get() fails) but it doesn't print anything if the 
calculation for min and max scanlines fails. It seems the latter case 
would fail if we have a bad mode (with vblank_start <= 1) or in some 
unrealistic scenarios involving a combination of low pixel clocks and 
high htotal.

In any case, I guess we would be better to add a the warning to 
intel_pipe_update_start() itself, since there are other users that 
follow the same pattern.

Ander


> Besides this, my only worry is what is going to happen if we disable
> the CRTC or plane (or even suspend - S3 or runtime - or something like
> that) after we schedule the work, but before the work actually
> happens. Isn't it possible to end up accidentally reenabling something
> that was just disabled? We can't seem to queue a new flip before the
> current one finishes, but maybe we can shut down the CRTC while the
> work is scheduled? I don't know much about vblanks to properly answer
> that, so I'll let Daniel/Ville take a look at this.
>
> Everything else looks correct, so if someone guarantees the above
> question is not a problem: Reviewed-by: Paulo Zanoni
> <paulo.r.zanoni@intel.com>
>
>> +
>>          reg = DSPCNTR(intel_crtc->plane);
>>          dspcntr = I915_READ(reg);
>>
>> @@ -9332,6 +9336,21 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>>          I915_WRITE(DSPSURF(intel_crtc->plane),
>>                     intel_crtc->unpin_work->gtt_offset);
>>          POSTING_READ(DSPSURF(intel_crtc->plane));
>> +
>> +       if (atomic_update)
>> +               intel_pipe_update_end(intel_crtc, start_vbl_count);
>> +
>> +       spin_lock_irq(&dev_priv->mmio_flip_lock);
>> +       intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE;
>> +       spin_unlock_irq(&dev_priv->mmio_flip_lock);
>> +}
>> +
>> +static void intel_mmio_flip_work_func(struct work_struct *work)
>> +{
>> +       struct intel_crtc *intel_crtc =
>> +               container_of(work, struct intel_crtc, mmio_flip.work);
>> +
>> +       intel_do_mmio_flip(intel_crtc);
>>   }
>>
>>   static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>> @@ -9374,15 +9393,15 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
>>                  struct intel_mmio_flip *mmio_flip;
>>
>>                  mmio_flip = &intel_crtc->mmio_flip;
>> -               if (mmio_flip->seqno == 0)
>> +               if (mmio_flip->status != INTEL_MMIO_FLIP_WAIT_RING)
>>                          continue;
>>
>>                  if (ring->id != mmio_flip->ring_id)
>>                          continue;
>>
>>                  if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
>> -                       intel_do_mmio_flip(intel_crtc);
>> -                       mmio_flip->seqno = 0;
>> +                       schedule_work(&intel_crtc->mmio_flip.work);
>> +                       mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED;
>>                          ring->irq_put(ring);
>>                  }
>>          }
>> @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>>          struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>          int ret;
>>
>> -       if (WARN_ON(intel_crtc->mmio_flip.seqno))
>> +       if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
>>                  return -EBUSY;
>>
>>          ret = intel_postpone_flip(obj);
>> @@ -9412,6 +9431,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>>          }
>>
>>          spin_lock_irq(&dev_priv->mmio_flip_lock);
>> +       intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_WAIT_RING;
>>          intel_crtc->mmio_flip.seqno = obj->last_write_seqno;
>>          intel_crtc->mmio_flip.ring_id = obj->ring->id;
>>          spin_unlock_irq(&dev_priv->mmio_flip_lock);
>> @@ -11883,6 +11903,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>>          dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>>          dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>>
>> +       INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_func);
>> +
>>          drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>>
>>          WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d53ac23..ff4af6b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -399,9 +399,17 @@ struct intel_pipe_wm {
>>          bool sprites_scaled;
>>   };
>>
>> +enum intel_mmio_flip_status {
>> +       INTEL_MMIO_FLIP_IDLE = 0,
>> +       INTEL_MMIO_FLIP_WAIT_RING,
>> +       INTEL_MMIO_FLIP_WORK_SCHEDULED,
>> +};
>> +
>>   struct intel_mmio_flip {
>>          u32 seqno;
>>          u32 ring_id;
>> +       enum intel_mmio_flip_status status;
>> +       struct work_struct work;
>>   };
>>
>>   struct intel_crtc {
>> @@ -1168,7 +1176,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>>                                struct drm_file *file_priv);
>>   int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
>>                                struct drm_file *file_priv);
>> -
>> +bool intel_pipe_update_start(struct intel_crtc *crtc,
>> +                            uint32_t *start_vbl_count);
>> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
>>
>>   /* intel_tv.c */
>>   void intel_tv_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index c1d9547..9e2a5e2 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -62,7 +62,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>>    *
>>    * Return: true if the call was successful
>>    */
>> -static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
>> +bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
>>   {
>>          struct drm_device *dev = crtc->base.dev;
>>          const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
>> @@ -135,7 +135,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
>>    * re-enables interrupts and verifies the update was actually completed
>>    * before a vblank using the value of @start_vbl_count.
>>    */
>> -static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
>> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
>>   {
>>          struct drm_device *dev = crtc->base.dev;
>>          enum pipe pipe = crtc->pipe;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Daniel Vetter]

On Tue, Oct 28, 2014 at 03:10:14PM +0200, Ander Conselvan de Oliveira wrote:
> @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int ret;
>  
> -	if (WARN_ON(intel_crtc->mmio_flip.seqno))
> +	if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
>  		return -EBUSY;

Ok I've merged the patch, but I think this check is wrong. do_mmio_flip
only sets this to idle _after_ the hw register writing is done. Which
means the flip could complete and userspace could receive the event all
before we set status to IDLE. Which means you can WARN and return -EBUSY
here when the flip definitely completed.

So either clear IDLE at the head of do_mmio_flip before doing anything, or
drop this state again. After all the previous seqno check only looked for
waiting or not. tbh I'm not even sure whether this buys us anything at all
- we already check that the flip completed, which should ensure ordering.
I think we could just rip out mmio_flip.status completely.

Can you pls do a fixup patch?

Thanks, 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: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

[Chris Wilson]

On Tue, Nov 04, 2014 at 12:04:03PM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:10:14PM +0200, Ander Conselvan de Oliveira wrote:
> > @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	int ret;
> >  
> > -	if (WARN_ON(intel_crtc->mmio_flip.seqno))
> > +	if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
> >  		return -EBUSY;
> 
> Ok I've merged the patch, but I think this check is wrong. do_mmio_flip
> only sets this to idle _after_ the hw register writing is done. Which
> means the flip could complete and userspace could receive the event all
> before we set status to IDLE. Which means you can WARN and return -EBUSY
> here when the flip definitely completed.
> 
> So either clear IDLE at the head of do_mmio_flip before doing anything, or
> drop this state again. After all the previous seqno check only looked for
> waiting or not. tbh I'm not even sure whether this buys us anything at all
> - we already check that the flip completed, which should ensure ordering.
> I think we could just rip out mmio_flip.status completely.
> 
> Can you pls do a fixup patch?

A better strategy would be to use the generic seqno mechanism for the
flip from a worker which then wouldn't need this extra vblank state
machinery on top of the vblank evade...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Ander Conselvan de Oliveira]

This simplifies the code quite a bit compared to iterating over all
rings during the ring interrupt.

Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
struct is only accessed in two places. The first is when the flip is
queued and the other when the mmio writes are done. Since a flip cannot
be queued while there is a pending flip, the two paths shouldn't ever
run in parallel. We might need to revisit that if support for replacing
flips is implemented though.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---

I'm not sure if locking dev->struct_mutex in the work function might
have any ill effects, so I'd appreciate if someone could enlighten me.

Thanks,
Ander

---
 drivers/gpu/drm/i915/i915_irq.c      |  3 --
 drivers/gpu/drm/i915/intel_display.c | 91 +++++-------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  9 +---
 3 files changed, 13 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 318a6a0..5fff287 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -979,9 +979,6 @@ static void notify_ring(struct drm_device *dev,
 
 	trace_i915_gem_request_complete(ring);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		intel_notify_mmio_flip(ring);
-
 	wake_up_all(&ring->irq_queue);
 	i915_queue_hangcheck(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f3ff8e8..24cfe19 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9424,73 +9424,25 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
-
-	spin_lock_irq(&dev_priv->mmio_flip_lock);
-	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE;
-	spin_unlock_irq(&dev_priv->mmio_flip_lock);
 }
 
 static void intel_mmio_flip_work_func(struct work_struct *work)
 {
 	struct intel_crtc *intel_crtc =
 		container_of(work, struct intel_crtc, mmio_flip.work);
-
-	intel_do_mmio_flip(intel_crtc);
-}
-
-static int intel_postpone_flip(struct drm_i915_gem_object *obj)
-{
 	struct intel_engine_cs *ring;
-	int ret;
-
-	lockdep_assert_held(&obj->base.dev->struct_mutex);
-
-	if (!obj->last_write_seqno)
-		return 0;
-
-	ring = obj->ring;
+	uint32_t seqno;
 
-	if (i915_seqno_passed(ring->get_seqno(ring, true),
-			      obj->last_write_seqno))
-		return 0;
-
-	ret = i915_gem_check_olr(ring, obj->last_write_seqno);
-	if (ret)
-		return ret;
-
-	if (WARN_ON(!ring->irq_get(ring)))
-		return 0;
+	seqno = intel_crtc->mmio_flip.seqno;
+	ring = intel_crtc->mmio_flip.ring;
 
-	return 1;
-}
-
-void intel_notify_mmio_flip(struct intel_engine_cs *ring)
-{
-	struct drm_i915_private *dev_priv = to_i915(ring->dev);
-	struct intel_crtc *intel_crtc;
-	unsigned long irq_flags;
-	u32 seqno;
-
-	seqno = ring->get_seqno(ring, false);
-
-	spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
-	for_each_intel_crtc(ring->dev, intel_crtc) {
-		struct intel_mmio_flip *mmio_flip;
-
-		mmio_flip = &intel_crtc->mmio_flip;
-		if (mmio_flip->status != INTEL_MMIO_FLIP_WAIT_RING)
-			continue;
-
-		if (ring->id != mmio_flip->ring_id)
-			continue;
-
-		if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
-			schedule_work(&intel_crtc->mmio_flip.work);
-			mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED;
-			ring->irq_put(ring);
-		}
+	if (seqno) {
+		mutex_lock(&ring->dev->struct_mutex);
+		WARN_ON(i915_wait_seqno(ring, seqno) != 0);
+		mutex_unlock(&ring->dev->struct_mutex);
 	}
-	spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
+
+	intel_do_mmio_flip(intel_crtc);
 }
 
 static int intel_queue_mmio_flip(struct drm_device *dev,
@@ -9500,32 +9452,13 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
 				 struct intel_engine_cs *ring,
 				 uint32_t flags)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
-		return -EBUSY;
-
-	ret = intel_postpone_flip(obj);
-	if (ret < 0)
-		return ret;
-	if (ret == 0) {
-		intel_do_mmio_flip(intel_crtc);
-		return 0;
-	}
 
-	spin_lock_irq(&dev_priv->mmio_flip_lock);
-	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_WAIT_RING;
 	intel_crtc->mmio_flip.seqno = obj->last_write_seqno;
-	intel_crtc->mmio_flip.ring_id = obj->ring->id;
-	spin_unlock_irq(&dev_priv->mmio_flip_lock);
+	intel_crtc->mmio_flip.ring = obj->ring;
+
+	schedule_work(&intel_crtc->mmio_flip.work);
 
-	/*
-	 * Double check to catch cases where irq fired before
-	 * mmio flip data was ready
-	 */
-	intel_notify_mmio_flip(obj->ring);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c8ddde6..492d346 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -399,16 +399,9 @@ struct intel_pipe_wm {
 	bool sprites_scaled;
 };
 
-enum intel_mmio_flip_status {
-	INTEL_MMIO_FLIP_IDLE = 0,
-	INTEL_MMIO_FLIP_WAIT_RING,
-	INTEL_MMIO_FLIP_WORK_SCHEDULED,
-};
-
 struct intel_mmio_flip {
 	u32 seqno;
-	u32 ring_id;
-	enum intel_mmio_flip_status status;
+	struct intel_engine_cs *ring;
 	struct work_struct work;
 };
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Chris Wilson]

On Wed, Nov 05, 2014 at 01:03:00PM +0200, Ander Conselvan de Oliveira wrote:
> This simplifies the code quite a bit compared to iterating over all
> rings during the ring interrupt.
> 
> Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
> struct is only accessed in two places. The first is when the flip is
> queued and the other when the mmio writes are done. Since a flip cannot
> be queued while there is a pending flip, the two paths shouldn't ever
> run in parallel. We might need to revisit that if support for replacing
> flips is implemented though.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> 
> I'm not sure if locking dev->struct_mutex in the work function might
> have any ill effects, so I'd appreciate if someone could enlighten me.

Good news is you can wait on the seqno without holding any locks. You
need to use the lower level entry point __wait_seqno() though.

Overall it looked good.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Ander Conselvan de Oliveira]

On 11/05/2014 01:23 PM, Chris Wilson wrote:
> On Wed, Nov 05, 2014 at 01:03:00PM +0200, Ander Conselvan de Oliveira wrote:
>> This simplifies the code quite a bit compared to iterating over all
>> rings during the ring interrupt.
>>
>> Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
>> struct is only accessed in two places. The first is when the flip is
>> queued and the other when the mmio writes are done. Since a flip cannot
>> be queued while there is a pending flip, the two paths shouldn't ever
>> run in parallel. We might need to revisit that if support for replacing
>> flips is implemented though.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>
>> I'm not sure if locking dev->struct_mutex in the work function might
>> have any ill effects, so I'd appreciate if someone could enlighten me.
>
> Good news is you can wait on the seqno without holding any locks. You
> need to use the lower level entry point __wait_seqno() though.

I considered the fact that __wait_seqno() is static as a warning that I 
shouldn't use it directly. Should I just change that so I can use it 
intel_display.c?

The other reason I avoided it is because I didn't really understand how 
the barrier for reading the reset_counter should work. Is it sufficient 
that I do

   atomic_read(&dev_priv->gpu_error.reset_counter))

before calling __wait_seqno() or is there something more to it?

Thanks,
ander


---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Chris Wilson]

On Wed, Nov 05, 2014 at 02:23:07PM +0200, Ander Conselvan de Oliveira wrote:
> On 11/05/2014 01:23 PM, Chris Wilson wrote:
> >On Wed, Nov 05, 2014 at 01:03:00PM +0200, Ander Conselvan de Oliveira wrote:
> >>This simplifies the code quite a bit compared to iterating over all
> >>rings during the ring interrupt.
> >>
> >>Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
> >>struct is only accessed in two places. The first is when the flip is
> >>queued and the other when the mmio writes are done. Since a flip cannot
> >>be queued while there is a pending flip, the two paths shouldn't ever
> >>run in parallel. We might need to revisit that if support for replacing
> >>flips is implemented though.
> >>
> >>Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >>---
> >>
> >>I'm not sure if locking dev->struct_mutex in the work function might
> >>have any ill effects, so I'd appreciate if someone could enlighten me.
> >
> >Good news is you can wait on the seqno without holding any locks. You
> >need to use the lower level entry point __wait_seqno() though.
> 
> I considered the fact that __wait_seqno() is static as a warning
> that I shouldn't use it directly. Should I just change that so I can
> use it intel_display.c?
> 
> The other reason I avoided it is because I didn't really understand
> how the barrier for reading the reset_counter should work. Is it
> sufficient that I do
> 
>   atomic_read(&dev_priv->gpu_error.reset_counter))
> 
> before calling __wait_seqno() or is there something more to it?

We should already have a reset counter associated with the flip (at
least I do...) But yes, the least you need to do is pass down the
current atomic_read(&reset_counter).

That it is __wait_seqno() is indeed meant to make you think twice before
using it, but really the only problem with is the name and cumbersome
arguments. I have no qualms about renaming it as __i915_wait_seqno() and
using it here - it is what I settled on in the requests conversion.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: Make __wait_seqno non-static and rename to __i915_wait_seqno

[Ander Conselvan de Oliveira]

So that it can be used by the flip code to wait for rendering without
holding any locks.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  5 +++++
 drivers/gpu/drm/i915/i915_gem.c | 20 +++++++++++---------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 54691bc..d6a0100 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2535,6 +2535,11 @@ int __i915_add_request(struct intel_engine_cs *ring,
 		       u32 *seqno);
 #define i915_add_request(ring, seqno) \
 	__i915_add_request(ring, NULL, NULL, seqno)
+int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
+			unsigned reset_counter,
+			bool interruptible,
+			s64 *timeout,
+			struct drm_i915_file_private *file_priv);
 int __must_check i915_wait_seqno(struct intel_engine_cs *ring,
 				 uint32_t seqno);
 int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1de94cc..3e0cabe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1134,7 +1134,7 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv)
 }
 
 /**
- * __wait_seqno - wait until execution of seqno has finished
+ * __i915_wait_seqno - wait until execution of seqno has finished
  * @ring: the ring expected to report seqno
  * @seqno: duh!
  * @reset_counter: reset sequence associated with the given seqno
@@ -1151,7 +1151,7 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv)
  * Returns 0 if the seqno was found within the alloted time. Else returns the
  * errno with remaining time filled in timeout argument.
  */
-static int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
+int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 			unsigned reset_counter,
 			bool interruptible,
 			s64 *timeout,
@@ -1262,6 +1262,7 @@ i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno)
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool interruptible = dev_priv->mm.interruptible;
+	unsigned reset_counter;
 	int ret;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
@@ -1275,9 +1276,9 @@ i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno)
 	if (ret)
 		return ret;
 
-	return __wait_seqno(ring, seqno,
-			    atomic_read(&dev_priv->gpu_error.reset_counter),
-			    interruptible, NULL, NULL);
+	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+	return __i915_wait_seqno(ring, seqno, reset_counter, interruptible,
+				 NULL, NULL);
 }
 
 static int
@@ -1353,7 +1354,8 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	mutex_unlock(&dev->struct_mutex);
-	ret = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv);
+	ret = __i915_wait_seqno(ring, seqno, reset_counter, true, NULL,
+				file_priv);
 	mutex_lock(&dev->struct_mutex);
 	if (ret)
 		return ret;
@@ -2847,8 +2849,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	mutex_unlock(&dev->struct_mutex);
 
-	return __wait_seqno(ring, seqno, reset_counter, true, &args->timeout_ns,
-			    file->driver_priv);
+	return __i915_wait_seqno(ring, seqno, reset_counter, true,
+				 &args->timeout_ns, file->driver_priv);
 
 out:
 	drm_gem_object_unreference(&obj->base);
@@ -4013,7 +4015,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	if (seqno == 0)
 		return 0;
 
-	ret = __wait_seqno(ring, seqno, reset_counter, true, NULL, NULL);
+	ret = __i915_wait_seqno(ring, seqno, reset_counter, true, NULL, NULL);
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Make mmio flip wait for seqno in the work function

[Ander Conselvan de Oliveira]

This simplifies the code quite a bit compared to iterating over all
rings during the ring interrupt.

Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
struct is only accessed in two places. The first is when the flip is
queued and the other when the mmio writes are done. Since a flip cannot
be queued while there is a pending flip, the two paths shouldn't ever
run in parallel. We might need to revisit that if support for replacing
flips is implemented though.

v2: Don't hold dev->struct_mutext while waiting (Chris)

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  3 --
 drivers/gpu/drm/i915/intel_display.c | 90 +++++-------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  9 +---
 3 files changed, 12 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 318a6a0..5fff287 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -979,9 +979,6 @@ static void notify_ring(struct drm_device *dev,
 
 	trace_i915_gem_request_complete(ring);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		intel_notify_mmio_flip(ring);
-
 	wake_up_all(&ring->irq_queue);
 	i915_queue_hangcheck(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f3ff8e8..ea8c2f2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9424,73 +9424,24 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
-
-	spin_lock_irq(&dev_priv->mmio_flip_lock);
-	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE;
-	spin_unlock_irq(&dev_priv->mmio_flip_lock);
 }
 
 static void intel_mmio_flip_work_func(struct work_struct *work)
 {
 	struct intel_crtc *intel_crtc =
 		container_of(work, struct intel_crtc, mmio_flip.work);
-
-	intel_do_mmio_flip(intel_crtc);
-}
-
-static int intel_postpone_flip(struct drm_i915_gem_object *obj)
-{
 	struct intel_engine_cs *ring;
-	int ret;
-
-	lockdep_assert_held(&obj->base.dev->struct_mutex);
-
-	if (!obj->last_write_seqno)
-		return 0;
-
-	ring = obj->ring;
-
-	if (i915_seqno_passed(ring->get_seqno(ring, true),
-			      obj->last_write_seqno))
-		return 0;
-
-	ret = i915_gem_check_olr(ring, obj->last_write_seqno);
-	if (ret)
-		return ret;
-
-	if (WARN_ON(!ring->irq_get(ring)))
-		return 0;
-
-	return 1;
-}
+	uint32_t seqno;
 
-void intel_notify_mmio_flip(struct intel_engine_cs *ring)
-{
-	struct drm_i915_private *dev_priv = to_i915(ring->dev);
-	struct intel_crtc *intel_crtc;
-	unsigned long irq_flags;
-	u32 seqno;
-
-	seqno = ring->get_seqno(ring, false);
+	seqno = intel_crtc->mmio_flip.seqno;
+	ring = intel_crtc->mmio_flip.ring;
 
-	spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
-	for_each_intel_crtc(ring->dev, intel_crtc) {
-		struct intel_mmio_flip *mmio_flip;
+	if (seqno)
+		WARN_ON(__i915_wait_seqno(ring, seqno,
+					  intel_crtc->reset_counter,
+					  true, NULL, NULL) != 0);
 
-		mmio_flip = &intel_crtc->mmio_flip;
-		if (mmio_flip->status != INTEL_MMIO_FLIP_WAIT_RING)
-			continue;
-
-		if (ring->id != mmio_flip->ring_id)
-			continue;
-
-		if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
-			schedule_work(&intel_crtc->mmio_flip.work);
-			mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED;
-			ring->irq_put(ring);
-		}
-	}
-	spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
+	intel_do_mmio_flip(intel_crtc);
 }
 
 static int intel_queue_mmio_flip(struct drm_device *dev,
@@ -9500,32 +9451,13 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
 				 struct intel_engine_cs *ring,
 				 uint32_t flags)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
-		return -EBUSY;
-
-	ret = intel_postpone_flip(obj);
-	if (ret < 0)
-		return ret;
-	if (ret == 0) {
-		intel_do_mmio_flip(intel_crtc);
-		return 0;
-	}
 
-	spin_lock_irq(&dev_priv->mmio_flip_lock);
-	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_WAIT_RING;
 	intel_crtc->mmio_flip.seqno = obj->last_write_seqno;
-	intel_crtc->mmio_flip.ring_id = obj->ring->id;
-	spin_unlock_irq(&dev_priv->mmio_flip_lock);
+	intel_crtc->mmio_flip.ring = obj->ring;
+
+	schedule_work(&intel_crtc->mmio_flip.work);
 
-	/*
-	 * Double check to catch cases where irq fired before
-	 * mmio flip data was ready
-	 */
-	intel_notify_mmio_flip(obj->ring);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c8ddde6..492d346 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -399,16 +399,9 @@ struct intel_pipe_wm {
 	bool sprites_scaled;
 };
 
-enum intel_mmio_flip_status {
-	INTEL_MMIO_FLIP_IDLE = 0,
-	INTEL_MMIO_FLIP_WAIT_RING,
-	INTEL_MMIO_FLIP_WORK_SCHEDULED,
-};
-
 struct intel_mmio_flip {
 	u32 seqno;
-	u32 ring_id;
-	enum intel_mmio_flip_status status;
+	struct intel_engine_cs *ring;
 	struct work_struct work;
 };
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Make mmio flip wait for seqno in the work function

[Chris Wilson]

On Thu, Nov 06, 2014 at 09:26:39AM +0200, Ander Conselvan de Oliveira wrote:
> @@ -9424,73 +9424,24 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>  
>  	if (atomic_update)
>  		intel_pipe_update_end(intel_crtc, start_vbl_count);
> -
> -	spin_lock_irq(&dev_priv->mmio_flip_lock);
> -	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE;
> -	spin_unlock_irq(&dev_priv->mmio_flip_lock);
>  }
>  
>  static void intel_mmio_flip_work_func(struct work_struct *work)
>  {
>  	struct intel_crtc *intel_crtc =
>  		container_of(work, struct intel_crtc, mmio_flip.work);
> -
> -	intel_do_mmio_flip(intel_crtc);
> -}
> -
> -static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> -{
>  	struct intel_engine_cs *ring;
> -	int ret;
> -
> -	lockdep_assert_held(&obj->base.dev->struct_mutex);
> -
> -	if (!obj->last_write_seqno)
> -		return 0;
> -
> -	ring = obj->ring;
> -
> -	if (i915_seqno_passed(ring->get_seqno(ring, true),
> -			      obj->last_write_seqno))
> -		return 0;
> -
> -	ret = i915_gem_check_olr(ring, obj->last_write_seqno);
> -	if (ret)
> -		return ret;
> -
> -	if (WARN_ON(!ring->irq_get(ring)))
> -		return 0;
> -
> -	return 1;
> -}
> +	uint32_t seqno;
>  
> -void intel_notify_mmio_flip(struct intel_engine_cs *ring)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> -	struct intel_crtc *intel_crtc;
> -	unsigned long irq_flags;
> -	u32 seqno;
> -
> -	seqno = ring->get_seqno(ring, false);
> +	seqno = intel_crtc->mmio_flip.seqno;
> +	ring = intel_crtc->mmio_flip.ring;
>  
> -	spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
> -	for_each_intel_crtc(ring->dev, intel_crtc) {
> -		struct intel_mmio_flip *mmio_flip;
> +	if (seqno)
> +		WARN_ON(__i915_wait_seqno(ring, seqno,
> +					  intel_crtc->reset_counter,
> +					  true, NULL, NULL) != 0);

interruptible needs to be false

That's the only thing I spotted wrong.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Ander Conselvan de Oliveira]

This simplifies the code quite a bit compared to iterating over all
rings during the ring interrupt.

Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
struct is only accessed in two places. The first is when the flip is
queued and the other when the mmio writes are done. Since a flip cannot
be queued while there is a pending flip, the two paths shouldn't ever
run in parallel. We might need to revisit that if support for replacing
flips is implemented though.

v2: Don't hold dev->struct_mutext while waiting (Chris)

v3: Make the wait uninterruptable (Chris)

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  3 --
 drivers/gpu/drm/i915/intel_display.c | 90 +++++-------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  9 +---
 3 files changed, 12 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 318a6a0..5fff287 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -979,9 +979,6 @@ static void notify_ring(struct drm_device *dev,
 
 	trace_i915_gem_request_complete(ring);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		intel_notify_mmio_flip(ring);
-
 	wake_up_all(&ring->irq_queue);
 	i915_queue_hangcheck(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f3ff8e8..e317e39 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9424,73 +9424,24 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
-
-	spin_lock_irq(&dev_priv->mmio_flip_lock);
-	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE;
-	spin_unlock_irq(&dev_priv->mmio_flip_lock);
 }
 
 static void intel_mmio_flip_work_func(struct work_struct *work)
 {
 	struct intel_crtc *intel_crtc =
 		container_of(work, struct intel_crtc, mmio_flip.work);
-
-	intel_do_mmio_flip(intel_crtc);
-}
-
-static int intel_postpone_flip(struct drm_i915_gem_object *obj)
-{
 	struct intel_engine_cs *ring;
-	int ret;
-
-	lockdep_assert_held(&obj->base.dev->struct_mutex);
-
-	if (!obj->last_write_seqno)
-		return 0;
-
-	ring = obj->ring;
-
-	if (i915_seqno_passed(ring->get_seqno(ring, true),
-			      obj->last_write_seqno))
-		return 0;
-
-	ret = i915_gem_check_olr(ring, obj->last_write_seqno);
-	if (ret)
-		return ret;
-
-	if (WARN_ON(!ring->irq_get(ring)))
-		return 0;
-
-	return 1;
-}
+	uint32_t seqno;
 
-void intel_notify_mmio_flip(struct intel_engine_cs *ring)
-{
-	struct drm_i915_private *dev_priv = to_i915(ring->dev);
-	struct intel_crtc *intel_crtc;
-	unsigned long irq_flags;
-	u32 seqno;
-
-	seqno = ring->get_seqno(ring, false);
+	seqno = intel_crtc->mmio_flip.seqno;
+	ring = intel_crtc->mmio_flip.ring;
 
-	spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
-	for_each_intel_crtc(ring->dev, intel_crtc) {
-		struct intel_mmio_flip *mmio_flip;
+	if (seqno)
+		WARN_ON(__i915_wait_seqno(ring, seqno,
+					  intel_crtc->reset_counter,
+					  false, NULL, NULL) != 0);
 
-		mmio_flip = &intel_crtc->mmio_flip;
-		if (mmio_flip->status != INTEL_MMIO_FLIP_WAIT_RING)
-			continue;
-
-		if (ring->id != mmio_flip->ring_id)
-			continue;
-
-		if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
-			schedule_work(&intel_crtc->mmio_flip.work);
-			mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED;
-			ring->irq_put(ring);
-		}
-	}
-	spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
+	intel_do_mmio_flip(intel_crtc);
 }
 
 static int intel_queue_mmio_flip(struct drm_device *dev,
@@ -9500,32 +9451,13 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
 				 struct intel_engine_cs *ring,
 				 uint32_t flags)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
-		return -EBUSY;
-
-	ret = intel_postpone_flip(obj);
-	if (ret < 0)
-		return ret;
-	if (ret == 0) {
-		intel_do_mmio_flip(intel_crtc);
-		return 0;
-	}
 
-	spin_lock_irq(&dev_priv->mmio_flip_lock);
-	intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_WAIT_RING;
 	intel_crtc->mmio_flip.seqno = obj->last_write_seqno;
-	intel_crtc->mmio_flip.ring_id = obj->ring->id;
-	spin_unlock_irq(&dev_priv->mmio_flip_lock);
+	intel_crtc->mmio_flip.ring = obj->ring;
+
+	schedule_work(&intel_crtc->mmio_flip.work);
 
-	/*
-	 * Double check to catch cases where irq fired before
-	 * mmio flip data was ready
-	 */
-	intel_notify_mmio_flip(obj->ring);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c8ddde6..492d346 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -399,16 +399,9 @@ struct intel_pipe_wm {
 	bool sprites_scaled;
 };
 
-enum intel_mmio_flip_status {
-	INTEL_MMIO_FLIP_IDLE = 0,
-	INTEL_MMIO_FLIP_WAIT_RING,
-	INTEL_MMIO_FLIP_WORK_SCHEDULED,
-};
-
 struct intel_mmio_flip {
 	u32 seqno;
-	u32 ring_id;
-	enum intel_mmio_flip_status status;
+	struct intel_engine_cs *ring;
 	struct work_struct work;
 };
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Chris Wilson]

On Thu, Nov 06, 2014 at 11:03:40AM +0200, Ander Conselvan de Oliveira wrote:
> This simplifies the code quite a bit compared to iterating over all
> rings during the ring interrupt.
> 
> Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
> struct is only accessed in two places. The first is when the flip is
> queued and the other when the mmio writes are done. Since a flip cannot
> be queued while there is a pending flip, the two paths shouldn't ever
> run in parallel. We might need to revisit that if support for replacing
> flips is implemented though.
> 
> v2: Don't hold dev->struct_mutext while waiting (Chris)
> 
> v3: Make the wait uninterruptable (Chris)
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> +		WARN_ON(__i915_wait_seqno(ring, seqno,
> +					  intel_crtc->reset_counter,
> +					  false, NULL, NULL) != 0);

Should probably mention the caveat that this wants RPS boost tweaks,
which have been posted to the list as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Daniel Vetter]

On Thu, Nov 06, 2014 at 09:25:22AM +0000, Chris Wilson wrote:
> On Thu, Nov 06, 2014 at 11:03:40AM +0200, Ander Conselvan de Oliveira wrote:
> > This simplifies the code quite a bit compared to iterating over all
> > rings during the ring interrupt.
> > 
> > Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
> > struct is only accessed in two places. The first is when the flip is
> > queued and the other when the mmio writes are done. Since a flip cannot
> > be queued while there is a pending flip, the two paths shouldn't ever
> > run in parallel. We might need to revisit that if support for replacing
> > flips is implemented though.
> > 
> > v2: Don't hold dev->struct_mutext while waiting (Chris)
> > 
> > v3: Make the wait uninterruptable (Chris)

Can we actually send singals to kworker threads? Just out of curiosity ...

> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
 
> > +		WARN_ON(__i915_wait_seqno(ring, seqno,
> > +					  intel_crtc->reset_counter,
> > +					  false, NULL, NULL) != 0);
> 
> Should probably mention the caveat that this wants RPS boost tweaks,
> which have been posted to the list as well.

Yeah I guess we don't want to boost here by default (since userspace might
send the pageflip right after having queued the pageflip), but only when
we indeed missed the next vblank. So I guess we should disable the
boosting here and get your pageflip booster in. Can you please
rebase/adapt and Ander could perhaps review?
-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: [PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Chris Wilson]

On Thu, Nov 06, 2014 at 02:53:09PM +0100, Daniel Vetter wrote:
> On Thu, Nov 06, 2014 at 09:25:22AM +0000, Chris Wilson wrote:
> > On Thu, Nov 06, 2014 at 11:03:40AM +0200, Ander Conselvan de Oliveira wrote:
> > > This simplifies the code quite a bit compared to iterating over all
> > > rings during the ring interrupt.
> > > 
> > > Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
> > > struct is only accessed in two places. The first is when the flip is
> > > queued and the other when the mmio writes are done. Since a flip cannot
> > > be queued while there is a pending flip, the two paths shouldn't ever
> > > run in parallel. We might need to revisit that if support for replacing
> > > flips is implemented though.
> > > 
> > > v2: Don't hold dev->struct_mutext while waiting (Chris)
> > > 
> > > v3: Make the wait uninterruptable (Chris)
> 
> Can we actually send singals to kworker threads? Just out of curiosity ...

Some, at least. Probably not the system workqueues we use here, but
interruptible=false also has the semantics of "no errors. please".
Definitely useful here.

> > > +		WARN_ON(__i915_wait_seqno(ring, seqno,
> > > +					  intel_crtc->reset_counter,
> > > +					  false, NULL, NULL) != 0);
> > 
> > Should probably mention the caveat that this wants RPS boost tweaks,
> > which have been posted to the list as well.
> 
> Yeah I guess we don't want to boost here by default (since userspace might
> send the pageflip right after having queued the pageflip), but only when
> we indeed missed the next vblank. So I guess we should disable the
> boosting here and get your pageflip booster in. Can you please
> rebase/adapt and Ander could perhaps review?

Yes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx