[v2,08/17] drm/i915: Set mode_changed for audio in intel_modeset_pipe_config()
diff mbox

Message ID 1431548627-2527-9-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 13, 2015, 8:23 p.m. UTC
From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

A follow up patch will make intel_modeset_compute_config() deal with
multiple crtcs, so move crtc specific stuff into the lower level crtc
specific function.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

Comments

Daniel Vetter May 18, 2015, 3:36 p.m. UTC | #1
On Wed, May 13, 2015 at 10:23:38PM +0200, Maarten Lankhorst wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> A follow up patch will make intel_modeset_compute_config() deal with
> multiple crtcs, so move crtc specific stuff into the lower level crtc
> specific function.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 583c9105cf49..ec548cbb06ee 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -427,6 +427,12 @@ static void vlv_clock(int refclk, intel_clock_t *clock)
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
>  }
>  
> +static bool
> +needs_modeset(struct drm_crtc_state *state)
> +{
> +	return state->mode_changed || state->active_changed;
> +}
> +
>  /**
>   * Returns whether any output on the specified pipe is of the specified type
>   */
> @@ -11387,6 +11393,15 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * XXX: Add all connectors to make the crtc state match the encoders.
> +	 */
> +	if (!needs_modeset(&pipe_config->base)) {
> +		ret = drm_atomic_add_affected_connectors(state, crtc);
> +		if (ret)
> +			return ret;
> +	}

Comment aside: Eventually we need to invert this check and use
intel_pipe_config_compare with a special mode to a) not scream into dmesg
b) ignore any changes we can fixup without a full modeset (i.e. pfit). And
then use that to decide whether we need a modeset or not for the crtc.
We still rely upon our internal set_mode functions to at least in some
cases do an unconditional modeset on the passed-in crtc for property
updates. But once converted to atomic we need to be more intelligent,
especially since userspace expects no-op changes to get filtered out. So
unconditionally doing a modeset wont cut it either (with the current code
all the set_prop hooks have such checks hand-rolled).
-Daniel

> +
>  	clear_intel_crtc_state(pipe_config);
>  
>  	pipe_config->cpu_transcoder =
> @@ -11478,6 +11493,18 @@ encoder_retry:
>  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
> +	/* Check if we need to force a modeset */
> +	if (pipe_config->has_audio !=
> +	    to_intel_crtc_state(crtc->state)->has_audio)
> +		pipe_config->base.mode_changed = true;
> +
> +	/*
> +	 * Note we have an issue here with infoframes: current code
> +	 * only updates them on the full mode set path per hw
> +	 * requirements.  So here we should be checking for any
> +	 * required changes and forcing a mode set.
> +	 */
> +
>  	return 0;
>  fail:
>  	return ret;
> @@ -11495,12 +11522,6 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  	return false;
>  }
>  
> -static bool
> -needs_modeset(struct drm_crtc_state *state)
> -{
> -	return state->mode_changed || state->active_changed;
> -}
> -
>  static void
>  intel_modeset_update_state(struct drm_atomic_state *state)
>  {
> @@ -12093,10 +12114,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	struct intel_crtc_state *pipe_config;
>  	int ret = 0;
>  
> -	ret = drm_atomic_add_affected_connectors(state, crtc);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>  	ret = drm_atomic_helper_check_modeset(state->dev, state);
>  	if (ret)
>  		return ERR_PTR(ret);
> @@ -12122,19 +12139,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -	/* Check things that can only be changed through modeset */
> -	if (pipe_config->has_audio !=
> -	    to_intel_crtc(crtc)->config->has_audio)
> -		pipe_config->base.mode_changed = true;
> -
> -	/*
> -	 * Note we have an issue here with infoframes: current code
> -	 * only updates them on the full mode set path per hw
> -	 * requirements.  So here we should be checking for any
> -	 * required changes and forcing a mode set.
> -	 */
> -
> -	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
> +	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
>  
>  	ret = drm_atomic_helper_check_planes(state->dev, state);
>  	if (ret)
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst May 18, 2015, 4:37 p.m. UTC | #2
Op 18-05-15 om 17:36 schreef Daniel Vetter:
> On Wed, May 13, 2015 at 10:23:38PM +0200, Maarten Lankhorst wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> A follow up patch will make intel_modeset_compute_config() deal with
>> multiple crtcs, so move crtc specific stuff into the lower level crtc
>> specific function.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++----------------
>>  1 file changed, 28 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 583c9105cf49..ec548cbb06ee 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -427,6 +427,12 @@ static void vlv_clock(int refclk, intel_clock_t *clock)
>>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
>>  }
>>  
>> +static bool
>> +needs_modeset(struct drm_crtc_state *state)
>> +{
>> +	return state->mode_changed || state->active_changed;
>> +}
>> +
>>  /**
>>   * Returns whether any output on the specified pipe is of the specified type
>>   */
>> @@ -11387,6 +11393,15 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>>  		return -EINVAL;
>>  	}
>>  
>> +	/*
>> +	 * XXX: Add all connectors to make the crtc state match the encoders.
>> +	 */
>> +	if (!needs_modeset(&pipe_config->base)) {
>> +		ret = drm_atomic_add_affected_connectors(state, crtc);
>> +		if (ret)
>> +			return ret;
>> +	}
> Comment aside: Eventually we need to invert this check and use
> intel_pipe_config_compare with a special mode to a) not scream into dmesg
> b) ignore any changes we can fixup without a full modeset (i.e. pfit). And
> then use that to decide whether we need a modeset or not for the crtc.
> We still rely upon our internal set_mode functions to at least in some
> cases do an unconditional modeset on the passed-in crtc for property
> updates. But once converted to atomic we need to be more intelligent,
> especially since userspace expects no-op changes to get filtered out. So
> unconditionally doing a modeset wont cut it either (with the current code
> all the set_prop hooks have such checks hand-rolled).
Yeah, but for now I can't get that to work properly until everything looks more atomic first.

~Maarten

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 583c9105cf49..ec548cbb06ee 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -427,6 +427,12 @@  static void vlv_clock(int refclk, intel_clock_t *clock)
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
 }
 
+static bool
+needs_modeset(struct drm_crtc_state *state)
+{
+	return state->mode_changed || state->active_changed;
+}
+
 /**
  * Returns whether any output on the specified pipe is of the specified type
  */
@@ -11387,6 +11393,15 @@  intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	/*
+	 * XXX: Add all connectors to make the crtc state match the encoders.
+	 */
+	if (!needs_modeset(&pipe_config->base)) {
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret)
+			return ret;
+	}
+
 	clear_intel_crtc_state(pipe_config);
 
 	pipe_config->cpu_transcoder =
@@ -11478,6 +11493,18 @@  encoder_retry:
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
+	/* Check if we need to force a modeset */
+	if (pipe_config->has_audio !=
+	    to_intel_crtc_state(crtc->state)->has_audio)
+		pipe_config->base.mode_changed = true;
+
+	/*
+	 * Note we have an issue here with infoframes: current code
+	 * only updates them on the full mode set path per hw
+	 * requirements.  So here we should be checking for any
+	 * required changes and forcing a mode set.
+	 */
+
 	return 0;
 fail:
 	return ret;
@@ -11495,12 +11522,6 @@  static bool intel_crtc_in_use(struct drm_crtc *crtc)
 	return false;
 }
 
-static bool
-needs_modeset(struct drm_crtc_state *state)
-{
-	return state->mode_changed || state->active_changed;
-}
-
 static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {
@@ -12093,10 +12114,6 @@  intel_modeset_compute_config(struct drm_crtc *crtc,
 	struct intel_crtc_state *pipe_config;
 	int ret = 0;
 
-	ret = drm_atomic_add_affected_connectors(state, crtc);
-	if (ret)
-		return ERR_PTR(ret);
-
 	ret = drm_atomic_helper_check_modeset(state->dev, state);
 	if (ret)
 		return ERR_PTR(ret);
@@ -12122,19 +12139,7 @@  intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (ret)
 		return ERR_PTR(ret);
 
-	/* Check things that can only be changed through modeset */
-	if (pipe_config->has_audio !=
-	    to_intel_crtc(crtc)->config->has_audio)
-		pipe_config->base.mode_changed = true;
-
-	/*
-	 * Note we have an issue here with infoframes: current code
-	 * only updates them on the full mode set path per hw
-	 * requirements.  So here we should be checking for any
-	 * required changes and forcing a mode set.
-	 */
-
-	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
+	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
 
 	ret = drm_atomic_helper_check_planes(state->dev, state);
 	if (ret)