| Message ID | 1376118364-2232-1-git-send-email-gclement@baobob.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
> >> 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
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
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
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
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
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,
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(-)