[PATCH 1/4] drm/i915: add msecs_to_jiffies_timeout to guarantee minimum duration

[PATCH 1/4] drm/i915: add msecs_to_jiffies_timeout to guarantee minimum duration

[Imre Deak]

We need this to avoid premature timeouts whenever scheduling a timeout
based on the current jiffies value. For an explanation see [1].
The following patches will take the helper into use.

Once the more generic solution proposed in the thread at [1] is accepted
this patch can be reverted while keeping the follow-up patches.

[1] http://marc.info/?l=linux-kernel&m=136854294730957&w=2

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 639ec0b..78b6c56 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1981,4 +1981,19 @@ static inline void __user *to_user_ptr(u64 address)
 	return (void __user *)(uintptr_t)address;
 }
 
+static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
+{
+	unsigned long j = msecs_to_jiffies(m);
+
+	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
+}
+
+static inline unsigned long
+timespec_to_jiffies_timeout(const struct timespec *value)
+{
+	unsigned long j = timespec_to_jiffies(value);
+
+	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
+}
+
 #endif
-- 
1.8.1.2

[PATCH 2/4] drm/i915: use msecs_to_jiffies_timeout instead of open coding the same

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 5d24503..98cd8535 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -228,7 +228,7 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 	 * need to wake up periodically and check that ourselves. */
 	I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);
 
-	for (i = 0; i < msecs_to_jiffies(50) + 1; i++) {
+	for (i = 0; i < msecs_to_jiffies_timeout(50); i++) {
 		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
 				TASK_UNINTERRUPTIBLE);
 
-- 
1.8.1.2

[PATCH 3/4] drm/i915: avoid premature timeouts in __wait_seqno()

[Imre Deak]

At the moment wait_event_timeout/wait_event_interruptible_timeout may
time out 1 jiffy too early, as the calculated expiry time is 1 less than
needed. Besides timing out too early this also means that the
calculation of the remaining time will be incorrect and we will pass a
non-zero remaining time to user space in case of a time out. This is one
reason for the following bugzilla report:

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64270

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a1a282c..2b51fa7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1003,7 +1003,7 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 		wait_forever = false;
 	}
 
-	timeout_jiffies = timespec_to_jiffies(&wait_time);
+	timeout_jiffies = timespec_to_jiffies_timeout(&wait_time);
 
 	if (WARN_ON(!ring->irq_get(ring)))
 		return -ENODEV;
-- 
1.8.1.2

[PATCH 4/4] drm/i915: avoid premature DP AUX timeouts

[Imre Deak]

During DP AUX communication we might time out 1 jiffy too early, because
the calculated expiry jiffy value is one less than needed.

This is only one reason for false DP AUX timeouts. For a complete
solution we also need the following fix, which is now queued for
mainline: http://marc.info/?l=linux-kernel&m=136748515710837&w=2

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64133

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 2 +-
 drivers/gpu/drm/i915/intel_i2c.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3426a61..8cdc3d7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -276,7 +276,7 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
 	if (has_aux_irq)
 		done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
-					  msecs_to_jiffies(10));
+					  msecs_to_jiffies_timeout(10));
 	else
 		done = wait_for_atomic(C, 10) == 0;
 	if (!done)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 98cd8535..639fe19 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -263,7 +263,8 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
 	/* Important: The hw handles only the first bit, so set only one! */
 	I915_WRITE(GMBUS4 + reg_offset, GMBUS_IDLE_EN);
 
-	ret = wait_event_timeout(dev_priv->gmbus_wait_queue, C, 10);
+	ret = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
+				 msecs_to_jiffies_timeout(10));
 
 	I915_WRITE(GMBUS4 + reg_offset, 0);
 
-- 
1.8.1.2

Re: [PATCH 2/4] drm/i915: use msecs_to_jiffies_timeout instead of open coding the same

[Daniel Vetter]

We have another one of these in the wait_for register wait macro in
intel_drv.h Can you please amend your patch with that fixed up, too?

Thanks, Daniel

On Tue, May 21, 2013 at 7:03 PM, Imre Deak <imre.deak@intel.com> wrote:
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 5d24503..98cd8535 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -228,7 +228,7 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
>          * need to wake up periodically and check that ourselves. */
>         I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);
>
> -       for (i = 0; i < msecs_to_jiffies(50) + 1; i++) {
> +       for (i = 0; i < msecs_to_jiffies_timeout(50); i++) {
>                 prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
>                                 TASK_UNINTERRUPTIBLE);
>
> --
> 1.8.1.2
>
> _______________________________________________
> 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

Re: [PATCH 2/4] drm/i915: use msecs_to_jiffies_timeout instead of open coding the same

[Imre Deak]

On Tue, 2013-05-21 at 19:20 +0200, Daniel Vetter wrote:
> We have another one of these in the wait_for register wait macro in
> intel_drv.h Can you please amend your patch with that fixed up, too?

I noticed it, but didn't change it since we don't need there the +1
adjustment to begin with. The time_after() check already makes sure we
wait at least MS amount. So I think we should remove +1 from there..

--Imre

Re: [PATCH 2/4] drm/i915: use msecs_to_jiffies_timeout instead of open coding the same

[Daniel Vetter]

On Tue, May 21, 2013 at 8:00 PM, Imre Deak <imre.deak@intel.com> wrote:
> On Tue, 2013-05-21 at 19:20 +0200, Daniel Vetter wrote:
>> We have another one of these in the wait_for register wait macro in
>> intel_drv.h Can you please amend your patch with that fixed up, too?
>
> I noticed it, but didn't change it since we don't need there the +1
> adjustment to begin with. The time_after() check already makes sure we
> wait at least MS amount. So I think we should remove +1 from there..

Hm, right. Looks like the important part of

commit 1d5bfac96f1e1856fbdb3f06679691e5b9c2ba8f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Mar 28 00:03:25 2013 +0100

    drm/i915: fix up _wait_for macro

was to just recheck the condition after the timeout expired. I agree
that this is a separate patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: add msecs_to_jiffies_timeout to guarantee minimum duration

[Jani Nikula]

On Tue, 21 May 2013, Imre Deak <imre.deak@intel.com> wrote:
> We need this to avoid premature timeouts whenever scheduling a timeout
> based on the current jiffies value. For an explanation see [1].
> The following patches will take the helper into use.
>
> Once the more generic solution proposed in the thread at [1] is accepted
> this patch can be reverted while keeping the follow-up patches.
>
> [1] http://marc.info/?l=linux-kernel&m=136854294730957&w=2

With the name clashes, what happens when the generic solution is merged?
Blows up the build? To avoid confusion with merging upstream and
potential stable backports, should we err on the safe side and rename
these? Or add suitable trickery to use the generic version when
available?

BR,
Jani.


>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 639ec0b..78b6c56 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1981,4 +1981,19 @@ static inline void __user *to_user_ptr(u64 address)
>  	return (void __user *)(uintptr_t)address;
>  }
>  
> +static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
> +{
> +	unsigned long j = msecs_to_jiffies(m);
> +
> +	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> +}
> +
> +static inline unsigned long
> +timespec_to_jiffies_timeout(const struct timespec *value)
> +{
> +	unsigned long j = timespec_to_jiffies(value);
> +
> +	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> +}
> +
>  #endif
> -- 
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: add msecs_to_jiffies_timeout to guarantee minimum duration

[Daniel Vetter]

On Wed, May 22, 2013 at 9:48 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 21 May 2013, Imre Deak <imre.deak@intel.com> wrote:
>> We need this to avoid premature timeouts whenever scheduling a timeout
>> based on the current jiffies value. For an explanation see [1].
>> The following patches will take the helper into use.
>>
>> Once the more generic solution proposed in the thread at [1] is accepted
>> this patch can be reverted while keeping the follow-up patches.
>>
>> [1] http://marc.info/?l=linux-kernel&m=136854294730957&w=2
>
> With the name clashes, what happens when the generic solution is merged?
> Blows up the build? To avoid confusion with merging upstream and
> potential stable backports, should we err on the safe side and rename
> these? Or add suitable trickery to use the generic version when
> available?

linux-next should catch such fallout and we can fix up things in the
merge. And since Linus has intel graphics I expect that he'll catch it
if it fails to compile ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: add msecs_to_jiffies_timeout to guarantee minimum duration

[Imre Deak]

On Wed, 2013-05-22 at 11:46 +0200, Daniel Vetter wrote:
> On Wed, May 22, 2013 at 9:48 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
> > On Tue, 21 May 2013, Imre Deak <imre.deak@intel.com> wrote:
> >> We need this to avoid premature timeouts whenever scheduling a timeout
> >> based on the current jiffies value. For an explanation see [1].
> >> The following patches will take the helper into use.
> >>
> >> Once the more generic solution proposed in the thread at [1] is accepted
> >> this patch can be reverted while keeping the follow-up patches.
> >>
> >> [1] http://marc.info/?l=linux-kernel&m=136854294730957&w=2
> >
> > With the name clashes, what happens when the generic solution is merged?
> > Blows up the build? To avoid confusion with merging upstream and
> > potential stable backports, should we err on the safe side and rename
> > these? Or add suitable trickery to use the generic version when
> > available?
> 
> linux-next should catch such fallout and we can fix up things in the
> merge. And since Linus has intel graphics I expect that he'll catch it
> if it fails to compile ;-)

Yea, I haven't thought this through properly. But after discussing with
Jani, this seems to be doable. I only hope Linus won't have to resolve
anything :)

--Imre

Re: [PATCH 1/4] drm/i915: add msecs_to_jiffies_timeout to guarantee minimum duration

[Daniel Vetter]

On Wed, May 22, 2013 at 12:57:01PM +0300, Imre Deak wrote:
> On Wed, 2013-05-22 at 11:46 +0200, Daniel Vetter wrote:
> > On Wed, May 22, 2013 at 9:48 AM, Jani Nikula
> > <jani.nikula@linux.intel.com> wrote:
> > > On Tue, 21 May 2013, Imre Deak <imre.deak@intel.com> wrote:
> > >> We need this to avoid premature timeouts whenever scheduling a timeout
> > >> based on the current jiffies value. For an explanation see [1].
> > >> The following patches will take the helper into use.
> > >>
> > >> Once the more generic solution proposed in the thread at [1] is accepted
> > >> this patch can be reverted while keeping the follow-up patches.
> > >>
> > >> [1] http://marc.info/?l=linux-kernel&m=136854294730957&w=2
> > >
> > > With the name clashes, what happens when the generic solution is merged?
> > > Blows up the build? To avoid confusion with merging upstream and
> > > potential stable backports, should we err on the safe side and rename
> > > these? Or add suitable trickery to use the generic version when
> > > available?
> > 
> > linux-next should catch such fallout and we can fix up things in the
> > merge. And since Linus has intel graphics I expect that he'll catch it
> > if it fails to compile ;-)
> 
> Yea, I haven't thought this through properly. But after discussing with
> Jani, this seems to be doable. I only hope Linus won't have to resolve
> anything :)

Ok, I've picked up the entire pile for -fixes, thanks for the patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch