[PATCH 0/4] drm/i915: remove is_cpu_edp()

[PATCH 0/4] drm/i915: remove is_cpu_edp()

[Imre Deak]

is_cpu_edp() is equivalent with a port==PORT_A check. There are two
exceptions (see patch 1 and 2), where we can rewrite things to handle
ValleyView separately as it's done at other places. With these out of
the way we can replace is_cpu_edp() with a simpler port check and
remove is_cpu_edp().

I tested this only on IVB, so before applying we should test it on VLV
at least.

Imre Deak (4):
  drm/i915: stop using is_cpu_edp() in intel_disable/post_disable_dp
  drm/i915: merge VLV eDP and DP AUX clock divider calculation
  drm/i915: replace is_cpu_edp() with a check for port A
  drm/i915: remove unused is_cpu_edp()

 drivers/gpu/drm/i915/intel_dp.c |   78 ++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 43 deletions(-)

-- 
1.7.10.4

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

[Imre Deak]

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)))
 		intel_dp_link_down(intel_dp);
 }
 
 static void intel_post_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;
 
-	if (is_cpu_edp(intel_dp)) {
+	if (port == PORT_A || (port == PORT_C && IS_VALLEYVIEW(dev))) {
 		intel_dp_link_down(intel_dp);
 		if (!IS_VALLEYVIEW(dev))
 			ironlake_edp_pll_off(intel_dp);
-- 
1.7.10.4

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

[Daniel Vetter]

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. 
-Daniel

>  		intel_dp_link_down(intel_dp);
>  }
>  
>  static void intel_post_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;
>  
> -	if (is_cpu_edp(intel_dp)) {
> +	if (port == PORT_A || (port == PORT_C && IS_VALLEYVIEW(dev))) {
>  		intel_dp_link_down(intel_dp);
>  		if (!IS_VALLEYVIEW(dev))
>  			ironlake_edp_pll_off(intel_dp);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Imre Deak]

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.

--Imre

> -Daniel
> 
> >  		intel_dp_link_down(intel_dp);
> >  }
> >  
> >  static void intel_post_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;
> >  
> > -	if (is_cpu_edp(intel_dp)) {
> > +	if (port == PORT_A || (port == PORT_C && IS_VALLEYVIEW(dev))) {
> >  		intel_dp_link_down(intel_dp);
> >  		if (!IS_VALLEYVIEW(dev))
> >  			ironlake_edp_pll_off(intel_dp);
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

[Imre Deak]

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

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

[Imre Deak]

Based on 3739850b46f - "drm/i915: disable the cpu edp port after the
cpu pipe" and the bspec disabling sequence for IVB and older it seems we
have to distinguish only the CPU vs. PCH port case, whether it's a DP or
eDP doesn't seem to matter. For IVB and older on the CPU side we can
only have eDP on port A, DP ports can only be on the PCH side. On VLV we
have only CPU side eDP/DP ports, no PCH. So the condition for the
disabling sequence we need for CPU ports is port == A || IS_VLV.

This allows us to remove is_cpu_edp() completely in a later patch.

v2:
- simplify (and fix) the condition for CPU side ports and adjust the
  commit message accordingly (Daniel)

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 c21fa4f..bd17887 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1372,6 +1372,8 @@ static void intel_dp_get_config(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. */
@@ -1381,16 +1383,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 || IS_VALLEYVIEW(dev)))
 		intel_dp_link_down(intel_dp);
 }
 
 static void intel_post_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;
 
-	if (is_cpu_edp(intel_dp)) {
+	if (port == PORT_A || IS_VALLEYVIEW(dev)) {
 		intel_dp_link_down(intel_dp);
 		if (!IS_VALLEYVIEW(dev))
 			ironlake_edp_pll_off(intel_dp);
-- 
1.8.1.2

[PATCH 2/4] drm/i915: merge VLV eDP and DP AUX clock divider calculation

[Imre Deak]

On ValleyView for both eDP and DP the AUX input clock is 200MHz, so we
can calculate for both the clock divider for the 2MHz target rate at the
same place. Afterwards we can also replace the is_cpu_edp() check with a
check for port A.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 90ae58a..b99da13 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -317,11 +317,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	 * Note that PCH attached eDP panels should use a 125MHz input
 	 * clock divider.
 	 */
-	if (is_cpu_edp(intel_dp)) {
+	if (IS_VALLEYVIEW(dev)) {
+		aux_clock_divider = 100;
+	} else if (intel_dig_port->port == PORT_A) {
 		if (HAS_DDI(dev))
 			aux_clock_divider = intel_ddi_get_cdclk_freq(dev_priv) >> 1;
-		else if (IS_VALLEYVIEW(dev))
-			aux_clock_divider = 100;
 		else if (IS_GEN6(dev) || IS_GEN7(dev))
 			aux_clock_divider = 200; /* SNB & IVB eDP input clock at 400Mhz */
 		else
-- 
1.7.10.4

Re: [PATCH 2/4] drm/i915: merge VLV eDP and DP AUX clock divider calculation

[Daniel Vetter]

On Thu, May 16, 2013 at 02:40:35PM +0300, Imre Deak wrote:
> On ValleyView for both eDP and DP the AUX input clock is 200MHz, so we
> can calculate for both the clock divider for the 2MHz target rate at the
> same place. Afterwards we can also replace the is_cpu_edp() check with a
> check for port A.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

There's a now-dead IS_VLV case in intel_hrawclk which should be killed
with this patch, too.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 90ae58a..b99da13 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -317,11 +317,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	 * Note that PCH attached eDP panels should use a 125MHz input
>  	 * clock divider.
>  	 */
> -	if (is_cpu_edp(intel_dp)) {
> +	if (IS_VALLEYVIEW(dev)) {
> +		aux_clock_divider = 100;
> +	} else if (intel_dig_port->port == PORT_A) {
>  		if (HAS_DDI(dev))
>  			aux_clock_divider = intel_ddi_get_cdclk_freq(dev_priv) >> 1;
> -		else if (IS_VALLEYVIEW(dev))
> -			aux_clock_divider = 100;
>  		else if (IS_GEN6(dev) || IS_GEN7(dev))
>  			aux_clock_divider = 200; /* SNB & IVB eDP input clock at 400Mhz */
>  		else
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: merge VLV eDP and DP AUX clock divider calculation

[Imre Deak]

On Tue, 2013-05-21 at 11:12 +0200, Daniel Vetter wrote:
> On Thu, May 16, 2013 at 02:40:35PM +0300, Imre Deak wrote:
> > On ValleyView for both eDP and DP the AUX input clock is 200MHz, so we
> > can calculate for both the clock divider for the 2MHz target rate at the
> > same place. Afterwards we can also replace the is_cpu_edp() check with a
> > check for port A.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> There's a now-dead IS_VLV case in intel_hrawclk which should be killed
> with this patch, too.

Shouldn't it still return the correct value if someone calls it in the
future? Actually it's also used in
intel_dp_init_panel_power_sequencer_registers().

--Imre


> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 90ae58a..b99da13 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -317,11 +317,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  	 * Note that PCH attached eDP panels should use a 125MHz input
> >  	 * clock divider.
> >  	 */
> > -	if (is_cpu_edp(intel_dp)) {
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		aux_clock_divider = 100;
> > +	} else if (intel_dig_port->port == PORT_A) {
> >  		if (HAS_DDI(dev))
> >  			aux_clock_divider = intel_ddi_get_cdclk_freq(dev_priv) >> 1;
> > -		else if (IS_VALLEYVIEW(dev))
> > -			aux_clock_divider = 100;
> >  		else if (IS_GEN6(dev) || IS_GEN7(dev))
> >  			aux_clock_divider = 200; /* SNB & IVB eDP input clock at 400Mhz */
> >  		else
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 2/4] drm/i915: merge VLV eDP and DP AUX clock divider calculation

[Daniel Vetter]

On Tue, May 21, 2013 at 12:36 PM, Imre Deak <imre.deak@intel.com> wrote:
> On Tue, 2013-05-21 at 11:12 +0200, Daniel Vetter wrote:
>> On Thu, May 16, 2013 at 02:40:35PM +0300, Imre Deak wrote:
>> > On ValleyView for both eDP and DP the AUX input clock is 200MHz, so we
>> > can calculate for both the clock divider for the 2MHz target rate at the
>> > same place. Afterwards we can also replace the is_cpu_edp() check with a
>> > check for port A.
>> >
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>>
>> There's a now-dead IS_VLV case in intel_hrawclk which should be killed
>> with this patch, too.
>
> Shouldn't it still return the correct value if someone calls it in the
> future? Actually it's also used in
> intel_dp_init_panel_power_sequencer_registers().

Indeed. I think though it's better to make this an explicit vlv case
since hrawclk talks about the FSB. And that thing pretty surely
doesn't exist on vlv any more ;-) So maybe a follow-up patch?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: merge VLV eDP and DP AUX clock divider calculation

[Imre Deak]

On Tue, 2013-05-21 at 12:42 +0200, Daniel Vetter wrote:
> On Tue, May 21, 2013 at 12:36 PM, Imre Deak <imre.deak@intel.com> wrote:
> > On Tue, 2013-05-21 at 11:12 +0200, Daniel Vetter wrote:
> >> On Thu, May 16, 2013 at 02:40:35PM +0300, Imre Deak wrote:
> >> > On ValleyView for both eDP and DP the AUX input clock is 200MHz, so we
> >> > can calculate for both the clock divider for the 2MHz target rate at the
> >> > same place. Afterwards we can also replace the is_cpu_edp() check with a
> >> > check for port A.
> >> >
> >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>
> >> There's a now-dead IS_VLV case in intel_hrawclk which should be killed
> >> with this patch, too.
> >
> > Shouldn't it still return the correct value if someone calls it in the
> > future? Actually it's also used in
> > intel_dp_init_panel_power_sequencer_registers().
> 
> Indeed. I think though it's better to make this an explicit vlv case
> since hrawclk talks about the FSB. And that thing pretty surely
> doesn't exist on vlv any more ;-)

Ok. Tbh, I haven't thought about the differences in clock topology
across the platforms, but would be nice to understand it better.

> So maybe a follow-up patch?

Yep, I think it could stand on its own.

--Imre

Re: [PATCH 2/4] drm/i915: merge VLV eDP and DP AUX clock divider calculation

[Imre Deak]

On Tue, 2013-05-21 at 13:59 +0300, Imre Deak wrote:
> On Tue, 2013-05-21 at 12:42 +0200, Daniel Vetter wrote:
> > On Tue, May 21, 2013 at 12:36 PM, Imre Deak <imre.deak@intel.com> wrote:
> > > On Tue, 2013-05-21 at 11:12 +0200, Daniel Vetter wrote:
> > >> On Thu, May 16, 2013 at 02:40:35PM +0300, Imre Deak wrote:
> > >> > On ValleyView for both eDP and DP the AUX input clock is 200MHz, so we
> > >> > can calculate for both the clock divider for the 2MHz target rate at the
> > >> > same place. Afterwards we can also replace the is_cpu_edp() check with a
> > >> > check for port A.
> > >> >
> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > >>
> > >> There's a now-dead IS_VLV case in intel_hrawclk which should be killed
> > >> with this patch, too.
> > >
> > > Shouldn't it still return the correct value if someone calls it in the
> > > future? Actually it's also used in
> > > intel_dp_init_panel_power_sequencer_registers().
> > 
> > Indeed. I think though it's better to make this an explicit vlv case
> > since hrawclk talks about the FSB. And that thing pretty surely
> > doesn't exist on vlv any more ;-)
> 
> Ok. Tbh, I haven't thought about the differences in clock topology
> across the platforms, but would be nice to understand it better.

I checked now and the VLV spec does define hrawclk, so that would
justify keeping it in intel_hrawclk.

--Imre

[PATCH 3/4] drm/i915: replace is_cpu_edp() with a check for port A

[Imre Deak]

The patch changes all remaining is_cpu_edp() check with a check for port
A. We can do this, since in all these cases ValleyView is handled
separately and port A is always a CPU side eDP port.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   49 +++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b99da13..1a4c103 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -684,6 +684,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	enum port port = dp_to_dig_port(intel_dp)->port;
 	struct intel_crtc *intel_crtc = encoder->new_crtc;
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	int lane_count, clock;
@@ -693,7 +694,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
 	int target_clock, link_avail, link_clock;
 
-	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && !is_cpu_edp(intel_dp))
+	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
 		pipe_config->has_pch_encoder = true;
 
 	pipe_config->has_dp_encoder = true;
@@ -828,6 +829,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	enum port port = dp_to_dig_port(intel_dp)->port;
 	struct drm_crtc *crtc = encoder->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
@@ -868,7 +870,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 
 	/* Split out the IBX/CPU vs CPT settings */
 
-	if (is_cpu_edp(intel_dp) && IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) {
+	if (port == PORT_A && IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) {
 		if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
 			intel_dp->DP |= DP_SYNC_HS_HIGH;
 		if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
@@ -885,7 +887,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 			intel_dp->DP |= DP_PLL_FREQ_160MHZ;
 		else
 			intel_dp->DP |= DP_PLL_FREQ_270MHZ;
-	} else if (!HAS_PCH_CPT(dev) || is_cpu_edp(intel_dp)) {
+	} else if (!HAS_PCH_CPT(dev) || port == PORT_A) {
 		if (!HAS_PCH_SPLIT(dev) && !IS_VALLEYVIEW(dev))
 			intel_dp->DP |= intel_dp->color_range;
 
@@ -901,7 +903,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		if (intel_crtc->pipe == 1)
 			intel_dp->DP |= DP_PIPEB_SELECT;
 
-		if (is_cpu_edp(intel_dp) && !IS_VALLEYVIEW(dev)) {
+		if (port == PORT_A && !IS_VALLEYVIEW(dev)) {
 			/* don't miss out required setting for eDP */
 			if (adjusted_mode->clock < 200000)
 				intel_dp->DP |= DP_PLL_FREQ_160MHZ;
@@ -912,7 +914,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
 	}
 
-	if (is_cpu_edp(intel_dp) && !IS_VALLEYVIEW(dev))
+	if (port == PORT_A && !IS_VALLEYVIEW(dev))
 		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
 }
 
@@ -1302,6 +1304,7 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
 				  enum pipe *pipe)
 {
 	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;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 tmp = I915_READ(intel_dp->output_reg);
@@ -1309,9 +1312,9 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
 	if (!(tmp & DP_PORT_EN))
 		return false;
 
-	if (is_cpu_edp(intel_dp) && IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) {
+	if (port == PORT_A && IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) {
 		*pipe = PORT_TO_PIPE_CPT(tmp);
-	} else if (!HAS_PCH_CPT(dev) || is_cpu_edp(intel_dp)) {
+	} else if (!HAS_PCH_CPT(dev) || port == PORT_A) {
 		*pipe = PORT_TO_PIPE(tmp);
 	} else {
 		u32 trans_sel;
@@ -1409,14 +1412,14 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 static void intel_pre_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (is_cpu_edp(intel_dp) && !IS_VALLEYVIEW(dev))
+	if (dport->port == PORT_A && !IS_VALLEYVIEW(dev))
 		ironlake_edp_pll_on(intel_dp);
 
 	if (IS_VALLEYVIEW(dev)) {
-		struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
 		struct intel_crtc *intel_crtc =
 			to_intel_crtc(encoder->base.crtc);
 		int port = vlv_dport_to_channel(dport);
@@ -1528,12 +1531,13 @@ static uint8_t
 intel_dp_voltage_max(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	enum port port = dp_to_dig_port(intel_dp)->port;
 
 	if (IS_VALLEYVIEW(dev))
 		return DP_TRAIN_VOLTAGE_SWING_1200;
-	else if (IS_GEN7(dev) && is_cpu_edp(intel_dp))
+	else if (IS_GEN7(dev) && port == PORT_A)
 		return DP_TRAIN_VOLTAGE_SWING_800;
-	else if (HAS_PCH_CPT(dev) && !is_cpu_edp(intel_dp))
+	else if (HAS_PCH_CPT(dev) && port != PORT_A)
 		return DP_TRAIN_VOLTAGE_SWING_1200;
 	else
 		return DP_TRAIN_VOLTAGE_SWING_800;
@@ -1543,6 +1547,7 @@ static uint8_t
 intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	enum port port = dp_to_dig_port(intel_dp)->port;
 
 	if (HAS_DDI(dev)) {
 		switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
@@ -1568,7 +1573,7 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
 		default:
 			return DP_TRAIN_PRE_EMPHASIS_0;
 		}
-	} else if (IS_GEN7(dev) && is_cpu_edp(intel_dp)) {
+	} else if (IS_GEN7(dev) && port == PORT_A) {
 		switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
 		case DP_TRAIN_VOLTAGE_SWING_400:
 			return DP_TRAIN_PRE_EMPHASIS_6;
@@ -1857,6 +1862,7 @@ static void
 intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	enum port port = intel_dig_port->port;
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	uint32_t signal_levels, mask;
 	uint8_t train_set = intel_dp->train_set[0];
@@ -1867,10 +1873,10 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 	} else if (IS_VALLEYVIEW(dev)) {
 		signal_levels = intel_vlv_signal_levels(intel_dp);
 		mask = 0;
-	} else if (IS_GEN7(dev) && is_cpu_edp(intel_dp)) {
+	} else if (IS_GEN7(dev) && port == PORT_A) {
 		signal_levels = intel_gen7_edp_signal_levels(train_set);
 		mask = EDP_LINK_TRAIN_VOL_EMP_MASK_IVB;
-	} else if (IS_GEN6(dev) && is_cpu_edp(intel_dp)) {
+	} else if (IS_GEN6(dev) && port == PORT_A) {
 		signal_levels = intel_gen6_edp_signal_levels(train_set);
 		mask = EDP_LINK_TRAIN_VOL_EMP_MASK_SNB;
 	} else {
@@ -1920,8 +1926,7 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 		}
 		I915_WRITE(DP_TP_CTL(port), temp);
 
-	} else if (HAS_PCH_CPT(dev) &&
-		   (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) {
+	} else if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A)) {
 		dp_reg_value &= ~DP_LINK_TRAIN_MASK_CPT;
 
 		switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
@@ -2172,6 +2177,7 @@ static void
 intel_dp_link_down(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	enum port port = intel_dig_port->port;
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc =
@@ -2201,7 +2207,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 
 	DRM_DEBUG_KMS("\n");
 
-	if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) {
+	if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A)) {
 		DP &= ~DP_LINK_TRAIN_MASK_CPT;
 		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
 	} else {
@@ -2929,9 +2935,6 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		pp_div_reg = PIPEA_PP_DIVISOR;
 	}
 
-	if (IS_VALLEYVIEW(dev))
-		port_sel = I915_READ(pp_on_reg) & 0xc0000000;
-
 	/* And finally store the new values in the power sequencer. */
 	pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
 		(seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
@@ -2945,8 +2948,10 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 
 	/* Haswell doesn't have any port selection bits for the panel
 	 * power sequencer any more. */
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		if (is_cpu_edp(intel_dp))
+	if (IS_VALLEYVIEW(dev)) {
+		port_sel = I915_READ(pp_on_reg) & 0xc0000000;
+	} else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
+		if (dp_to_dig_port(intel_dp)->port == PORT_A)
 			port_sel = PANEL_POWER_PORT_DP_A;
 		else
 			port_sel = PANEL_POWER_PORT_DP_D;
-- 
1.7.10.4

[PATCH 4/4] drm/i915: remove unused is_cpu_edp()

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1a4c103..0d326e2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -59,22 +59,6 @@ static struct drm_device *intel_dp_to_dev(struct intel_dp *intel_dp)
 	return intel_dig_port->base.base.dev;
 }
 
-/**
- * is_cpu_edp - is the port on the CPU and attached to an eDP panel?
- * @intel_dp: DP struct
- *
- * Returns true if the given DP struct corresponds to a CPU eDP port.
- */
-static bool is_cpu_edp(struct intel_dp *intel_dp)
-{
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	enum port port = intel_dig_port->port;
-
-	return is_edp(intel_dp) &&
-		(port == PORT_A || (port == PORT_C && IS_VALLEYVIEW(dev)));
-}
-
 static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
 {
 	return enc_to_intel_dp(&intel_attached_encoder(connector)->base);
-- 
1.7.10.4

Re: [PATCH 0/4] drm/i915: remove is_cpu_edp()

[Rodrigo Vivi]

Hi Imre,

I just reviewed all patches in this series and saw no issue.
So, in this sense, you and Daniel are free to use "Reviewed-by:
Rodrigo Vivi <rodrigo.vivi@gmail.com>".

However before you move ahead I need to show my concern with the whole idea.
is_cpu_edp is an easy abstraction and provides an easy way to add new
things/restrictions/was/etc like eDP on port D support for HSW.

In the new way code can be more polluted and bugs can be easily added
by forgetting to add a check somewhere when hunting for places like
port_A || vlv ||  to add || (HSW && port_D).

Imagine in some time we have more platforms like valleyview and more
platforms that supports edp on many other different ports.
I really would like to see those checks together in only one place, i.
e., is_cpu_edp().

Cheers,
Rodrigo.

On Thu, May 16, 2013 at 8:40 AM, Imre Deak <imre.deak@intel.com> wrote:
> is_cpu_edp() is equivalent with a port==PORT_A check. There are two
> exceptions (see patch 1 and 2), where we can rewrite things to handle
> ValleyView separately as it's done at other places. With these out of
> the way we can replace is_cpu_edp() with a simpler port check and
> remove is_cpu_edp().
>
> I tested this only on IVB, so before applying we should test it on VLV
> at least.
>
> Imre Deak (4):
>   drm/i915: stop using is_cpu_edp() in intel_disable/post_disable_dp
>   drm/i915: merge VLV eDP and DP AUX clock divider calculation
>   drm/i915: replace is_cpu_edp() with a check for port A
>   drm/i915: remove unused is_cpu_edp()
>
>  drivers/gpu/drm/i915/intel_dp.c |   78 ++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 43 deletions(-)
>
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 0/4] drm/i915: remove is_cpu_edp()

[Daniel Vetter]

On Mon, May 27, 2013 at 7:16 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> Hi Imre,
>
> I just reviewed all patches in this series and saw no issue.
> So, in this sense, you and Daniel are free to use "Reviewed-by:
> Rodrigo Vivi <rodrigo.vivi@gmail.com>".
>
> However before you move ahead I need to show my concern with the whole idea.
> is_cpu_edp is an easy abstraction and provides an easy way to add new
> things/restrictions/was/etc like eDP on port D support for HSW.
>
> In the new way code can be more polluted and bugs can be easily added
> by forgetting to add a check somewhere when hunting for places like
> port_A || vlv ||  to add || (HSW && port_D).
>
> Imagine in some time we have more platforms like valleyview and more
> platforms that supports edp on many other different ports.
> I really would like to see those checks together in only one place, i.
> e., is_cpu_edp().

My idea behind all this is that I think is_cpu_edp is horribly
confusing as an abstraction. Imo we should be really careful to
differentiate between two things which is_cpu_edp smashes toghether:
- eDP vs. non-DP: This should control stuff like enabling the
backlight and controlling the panel power sequence. Also we should use
this for things like grabbing (and caching) the fixed panel mode. In
short this is all about the sink capabilities of a DP output.
- cpu vs. pch port (on ilk/ivb/snb) or port A vs. port B-D: This is
about how some DP ports are different on the same platform, e.g. the
link training is a bit different, or we need a special transcoder, or
a special clock. But all this has nothing to do with whether the DP
sink device is a panel or an external display. The big offender here
was eDP on port D, since on the source side there's zero difference
between eDP mode and DP mode. The only exception really is that we
don't set up a HDMI output, but that's rather external to the dp code.

With Imre's latest patches this should now be untangled: Everything
which cares about the sink only checks for is_edp, everything which
cares about the source checks for platform + port.

So your example of adding a (HSW && port_D) check for eDP support
should never come up.

Does this alleviate your concerns about this refactoring?

Note that I understand that some of the checks are now pretty ugly,
but the only thing that's changed is that the ugly is now explicit.
Once the next big platform change comes around I expect that we need
to refactor intel_dp.c big time and add a vtable to abstract away a
lot of this stuff from the core dp logic.

Yours, Daniel

>
> Cheers,
> Rodrigo.
>
> On Thu, May 16, 2013 at 8:40 AM, Imre Deak <imre.deak@intel.com> wrote:
>> is_cpu_edp() is equivalent with a port==PORT_A check. There are two
>> exceptions (see patch 1 and 2), where we can rewrite things to handle
>> ValleyView separately as it's done at other places. With these out of
>> the way we can replace is_cpu_edp() with a simpler port check and
>> remove is_cpu_edp().
>>
>> I tested this only on IVB, so before applying we should test it on VLV
>> at least.
>>
>> Imre Deak (4):
>>   drm/i915: stop using is_cpu_edp() in intel_disable/post_disable_dp
>>   drm/i915: merge VLV eDP and DP AUX clock divider calculation
>>   drm/i915: replace is_cpu_edp() with a check for port A
>>   drm/i915: remove unused is_cpu_edp()
>>
>>  drivers/gpu/drm/i915/intel_dp.c |   78 ++++++++++++++++++---------------------
>>  1 file changed, 35 insertions(+), 43 deletions(-)
>>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 0/4] drm/i915: remove is_cpu_edp()

[Rodrigo Vivi]

Yeap, makes sense let them explicit then...
Thanks for explanation and fell free to go ahead ;)

On Mon, May 27, 2013 at 2:28 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, May 27, 2013 at 7:16 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> Hi Imre,
>>
>> I just reviewed all patches in this series and saw no issue.
>> So, in this sense, you and Daniel are free to use "Reviewed-by:
>> Rodrigo Vivi <rodrigo.vivi@gmail.com>".
>>
>> However before you move ahead I need to show my concern with the whole idea.
>> is_cpu_edp is an easy abstraction and provides an easy way to add new
>> things/restrictions/was/etc like eDP on port D support for HSW.
>>
>> In the new way code can be more polluted and bugs can be easily added
>> by forgetting to add a check somewhere when hunting for places like
>> port_A || vlv ||  to add || (HSW && port_D).
>>
>> Imagine in some time we have more platforms like valleyview and more
>> platforms that supports edp on many other different ports.
>> I really would like to see those checks together in only one place, i.
>> e., is_cpu_edp().
>
> My idea behind all this is that I think is_cpu_edp is horribly
> confusing as an abstraction. Imo we should be really careful to
> differentiate between two things which is_cpu_edp smashes toghether:
> - eDP vs. non-DP: This should control stuff like enabling the
> backlight and controlling the panel power sequence. Also we should use
> this for things like grabbing (and caching) the fixed panel mode. In
> short this is all about the sink capabilities of a DP output.
> - cpu vs. pch port (on ilk/ivb/snb) or port A vs. port B-D: This is
> about how some DP ports are different on the same platform, e.g. the
> link training is a bit different, or we need a special transcoder, or
> a special clock. But all this has nothing to do with whether the DP
> sink device is a panel or an external display. The big offender here
> was eDP on port D, since on the source side there's zero difference
> between eDP mode and DP mode. The only exception really is that we
> don't set up a HDMI output, but that's rather external to the dp code.
>
> With Imre's latest patches this should now be untangled: Everything
> which cares about the sink only checks for is_edp, everything which
> cares about the source checks for platform + port.
>
> So your example of adding a (HSW && port_D) check for eDP support
> should never come up.
>
> Does this alleviate your concerns about this refactoring?
>
> Note that I understand that some of the checks are now pretty ugly,
> but the only thing that's changed is that the ugly is now explicit.
> Once the next big platform change comes around I expect that we need
> to refactor intel_dp.c big time and add a vtable to abstract away a
> lot of this stuff from the core dp logic.
>
> Yours, Daniel
>
>>
>> Cheers,
>> Rodrigo.
>>
>> On Thu, May 16, 2013 at 8:40 AM, Imre Deak <imre.deak@intel.com> wrote:
>>> is_cpu_edp() is equivalent with a port==PORT_A check. There are two
>>> exceptions (see patch 1 and 2), where we can rewrite things to handle
>>> ValleyView separately as it's done at other places. With these out of
>>> the way we can replace is_cpu_edp() with a simpler port check and
>>> remove is_cpu_edp().
>>>
>>> I tested this only on IVB, so before applying we should test it on VLV
>>> at least.
>>>
>>> Imre Deak (4):
>>>   drm/i915: stop using is_cpu_edp() in intel_disable/post_disable_dp
>>>   drm/i915: merge VLV eDP and DP AUX clock divider calculation
>>>   drm/i915: replace is_cpu_edp() with a check for port A
>>>   drm/i915: remove unused is_cpu_edp()
>>>
>>>  drivers/gpu/drm/i915/intel_dp.c |   78 ++++++++++++++++++---------------------
>>>  1 file changed, 35 insertions(+), 43 deletions(-)
>>>
>>> --
>>> 1.7.10.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 0/4] drm/i915: remove is_cpu_edp()

[Daniel Vetter]

On Mon, May 27, 2013 at 02:54:33PM -0300, Rodrigo Vivi wrote:
> Yeap, makes sense let them explicit then...
> Thanks for explanation and fell free to go ahead ;)

Thanks for your review, all patches merged to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch