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

[ykzhao <yakui.zhao@intel.com>]

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