[PATCH] gma500: Fix SDVO turning off randomly

[PATCH] gma500: Fix SDVO turning off randomly

[Guillaume Clement]

Some Poulsbo cards seem to incorrectly report SDVO_CMD_STATUS_TARGET_NOT_SPECIFIED instead of SDVO_CMD_STATUS_PENDING, which causes the display to be turned off.

Signed-off-by: Guillaume Clement <gclement@baobob.org>
---
 drivers/gpu/drm/gma500/psb_intel_sdvo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
index 77841a1..6f01cdf 100644
--- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
+++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
@@ -500,7 +500,8 @@ static bool psb_intel_sdvo_read_response(struct psb_intel_sdvo *psb_intel_sdvo,
 				  &status))
 		goto log_fail;
 
-	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
+	while ((status == SDVO_CMD_STATUS_PENDING ||
+		status == SDVO_CMD_STATUS_TARGET_NOT_SPECIFIED) && retry--) {
 		udelay(15);
 		if (!psb_intel_sdvo_read_byte(psb_intel_sdvo,
 					  SDVO_I2C_CMD_STATUS,
-- 
1.8.1.5

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Daniel Vetter]

On Sat, Aug 10, 2013 at 9:06 AM, Guillaume Clement <gclement@baobob.org> wrote:
> Some Poulsbo cards seem to incorrectly report SDVO_CMD_STATUS_TARGET_NOT_SPECIFIED instead of SDVO_CMD_STATUS_PENDING, which causes the display to be turned off.
>
> Signed-off-by: Guillaume Clement <gclement@baobob.org>


Can you please submit the exact same change for drm/i915/intel_sdvoc.c
too? The sdvo encoders can be the same ones for psb and i915 so having
the same code would be great. Separate patch for drm/i915 ofc.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Chris Wilson]

On Sat, Aug 10, 2013 at 10:55:08AM +0200, Daniel Vetter wrote:
> On Sat, Aug 10, 2013 at 9:06 AM, Guillaume Clement <gclement@baobob.org> wrote:
> > Some Poulsbo cards seem to incorrectly report SDVO_CMD_STATUS_TARGET_NOT_SPECIFIED instead of SDVO_CMD_STATUS_PENDING, which causes the display to be turned off.
> >
> > Signed-off-by: Guillaume Clement <gclement@baobob.org>
> 
> 
> Can you please submit the exact same change for drm/i915/intel_sdvoc.c
> too? The sdvo encoders can be the same ones for psb and i915 so having
> the same code would be great. Separate patch for drm/i915 ofc.

Are you absolutely sure that is a hardware error and not a software SDVO
protocol goof? TARGET_NOT_SPECIFIED seems pretty specific.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Daniel Vetter]

On Sat, Aug 10, 2013 at 11:19 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, Aug 10, 2013 at 10:55:08AM +0200, Daniel Vetter wrote:
>> On Sat, Aug 10, 2013 at 9:06 AM, Guillaume Clement <gclement@baobob.org> wrote:
>> > Some Poulsbo cards seem to incorrectly report SDVO_CMD_STATUS_TARGET_NOT_SPECIFIED instead of SDVO_CMD_STATUS_PENDING, which causes the display to be turned off.
>> >
>> > Signed-off-by: Guillaume Clement <gclement@baobob.org>
>>
>>
>> Can you please submit the exact same change for drm/i915/intel_sdvoc.c
>> too? The sdvo encoders can be the same ones for psb and i915 so having
>> the same code would be great. Separate patch for drm/i915 ofc.
>
> Are you absolutely sure that is a hardware error and not a software SDVO
> protocol goof? TARGET_NOT_SPECIFIED seems pretty specific.

No idea, but since the code still retries a limited amount and then
still fails I can imagine that a few sdvo encoders just have a hard
time waking up and responding with something useful. In all cases we
log the actual return value and everything interesting.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Guillaume CLÉMENT]

> >> Can you please submit the exact same change for drm/i915/intel_sdvoc.c
> >> too? The sdvo encoders can be the same ones for psb and i915 so having
> >> the same code would be great. Separate patch for drm/i915 ofc.
> >
> > Are you absolutely sure that is a hardware error and not a software SDVO
> > protocol goof? TARGET_NOT_SPECIFIED seems pretty specific.
>
> No idea, but since the code still retries a limited amount and then
> still fails I can imagine that a few sdvo encoders just have a hard
> time waking up and responding with something useful. In all cases we
> log the actual return value and everything interesting.

I also have no idea if we're actually fixing a symptom instead of a
real unrelated problem. What I know is that when I first reported this
problem last year, Alan Cox told me he had received reports of such
problems before, so this affects other Poulsbo users at least.


But I can't know for sure that it's related to hardware or the driver.



Either way, this patch can't make harm as far as I can see, but I
cannot test it as I don't have the required hardware.


I'll be Submitting the updated patch set.


- Guillaume

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Patrik Jakobsson]

On Sat, Aug 10, 2013 at 9:40 PM, Guillaume CLÉMENT <gclement@baobob.org> wrote:
>> >> Can you please submit the exact same change for drm/i915/intel_sdvoc.c
>> >> too? The sdvo encoders can be the same ones for psb and i915 so having
>> >> the same code would be great. Separate patch for drm/i915 ofc.
>> >
>> > Are you absolutely sure that is a hardware error and not a software SDVO
>> > protocol goof? TARGET_NOT_SPECIFIED seems pretty specific.
>>
>> No idea, but since the code still retries a limited amount and then
>> still fails I can imagine that a few sdvo encoders just have a hard
>> time waking up and responding with something useful. In all cases we
>> log the actual return value and everything interesting.
>
> I also have no idea if we're actually fixing a symptom instead of a
> real unrelated problem. What I know is that when I first reported this
> problem last year, Alan Cox told me he had received reports of such
> problems before, so this affects other Poulsbo users at least.
>
>
> But I can't know for sure that it's related to hardware or the driver.
>
>
>
> Either way, this patch can't make harm as far as I can see, but I
> cannot test it as I don't have the required hardware.
>
>
> I'll be Submitting the updated patch set.

Thanks for the patch

I will give this a spin on my gma500 and i915 hardware on monday. The gma500
sdvo code should be pretty much identical to i915 from around 2011 but I guess
we've diverged since then. I'll also rework sdvo for gma500 a little in the
coming days when I start working on Minnowboard support so I'll probably compare
it to i915 for any peculiarities.

-Patrik

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Daniel Vetter]

On Sat, Aug 10, 2013 at 10:12 PM, Patrik Jakobsson
<patrik.r.jakobsson@gmail.com> wrote:
> I will give this a spin on my gma500 and i915 hardware on monday. The gma500
> sdvo code should be pretty much identical to i915 from around 2011 but I guess
> we've diverged since then. I'll also rework sdvo for gma500 a little in the
> coming days when I start working on Minnowboard support so I'll probably compare
> it to i915 for any peculiarities.

If you take this opportunity to share a bit of code between gma500 and
i915 (the sdvo #defines header and maybe the sdvo i2c read/write and
cmd helpers) I'd be interested in such a patch. We probably need an
struct intel_sdvo_chip to abstract away a few things.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Patrik Jakobsson]

On Sun, Aug 11, 2013 at 12:00 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Aug 10, 2013 at 10:12 PM, Patrik Jakobsson
> <patrik.r.jakobsson@gmail.com> wrote:
>> I will give this a spin on my gma500 and i915 hardware on monday. The gma500
>> sdvo code should be pretty much identical to i915 from around 2011 but I guess
>> we've diverged since then. I'll also rework sdvo for gma500 a little in the
>> coming days when I start working on Minnowboard support so I'll probably compare
>> it to i915 for any peculiarities.

Monday turned into friday, but it seems ok on my Poulsbo boxes.
I'll apply it to gma500-fixes

>
> If you take this opportunity to share a bit of code between gma500 and
> i915 (the sdvo #defines header and maybe the sdvo i2c read/write and
> cmd helpers) I'd be interested in such a patch. We probably need an
> struct intel_sdvo_chip to abstract away a few things.
> -Daniel

I'll take a look at it. Does i915 also have GMBUS and GPIOs spread out
on different
devices like the minnow? GPIOs seem to be accessed through LPC.

-Patrik

Re: [PATCH] gma500: Fix SDVO turning off randomly

[Daniel Vetter]

On Fri, Aug 16, 2013 at 02:23:35AM +0200, Patrik Jakobsson wrote:
> On Sun, Aug 11, 2013 at 12:00 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sat, Aug 10, 2013 at 10:12 PM, Patrik Jakobsson
> > <patrik.r.jakobsson@gmail.com> wrote:
> > If you take this opportunity to share a bit of code between gma500 and
> > i915 (the sdvo #defines header and maybe the sdvo i2c read/write and
> > cmd helpers) I'd be interested in such a patch. We probably need an
> > struct intel_sdvo_chip to abstract away a few things.
> > -Daniel
> 
> I'll take a look at it. Does i915 also have GMBUS and GPIOs spread out
> on different
> devices like the minnow? GPIOs seem to be accessed through LPC.

Hm, we always have the intel gmbus controller, but at different offsets.
In any case the abstraction provided by i2c_adapter should be more than
enough I hope. In drm/i915 we always use the bit-banging i2c encoder, but
such quirks are all handled within the i2c_adapter we pass to the
low-level sdvo i/o functions.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch