Re: [PATCH 1/4] drm/i915: stop using is_cpu_edp() in intel_disable/post_disable_dp

[Imre Deak <imre.deak@intel.com>]

On Tue, 2013-05-21 at 13:29 +0300, Imre Deak wrote:
> On Tue, 2013-05-21 at 11:15 +0200, Daniel Vetter wrote:
> > On Thu, May 16, 2013 at 02:40:34PM +0300, Imre Deak wrote:
> > > On port A and for Valleyview on port C we can have only eDP and in both
> > > cases it's a CPU port. So we can replace is_cpu_edp() with a port check
> > > for these two cases. This allows us to remove is_cpu_edp() completely in
> > > a later patch.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c |    7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 4dae01a..90ae58a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1350,6 +1350,8 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
> > >  static void intel_disable_dp(struct intel_encoder *encoder)
> > >  {
> > >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > > +	enum port port = dp_to_dig_port(intel_dp)->port;
> > > +	struct drm_device *dev = encoder->base.dev;
> > >  
> > >  	/* Make sure the panel is off before trying to change the mode. But also
> > >  	 * ensure that we have vdd while we switch off the panel. */
> > > @@ -1359,16 +1361,17 @@ static void intel_disable_dp(struct intel_encoder *encoder)
> > >  	ironlake_edp_panel_off(intel_dp);
> > >  
> > >  	/* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */
> > > -	if (!is_cpu_edp(intel_dp))
> > > +	if (port != PORT_A && (port != PORT_C || !IS_VALLEYVIEW(dev)))
> > 
> > I think this would read easier as port !(port == A || IS_VLV). I think
> > that should also be more correct since there's no reason (besides
> > hard-coding) that DP on port B (or external DP fwiw) should work different
> > on vlv.
> 
> I don't have the VLV spec to verify this, so this is just keeping the
> current behavior. I'll try to get the spec and follow up on this.

Ok, I dig around in git history and IVB,VLV bspec to understand this
better. Commit "drm/i915: disable the cpu edp port after the cpu pipe"
you add the above distinction in the disable sequence. From that and the
cited bspec modeset sequence, it seems we have to distinguish between
the CPU and PCH cases and whether it's eDP or DP doesn't seem to play a
role.

Since on VLV we don't have a PCH we should always follow there the
sequence we do on other platforms for the CPU case. Based on this I
agree with the simplification you give above and I will follow up with a
new version with that.

Contradictory to the above though, from the VLV display spec 37.2 Power
down sequence, it seems the sequence has to be always disable port,
disable planes, disable pipe. So that would mean port != A || IS_VLV
above. But I suggest handling this in a follow up patch.

--Imre