[PATCH] drm/i915: increase the SWSCI DSLP default timeout to 50ms

[PATCH] drm/i915: increase the SWSCI DSLP default timeout to 50ms

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The spec says the default timeout should be 2ms, but on my machine
this doesn't seem to be enough. Sometimes it works, sometimes I get
these messages when booting:
  - SWSCI request timed out
  - SWSCI request already in progress

And my guess is that the "already in progress" message is because the
first one is still happening.

I did some experiments on my machine (that has CONFIG_HZ=1000) and the
wait_for function usually takes 4-6 jiffies to finish, but I've seen
up to 9. So increase the timeout to 50ms. We only expect to wait for
the actual amount of time the operation takes, so even a huge timeout
shouldn't delay us more than what the hardware actually requires.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 2acf5ca..9044640 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -258,7 +258,9 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
 	/* Driver sleep timeout in ms. */
 	dslp = ioread32(&swsci->dslp);
 	if (!dslp) {
-		dslp = 2;
+		/* The spec says 2ms should be the default, but it's too small
+		 * for some machines. */
+		dslp = 50;
 	} else if (dslp > 500) {
 		/* Hey bios, trust must be earned. */
 		WARN_ONCE(1, "excessive driver sleep timeout (DSPL) %u\n", dslp);
-- 
1.8.3.1

Re: [PATCH] drm/i915: increase the SWSCI DSLP default timeout to 50ms

[Jani Nikula]

On Wed, 09 Oct 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The spec says the default timeout should be 2ms, but on my machine
> this doesn't seem to be enough. Sometimes it works, sometimes I get
> these messages when booting:
>   - SWSCI request timed out

BTW if this happens, and BIOS missed our call, will anyone clear
SWSCI_SCIC_INDICATOR? If not, we're bound to keep hitting:

>   - SWSCI request already in progress

So maybe we should just clear SWSCI_SCIC_INDICATOR ourselves on timeout?
Would that help?

In any case, this patch is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> And my guess is that the "already in progress" message is because the
> first one is still happening.
>
> I did some experiments on my machine (that has CONFIG_HZ=1000) and the
> wait_for function usually takes 4-6 jiffies to finish, but I've seen
> up to 9. So increase the timeout to 50ms. We only expect to wait for
> the actual amount of time the operation takes, so even a huge timeout
> shouldn't delay us more than what the hardware actually requires.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 2acf5ca..9044640 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -258,7 +258,9 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>  	/* Driver sleep timeout in ms. */
>  	dslp = ioread32(&swsci->dslp);
>  	if (!dslp) {
> -		dslp = 2;
> +		/* The spec says 2ms should be the default, but it's too small
> +		 * for some machines. */
> +		dslp = 50;
>  	} else if (dslp > 500) {
>  		/* Hey bios, trust must be earned. */
>  		WARN_ONCE(1, "excessive driver sleep timeout (DSPL) %u\n", dslp);
> -- 
> 1.8.3.1
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: increase the SWSCI DSLP default timeout to 50ms

[Paulo Zanoni]

2013/10/10 Jani Nikula <jani.nikula@intel.com>:
> On Wed, 09 Oct 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> The spec says the default timeout should be 2ms, but on my machine
>> this doesn't seem to be enough. Sometimes it works, sometimes I get
>> these messages when booting:
>>   - SWSCI request timed out
>
> BTW if this happens, and BIOS missed our call, will anyone clear
> SWSCI_SCIC_INDICATOR? If not, we're bound to keep hitting:
>
>>   - SWSCI request already in progress
>
> So maybe we should just clear SWSCI_SCIC_INDICATOR ourselves on timeout?
> Would that help?
>

We definitely should do something in case we get a timeout. I don't
really know the best solution here, I need to study the problem. I
just tried to solve the immediate problem of "make my machine work" :)
I also think we should be careful to not mess with someone else's operation.

Also, I think this problem is kinda "orthogonal" to increasing the
timeout (although increasing the timeout hides it, hopefully forever).
I added this to my TODO list, but I don't guarantee I will fix it in
the next weeks.


> In any case, this patch is
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>


Thanks for the review :)

>
>
>> And my guess is that the "already in progress" message is because the
>> first one is still happening.
>>
>> I did some experiments on my machine (that has CONFIG_HZ=1000) and the
>> wait_for function usually takes 4-6 jiffies to finish, but I've seen
>> up to 9. So increase the timeout to 50ms. We only expect to wait for
>> the actual amount of time the operation takes, so even a huge timeout
>> shouldn't delay us more than what the hardware actually requires.
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_opregion.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 2acf5ca..9044640 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -258,7 +258,9 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out)
>>       /* Driver sleep timeout in ms. */
>>       dslp = ioread32(&swsci->dslp);
>>       if (!dslp) {
>> -             dslp = 2;
>> +             /* The spec says 2ms should be the default, but it's too small
>> +              * for some machines. */
>> +             dslp = 50;
>>       } else if (dslp > 500) {
>>               /* Hey bios, trust must be earned. */
>>               WARN_ONCE(1, "excessive driver sleep timeout (DSPL) %u\n", dslp);
>> --
>> 1.8.3.1
>>
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: increase the SWSCI DSLP default timeout to 50ms

[Daniel Vetter]

On Fri, Oct 11, 2013 at 05:29:17PM -0300, Paulo Zanoni wrote:
> 2013/10/10 Jani Nikula <jani.nikula@intel.com>:
> > On Wed, 09 Oct 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> The spec says the default timeout should be 2ms, but on my machine
> >> this doesn't seem to be enough. Sometimes it works, sometimes I get
> >> these messages when booting:
> >>   - SWSCI request timed out
> >
> > BTW if this happens, and BIOS missed our call, will anyone clear
> > SWSCI_SCIC_INDICATOR? If not, we're bound to keep hitting:
> >
> >>   - SWSCI request already in progress
> >
> > So maybe we should just clear SWSCI_SCIC_INDICATOR ourselves on timeout?
> > Would that help?
> >
> 
> We definitely should do something in case we get a timeout. I don't
> really know the best solution here, I need to study the problem. I
> just tried to solve the immediate problem of "make my machine work" :)
> I also think we should be careful to not mess with someone else's operation.
> 
> Also, I think this problem is kinda "orthogonal" to increasing the
> timeout (although increasing the timeout hides it, hopefully forever).
> I added this to my TODO list, but I don't guarantee I will fix it in
> the next weeks.
> 
> 
> > In any case, this patch is
> >
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> Thanks for the review :)

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch