diff mbox

[v3,3/3] drm/i915: move encoder->enable callback later in VLV crtc enable

Message ID 3eb5007d000302144b9e77c25536feba3168f134.1375175822.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jani Nikula July 30, 2013, 9:20 a.m. UTC
VLV wants encoder enabling before the pipe is up. With the previously
rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
this no longer depends on the early ->enable hook call. Move the
->enable call at the end of the sequence, similar to the crtc enable on
other platforms. This will be needed e.g. for moving the eDP backlight
enabling to the right place in the sequence, currently done too early on
VLV.

There should be no functional changes.

v2: Rebase.

v3: Explain why this is needed in the commit message (Chris).

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Chris Wilson July 30, 2013, 10:58 a.m. UTC | #1
On Tue, Jul 30, 2013 at 12:20:32PM +0300, Jani Nikula wrote:
> VLV wants encoder enabling before the pipe is up. With the previously
> rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
> this no longer depends on the early ->enable hook call. Move the
> ->enable call at the end of the sequence, similar to the crtc enable on
> other platforms. This will be needed e.g. for moving the eDP backlight
> enabling to the right place in the sequence, currently done too early on
> VLV.
> 
> There should be no functional changes.
> 
> v2: Rebase.
> 
> v3: Explain why this is needed in the commit message (Chris).
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

I couldn't spot any functional changes in the 3 patches, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä July 30, 2013, 2:24 p.m. UTC | #2
On Tue, Jul 30, 2013 at 12:20:32PM +0300, Jani Nikula wrote:
> VLV wants encoder enabling before the pipe is up. With the previously
> rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
> this no longer depends on the early ->enable hook call. Move the
> ->enable call at the end of the sequence, similar to the crtc enable on
> other platforms. This will be needed e.g. for moving the eDP backlight
> enabling to the right place in the sequence, currently done too early on
> VLV.
> 
> There should be no functional changes.
> 
> v2: Rebase.
> 
> v3: Explain why this is needed in the commit message (Chris).
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Looks sane to me as well. For the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Note that I just lost my VLV board so I wasn't able to test this.

I'm assuming there will eventually be some followon patch to move the
backlight stuff to right place for VLV?

Also should we make encoder->enable optional to avoid the stubs for VLV?

And it might make sense to rename all the VLV specific dp/hdmi functions
to vlv_foo instead of the intel_foo that they are called now.

> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7d63d8d..c3c0bf1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3663,10 +3663,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  		if (encoder->pre_enable)
>  			encoder->pre_enable(encoder);
>  
> -	/* VLV wants encoder enabling _before_ the pipe is up. */
> -	for_each_encoder_on_crtc(dev, crtc, encoder)
> -		encoder->enable(encoder);
> -
>  	i9xx_pfit_enable(intel_crtc);
>  
>  	intel_crtc_load_lut(crtc);
> @@ -3677,6 +3673,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_update_cursor(crtc, true);
>  
>  	intel_update_fbc(dev);
> +
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		encoder->enable(encoder);
>  }
>  
>  static void i9xx_crtc_enable(struct drm_crtc *crtc)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Aug. 5, 2013, 6:14 a.m. UTC | #3
On Tue, Jul 30, 2013 at 11:58:10AM +0100, Chris Wilson wrote:
> On Tue, Jul 30, 2013 at 12:20:32PM +0300, Jani Nikula wrote:
> > VLV wants encoder enabling before the pipe is up. With the previously
> > rearranged VLV DP and HDMI ->pre_enable and ->enable callbacks in place,
> > this no longer depends on the early ->enable hook call. Move the
> > ->enable call at the end of the sequence, similar to the crtc enable on
> > other platforms. This will be needed e.g. for moving the eDP backlight
> > enabling to the right place in the sequence, currently done too early on
> > VLV.
> > 
> > There should be no functional changes.
> > 
> > v2: Rebase.
> > 
> > v3: Explain why this is needed in the commit message (Chris).
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> I couldn't spot any functional changes in the 3 patches, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Series merged to dinq, thanks for the patches&review.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7d63d8d..c3c0bf1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3663,10 +3663,6 @@  static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		if (encoder->pre_enable)
 			encoder->pre_enable(encoder);
 
-	/* VLV wants encoder enabling _before_ the pipe is up. */
-	for_each_encoder_on_crtc(dev, crtc, encoder)
-		encoder->enable(encoder);
-
 	i9xx_pfit_enable(intel_crtc);
 
 	intel_crtc_load_lut(crtc);
@@ -3677,6 +3673,9 @@  static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_update_cursor(crtc, true);
 
 	intel_update_fbc(dev);
+
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		encoder->enable(encoder);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)