Re: [PATCH 1/4] drm/i915: Configure the TV sense state correctly on GM45 to make TV detection reliable

Re: [PATCH 1/4] drm/i915: Configure the TV sense state correctly on GM45 to make TV detection reliable

[ykzhao]

On Sat, 2010-04-10 at 05:14 +0800, Eric Anholt wrote:
> On Wed,  7 Apr 2010 17:11:19 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > From: ykzhao <yakui.zhao@intel.com>
> > 
> > The TV detection logic is not reliable on the Cantiga platform.
> > Sometimes the TV will be misdetected as the following two cases:
> > - TV is misdetected on some laptops. e.g. There is no TV connector
> > port or no TV is attached. But the TV is shown as connected.
> > - TV connector type is misdetected. e.g. the component TV is
> > attached, but the TV is shown as S-video type.
> > 
> > According to the hardware requirement, the TV sense state bits of TV DAC
> > register should be cleared to zero on Cantiga platfrom.
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=14792
> 
> As far as I can tell from reading the specs, this patch just completely
> breaks TV detect on GM45.  And the logic of "set all the bits for the
> register setting we're going to do and then if it's gm45 skip a few of
> them" is unintuitive and backwards, even if it was correct.\\

This is from bios code. And the bios team also helps to confirm that TV
DAC sense state bits should be cleared to zero on cantiga platform.

How about the following commit log?

   The TV detection logic is not reliable on the cantiga platform. Sometimes the
TV will be misdetected as the following two cases:
   a. TV is misdetected on some laptops. e.g. There is no TV connector
port or no TV is attaced. But the TV is shown as connected.
   b. TV connector type is misdetected. E.g. the component TV is
attached, but the TV is shown as S-video type.

It is confirmed that in bios code the TV DAC sense bits should be cleared
to zero on GM45 in course of TV detection, which is different with other
platforms. It uses the following conditional definition:
  IF   CTG
    TVDAC_SENSE_CTL EQU 0               ; Cantiga to Low level
  ELSE
    TVDAC_SENSE_CTL EQU BIT27 + BIT26 + BIT25 + BIT24
  ENDIF         ; CTG

https://bugzilla.kernel.org/show_bug.cgi?id=14792

> 
> You don't get to just say "according to the hardware requirement."  You
> need to refer to either the b-spec (so someone can confirm what you
> read) or BIOS traces (so someone knows how you did it) or to a personal
> conversation with a HW engineer (so people can be appropriately
> dubious), or to randomly shuffling bits of the registers until people
> reported that their bug was fixed, as this patch appears to do.

Re: [PATCH 1/4] drm/i915: Configure the TV sense state correctly on GM45 to make TV detection reliable

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 2223 bytes --]

On Wed, 2010-04-21 at 16:46 +0800, ykzhao wrote:
> On Sat, 2010-04-10 at 05:14 +0800, Eric Anholt wrote:
> > As far as I can tell from reading the specs, this patch just completely
> > breaks TV detect on GM45.  And the logic of "set all the bits for the
> > register setting we're going to do and then if it's gm45 skip a few of
> > them" is unintuitive and backwards, even if it was correct.\\
> 
> This is from bios code. And the bios team also helps to confirm that TV
> DAC sense state bits should be cleared to zero on cantiga platform.

Uh.  The patch does this:

> +       if (IS_GM45(dev))
> +               tv_dac &= ~(TVDAC_STATE_CHG_EN | TVDAC_A_SENSE_CTL |
> +                           TVDAC_B_SENSE_CTL | TVDAC_C_SENSE_CTL);
> +

TVDAC_STATE_CHG_EN is documented as:

> TVDAC_STATE_CHG_EN : Enables DAC state change detection logic
>  0 = disabled
>  1 = enabled

You mean to tell me that, on Cantiga, _disabling_ the state change
detection logic, enables state change detection?

> How about the following commit log?
> 
>    The TV detection logic is not reliable on the cantiga platform. Sometimes the
> TV will be misdetected as the following two cases:
>    a. TV is misdetected on some laptops. e.g. There is no TV connector
> port or no TV is attaced. But the TV is shown as connected.
>    b. TV connector type is misdetected. E.g. the component TV is
> attached, but the TV is shown as S-video type.
> 
> It is confirmed that in bios code the TV DAC sense bits should be cleared
> to zero on GM45 in course of TV detection, which is different with other
> platforms. It uses the following conditional definition:
>   IF   CTG
>     TVDAC_SENSE_CTL EQU 0               ; Cantiga to Low level
>   ELSE
>     TVDAC_SENSE_CTL EQU BIT27 + BIT26 + BIT25 + BIT24
>   ENDIF         ; CTG

If the BIOS clears all those bits in TV_DAC, _then_ enables bit 27, then
this seems like a reasonable thing to do.  That would mean that the
detection logic is basically one shot, and needs to be completely reset
between uses.

But I really don't see how, short of a really really bad hardware bug,
turning a subsystem _off_ would make that subsystem work.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/4] drm/i915: Configure the TV sense state correctly on GM45 to make TV detection reliable

[Peter Clifton]

On Wed, 2010-04-21 at 11:09 -0400, Adam Jackson wrote:

> But I really don't see how, short of a really really bad hardware bug,
> turning a subsystem _off_ would make that subsystem work.

Not knowing how the HW works, I'm only guessing.. but the Intel driver
is using a polling based method to detect TV-out, not the state-change
interrupt based method.

It is (just) possible that the state-change detection logic is separate
to that used for polling the status, and the two are conflicting. Some
hardware guys at Intel will probably have to confirm this.


-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)

Re: [PATCH 1/4] drm/i915: Configure the TV sense state correctly on GM45 to make TV detection reliable

[ykzhao]

On Wed, 2010-04-21 at 23:09 +0800, Adam Jackson wrote:
> On Wed, 2010-04-21 at 16:46 +0800, ykzhao wrote:
> > On Sat, 2010-04-10 at 05:14 +0800, Eric Anholt wrote:
> > > As far as I can tell from reading the specs, this patch just completely
> > > breaks TV detect on GM45.  And the logic of "set all the bits for the
> > > register setting we're going to do and then if it's gm45 skip a few of
> > > them" is unintuitive and backwards, even if it was correct.\\
> > 
> > This is from bios code. And the bios team also helps to confirm that TV
> > DAC sense state bits should be cleared to zero on cantiga platform.
> 
> Uh.  The patch does this:
> 
> > +       if (IS_GM45(dev))
> > +               tv_dac &= ~(TVDAC_STATE_CHG_EN | TVDAC_A_SENSE_CTL |
> > +                           TVDAC_B_SENSE_CTL | TVDAC_C_SENSE_CTL);
> > +
> 
> TVDAC_STATE_CHG_EN is documented as:
> 
> > TVDAC_STATE_CHG_EN : Enables DAC state change detection logic
> >  0 = disabled
> >  1 = enabled
> 
> You mean to tell me that, on Cantiga, _disabling_ the state change
> detection logic, enables state change detection?

On the cantiga the TV sense state bits[27:24] are required to be zero,
which is inconsistent with the description on the document.

After consulting with the BIOS team, they confirm that we had better
follow up the BIOS code on cantiga.

> 
> > How about the following commit log?
> > 
> >    The TV detection logic is not reliable on the cantiga platform. Sometimes the
> > TV will be misdetected as the following two cases:
> >    a. TV is misdetected on some laptops. e.g. There is no TV connector
> > port or no TV is attaced. But the TV is shown as connected.
> >    b. TV connector type is misdetected. E.g. the component TV is
> > attached, but the TV is shown as S-video type.
> > 
> > It is confirmed that in bios code the TV DAC sense bits should be cleared
> > to zero on GM45 in course of TV detection, which is different with other
> > platforms. It uses the following conditional definition:
> >   IF   CTG
> >     TVDAC_SENSE_CTL EQU 0               ; Cantiga to Low level
> >   ELSE
> >     TVDAC_SENSE_CTL EQU BIT27 + BIT26 + BIT25 + BIT24
> >   ENDIF         ; CTG
> 
> If the BIOS clears all those bits in TV_DAC, _then_ enables bit 27, then
> this seems like a reasonable thing to do.  That would mean that the
> detection logic is basically one shot, and needs to be completely reset
> between uses.
> 
> But I really don't see how, short of a really really bad hardware bug,
> turning a subsystem _off_ would make that subsystem work.

> - ajax

Re: [PATCH 4/4] drm/i915: Ignore LVDS EDID when it is unavailabe or invalid

[ykzhao]

On Sat, 2010-04-10 at 05:22 +0800, Eric Anholt wrote:
> On Wed,  7 Apr 2010 17:11:22 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> > From: Zhao Yakui <yakui.zhao@intel.com>
> > 
> > This trys to shut up complains about invalid LVDS EDID during
> > mode probe, but uses fixed panel mode directly for panels with
> > broken EDID.
> > 
> > https://bugs.freedesktop.org/show_bug.cgi?id=23099
> > https://bugs.freedesktop.org/show_bug.cgi?id=26395
> > 
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h   |    2 ++
> >  drivers/gpu/drm/i915/intel_lvds.c |   13 +++++++++----
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b7cb4aa..6960849 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -611,6 +611,8 @@ typedef struct drm_i915_private {
> >  	/* Reclocking support */
> >  	bool render_reclock_avail;
> >  	bool lvds_downclock_avail;
> > +	/* indicate whether the LVDS EDID is OK */
> > +	bool lvds_edid_good;
> >  	/* indicates the reduced downclock for LVDS*/
> >  	int lvds_downclock;
> >  	struct work_struct idle_work;
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 3fa088c..8bbcb32 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -638,10 +638,12 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret = 0;
> >  
> > -	ret = intel_ddc_get_modes(intel_encoder);
> > +	if (dev_priv->lvds_edid_good) {
> > +		ret = intel_ddc_get_modes(intel_encoder);
> >  
> > -	if (ret)
> > -		return ret;
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	/* Didn't get an EDID, so
> >  	 * Set wide sync ranges so we get all modes
> > @@ -1064,7 +1066,10 @@ void intel_lvds_init(struct drm_device *dev)
> >  	 * Attempt to get the fixed panel mode from DDC.  Assume that the
> >  	 * preferred mode is the right one.
> >  	 */
> > -	intel_ddc_get_modes(intel_encoder);
> > +	dev_priv->lvds_edid_good = true;
> > +
> > +	if (!intel_ddc_get_modes(intel_encoder))
> > +		dev_priv->lvds_edid_good = false;
> 
> I've applied this one to for-linus, though I'm not a huge fan.  We
> shouldn't be re-probing DDC on LVDS panels anyway -- there shouldn't be
> any circumstance when it can change.

Yes. The edid won't be changed for the LVDS panel. 

Can we cache the probed EDID for LVDS panel if we can get the EDID for
LVDS panel? Then when we try to probe LVDS EDID next time, we use the
cached EDID and don't probe the EDID really.(And now the LID state is
not used for the LVDS detection any more).It seems that the feature of
cached LVDS EDID is already used in the meego kernel.

Is this idea ok?
    
thanks.