[v6,02/11] drm/i915: Remove hw.mode

Message ID	20200715224222.7557-2-manasi.d.navare@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=ev6m=A2=lists.freedesktop.org=intel-gfx-bounces@kernel.org> DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B018C20658 IronPort-SDR: 7WpjxKvllMyoXH5y0h1uFFaaOqY5/0ZSG+SrdFh8KQDAayfkurV8bE8hJdDvseE+yyo2f1qXpq LPsjGgu8zt3A== IronPort-SDR: Q6JGtozbBxaaEaAS4w6mz0NM+vtICqHsIKDPe8hAIngps7YrdR2wx00yEMo7BJZMCLME+ah77x 5e3wPdZZxBSw== From: Manasi Navare <manasi.d.navare@intel.com> To: intel-gfx@lists.freedesktop.org Date: Wed, 15 Jul 2020 15:42:13 -0700 Message-Id: <20200715224222.7557-2-manasi.d.navare@intel.com> In-Reply-To: <20200715224222.7557-1-manasi.d.navare@intel.com> References: <20200715224222.7557-1-manasi.d.navare@intel.com> MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH v6 02/11] drm/i915: Remove hw.mode Precedence: list Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
Series	[v6,01/11] HAX to make DSC work on the icelake test system \| expand [v6,01/11] HAX to make DSC work on the icelake test system [v6,02/11] drm/i915: Remove hw.mode [v6,03/11] drm/i915: Add hw.pipe_mode to allow bigjoiner pipe/transcoder split [v6,04/11] drm/i915/dp: Allow big joiner modes in intel_dp_mode_valid(), v3. [v6,05/11] drm/i915: Try to make bigjoiner work in atomic check [v6,06/11] drm/i915: Enable big joiner support in enable and disable sequences. [v6,07/11] drm/i915: Make hardware readout work on i915. [v6,08/11] drm/i915: Link planes in a bigjoiner configuration, v3. [v6,09/11] drm/i915: Add bigjoiner aware plane clipping checks [v6,10/11] drm/i915: Add intel_update_bigjoiner handling. [v6,11/11] drm/i915: Add debugfs dumping for bigjoiner, v3.

Navare, Manasi July 15, 2020, 10:42 p.m. UTC

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

The members in hw.mode can be used from adjusted_mode as well,
use that when available.

Some places that use hw.mode can be converted to use adjusted_mode
as well.

v2:
* Manual rebase (Manasi)
* remove the use of pipe_mode defined in patch 3 (Manasi)

v3:
* Rebase on drm-tip (Manasi)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
 .../drm/i915/display/intel_display_types.h    |  2 +-
 drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
 drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
 4 files changed, 23 insertions(+), 26 deletions(-)

Manna, Animesh Aug. 17, 2020, 7:26 a.m. UTC | #1

On 16-07-2020 04:12, Manasi Navare wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> The members in hw.mode can be used from adjusted_mode as well,
> use that when available.
>
> Some places that use hw.mode can be converted to use adjusted_mode
> as well.
>
> v2:
> * Manual rebase (Manasi)
> * remove the use of pipe_mode defined in patch 3 (Manasi)
>
> v3:
> * Rebase on drm-tip (Manasi)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>

Changes looks ok to me.
Reviewed-by: Animesh Manna <animesh.manna@intel.com>

> ---
>   drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
>   .../drm/i915/display/intel_display_types.h    |  2 +-
>   drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
>   drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
>   4 files changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 729ec6e0d43a..8652a7c6bf11 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
>   	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
>   	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
>   	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> -
> -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
>   }
>   
>   void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
>   		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
>   
>   	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
>   	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
>   	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
>   	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
>   {
>   	crtc_state->hw.enable = crtc_state->uapi.enable;
>   	crtc_state->hw.active = crtc_state->uapi.active;
> -	crtc_state->hw.mode = crtc_state->uapi.mode;
>   	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
>   	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
>   }
>   
> -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> +					     struct drm_display_mode *user_mode)
>   {
>   	crtc_state->uapi.enable = crtc_state->hw.enable;
>   	crtc_state->uapi.active = crtc_state->hw.active;
>   	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
>   
>   	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
>   
> @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
>   	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
>   	kfree(saved_state);
>   
> +	/* Clear I915_MODE_FLAG_INHERITED */
> +	crtc_state->uapi.mode.private_flags = 0;
> +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> +
>   	intel_crtc_copy_uapi_to_hw_state(crtc_state);
>   
>   	return 0;
> @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>   	 * computation to clearly distinguish it from the adjusted mode, which
>   	 * can be changed by the connectors in the below retry loop.
>   	 */
> -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
>   			       &pipe_config->pipe_src_w,
>   			       &pipe_config->pipe_src_h);
>   
> @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>   		int min_cdclk = 0;
>   
>   		if (crtc_state->hw.active) {
> -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> +			struct drm_display_mode mode;
>   
>   			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
>   						    crtc_state);
>   
> -			*mode = crtc_state->hw.adjusted_mode;
> -			mode->hdisplay = crtc_state->pipe_src_w;
> -			mode->vdisplay = crtc_state->pipe_src_h;
> -
>   			/*
>   			 * The initial mode needs to be set in order to keep
>   			 * the atomic core happy. It wants a valid mode if the
> @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>   			 */
>   			crtc_state->inherited = true;
>   
> +			mode = crtc_state->hw.adjusted_mode;
> +			mode.hdisplay = crtc_state->pipe_src_w;
> +			mode.vdisplay = crtc_state->pipe_src_h;
> +
>   			intel_crtc_compute_pixel_rate(crtc_state);
>   
>   			intel_crtc_update_active_timings(crtc_state);
>   
> -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
>   		}
>   
>   		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e8f809161c75..f1e29d9a75d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -807,7 +807,7 @@ struct intel_crtc_state {
>   	struct {
>   		bool active, enable;
>   		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> -		struct drm_display_mode mode, adjusted_mode;
> +		struct drm_display_mode adjusted_mode;
>   	} hw;
>   
>   	/**
> diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> index 307ed8ae9a19..0b9bf1fec0f4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
>   	u32 temp = intel_de_read(dev_priv, dvo_reg);
>   
>   	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> -					 &pipe_config->hw.mode,
> +					 &pipe_config->hw.adjusted_mode,
>   					 &pipe_config->hw.adjusted_mode);
>   
>   	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 2da4388e1540..8b78ae0c39a0 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
>   static bool
>   intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
>   				    struct intel_sdvo_connector *intel_sdvo_connector,
> -				    const struct drm_display_mode *mode,
>   				    struct drm_display_mode *adjusted_mode)
>   {
>   	struct intel_sdvo_dtd input_dtd;
> @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
>   
>   	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
>   						      intel_sdvo_connector,
> -						      mode->clock / 10,
> -						      mode->hdisplay,
> -						      mode->vdisplay))
> +						      adjusted_mode->clock / 10,
> +						      adjusted_mode->hdisplay,
> +						      adjusted_mode->vdisplay))
>   		return false;
>   
>   	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>   	struct intel_sdvo_connector *intel_sdvo_connector =
>   		to_intel_sdvo_connector(conn_state->connector);
>   	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> -	struct drm_display_mode *mode = &pipe_config->hw.mode;
>   
>   	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
>   	pipe_config->pipe_bpp = 8*3;
> @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>   	 * the sequence to do it. Oh well.
>   	 */
>   	if (IS_TV(intel_sdvo_connector)) {
> -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> +							     adjusted_mode))
>   			return -EINVAL;
>   
>   		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
>   							   intel_sdvo_connector,
> -							   mode,
>   							   adjusted_mode);
>   		pipe_config->sdvo_tv_clock = true;
>   	} else if (IS_LVDS(intel_sdvo_connector)) {
> @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>   
>   		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
>   							   intel_sdvo_connector,
> -							   mode,
>   							   adjusted_mode);
>   	}
>   
> @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
>   		to_intel_sdvo_connector_state(conn_state);
>   	const struct intel_sdvo_connector *intel_sdvo_connector =
>   		to_intel_sdvo_connector(conn_state->connector);
> -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
>   	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
>   	u32 sdvox;
>   	struct intel_sdvo_in_out_map in_out;
> @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
>   		intel_sdvo_get_dtd_from_mode(&output_dtd,
>   					     intel_sdvo_connector->base.panel.fixed_mode);
>   	else
> -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
>   	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
>   		drm_info(&dev_priv->drm,
>   			 "Setting output timings on %s failed\n",

Ville Syrjala Sept. 3, 2020, 5:49 p.m. UTC | #2

On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> The members in hw.mode can be used from adjusted_mode as well,
> use that when available.
> 
> Some places that use hw.mode can be converted to use adjusted_mode
> as well.
> 
> v2:
> * Manual rebase (Manasi)
> * remove the use of pipe_mode defined in patch 3 (Manasi)
> 
> v3:
> * Rebase on drm-tip (Manasi)

Previous review was apparently ignored. Or is there a better version
somewhere? If not, this still looks very wrong.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
>  .../drm/i915/display/intel_display_types.h    |  2 +-
>  drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
>  drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
>  4 files changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 729ec6e0d43a..8652a7c6bf11 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
>  	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
>  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
>  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> -
> -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
>  }
>  
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
>  		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
>  
>  	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
>  	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
>  	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
>  	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
>  {
>  	crtc_state->hw.enable = crtc_state->uapi.enable;
>  	crtc_state->hw.active = crtc_state->uapi.active;
> -	crtc_state->hw.mode = crtc_state->uapi.mode;
>  	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
>  	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
>  }
>  
> -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> +					     struct drm_display_mode *user_mode)
>  {
>  	crtc_state->uapi.enable = crtc_state->hw.enable;
>  	crtc_state->uapi.active = crtc_state->hw.active;
>  	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
>  
>  	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
>  
> @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
>  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
>  	kfree(saved_state);
>  
> +	/* Clear I915_MODE_FLAG_INHERITED */
> +	crtc_state->uapi.mode.private_flags = 0;
> +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> +
>  	intel_crtc_copy_uapi_to_hw_state(crtc_state);
>  
>  	return 0;
> @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	 * computation to clearly distinguish it from the adjusted mode, which
>  	 * can be changed by the connectors in the below retry loop.
>  	 */
> -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
>  			       &pipe_config->pipe_src_w,
>  			       &pipe_config->pipe_src_h);
>  
> @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		int min_cdclk = 0;
>  
>  		if (crtc_state->hw.active) {
> -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> +			struct drm_display_mode mode;
>  
>  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
>  						    crtc_state);
>  
> -			*mode = crtc_state->hw.adjusted_mode;
> -			mode->hdisplay = crtc_state->pipe_src_w;
> -			mode->vdisplay = crtc_state->pipe_src_h;
> -
>  			/*
>  			 * The initial mode needs to be set in order to keep
>  			 * the atomic core happy. It wants a valid mode if the
> @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			 */
>  			crtc_state->inherited = true;
>  
> +			mode = crtc_state->hw.adjusted_mode;
> +			mode.hdisplay = crtc_state->pipe_src_w;
> +			mode.vdisplay = crtc_state->pipe_src_h;
> +
>  			intel_crtc_compute_pixel_rate(crtc_state);
>  
>  			intel_crtc_update_active_timings(crtc_state);
>  
> -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
>  		}
>  
>  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e8f809161c75..f1e29d9a75d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -807,7 +807,7 @@ struct intel_crtc_state {
>  	struct {
>  		bool active, enable;
>  		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> -		struct drm_display_mode mode, adjusted_mode;
> +		struct drm_display_mode adjusted_mode;
>  	} hw;
>  
>  	/**
> diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> index 307ed8ae9a19..0b9bf1fec0f4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
>  	u32 temp = intel_de_read(dev_priv, dvo_reg);
>  
>  	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> -					 &pipe_config->hw.mode,
> +					 &pipe_config->hw.adjusted_mode,
>  					 &pipe_config->hw.adjusted_mode);
>  
>  	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 2da4388e1540..8b78ae0c39a0 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
>  static bool
>  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
>  				    struct intel_sdvo_connector *intel_sdvo_connector,
> -				    const struct drm_display_mode *mode,
>  				    struct drm_display_mode *adjusted_mode)
>  {
>  	struct intel_sdvo_dtd input_dtd;
> @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
>  
>  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
>  						      intel_sdvo_connector,
> -						      mode->clock / 10,
> -						      mode->hdisplay,
> -						      mode->vdisplay))
> +						      adjusted_mode->clock / 10,
> +						      adjusted_mode->hdisplay,
> +						      adjusted_mode->vdisplay))
>  		return false;
>  
>  	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>  	struct intel_sdvo_connector *intel_sdvo_connector =
>  		to_intel_sdvo_connector(conn_state->connector);
>  	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> -	struct drm_display_mode *mode = &pipe_config->hw.mode;
>  
>  	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
>  	pipe_config->pipe_bpp = 8*3;
> @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>  	 * the sequence to do it. Oh well.
>  	 */
>  	if (IS_TV(intel_sdvo_connector)) {
> -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> +							     adjusted_mode))
>  			return -EINVAL;
>  
>  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
>  							   intel_sdvo_connector,
> -							   mode,
>  							   adjusted_mode);
>  		pipe_config->sdvo_tv_clock = true;
>  	} else if (IS_LVDS(intel_sdvo_connector)) {
> @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
>  
>  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
>  							   intel_sdvo_connector,
> -							   mode,
>  							   adjusted_mode);
>  	}
>  
> @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
>  		to_intel_sdvo_connector_state(conn_state);
>  	const struct intel_sdvo_connector *intel_sdvo_connector =
>  		to_intel_sdvo_connector(conn_state->connector);
> -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
>  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
>  	u32 sdvox;
>  	struct intel_sdvo_in_out_map in_out;
> @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
>  		intel_sdvo_get_dtd_from_mode(&output_dtd,
>  					     intel_sdvo_connector->base.panel.fixed_mode);
>  	else
> -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
>  	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
>  		drm_info(&dev_priv->drm,
>  			 "Setting output timings on %s failed\n",
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi Sept. 3, 2020, 6:04 p.m. UTC | #3

On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > The members in hw.mode can be used from adjusted_mode as well,
> > use that when available.
> > 
> > Some places that use hw.mode can be converted to use adjusted_mode
> > as well.
> > 
> > v2:
> > * Manual rebase (Manasi)
> > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > 
> > v3:
> > * Rebase on drm-tip (Manasi)
> 
> Previous review was apparently ignored. Or is there a better version
> somewhere? If not, this still looks very wrong.

This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
What in particular looks wrong or what review comments were unaddressed here?

@Maarten any feedback on Ville's unaddressed comments?

Manasi

> 
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
> >  .../drm/i915/display/intel_display_types.h    |  2 +-
> >  drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
> >  drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
> >  4 files changed, 23 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 729ec6e0d43a..8652a7c6bf11 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> >  	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
> >  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> >  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> > -
> > -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> > -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
> >  }
> >  
> >  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> >  		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
> >  
> >  	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> > -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> > +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
> >  	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
> >  	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
> >  	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> > @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> >  {
> >  	crtc_state->hw.enable = crtc_state->uapi.enable;
> >  	crtc_state->hw.active = crtc_state->uapi.active;
> > -	crtc_state->hw.mode = crtc_state->uapi.mode;
> >  	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> >  	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> >  }
> >  
> > -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > +					     struct drm_display_mode *user_mode)
> >  {
> >  	crtc_state->uapi.enable = crtc_state->hw.enable;
> >  	crtc_state->uapi.active = crtc_state->hw.active;
> >  	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> > +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
> >  
> >  	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> >  
> > @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> >  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> >  	kfree(saved_state);
> >  
> > +	/* Clear I915_MODE_FLAG_INHERITED */
> > +	crtc_state->uapi.mode.private_flags = 0;
> > +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> > +
> >  	intel_crtc_copy_uapi_to_hw_state(crtc_state);
> >  
> >  	return 0;
> > @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	 * computation to clearly distinguish it from the adjusted mode, which
> >  	 * can be changed by the connectors in the below retry loop.
> >  	 */
> > -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> > +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
> >  			       &pipe_config->pipe_src_w,
> >  			       &pipe_config->pipe_src_h);
> >  
> > @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  		int min_cdclk = 0;
> >  
> >  		if (crtc_state->hw.active) {
> > -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> > +			struct drm_display_mode mode;
> >  
> >  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> >  						    crtc_state);
> >  
> > -			*mode = crtc_state->hw.adjusted_mode;
> > -			mode->hdisplay = crtc_state->pipe_src_w;
> > -			mode->vdisplay = crtc_state->pipe_src_h;
> > -
> >  			/*
> >  			 * The initial mode needs to be set in order to keep
> >  			 * the atomic core happy. It wants a valid mode if the
> > @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  			 */
> >  			crtc_state->inherited = true;
> >  
> > +			mode = crtc_state->hw.adjusted_mode;
> > +			mode.hdisplay = crtc_state->pipe_src_w;
> > +			mode.vdisplay = crtc_state->pipe_src_h;
> > +
> >  			intel_crtc_compute_pixel_rate(crtc_state);
> >  
> >  			intel_crtc_update_active_timings(crtc_state);
> >  
> > -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
> >  		}
> >  
> >  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index e8f809161c75..f1e29d9a75d0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -807,7 +807,7 @@ struct intel_crtc_state {
> >  	struct {
> >  		bool active, enable;
> >  		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> > -		struct drm_display_mode mode, adjusted_mode;
> > +		struct drm_display_mode adjusted_mode;
> >  	} hw;
> >  
> >  	/**
> > diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> > index 307ed8ae9a19..0b9bf1fec0f4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> > @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
> >  	u32 temp = intel_de_read(dev_priv, dvo_reg);
> >  
> >  	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> > -					 &pipe_config->hw.mode,
> > +					 &pipe_config->hw.adjusted_mode,
> >  					 &pipe_config->hw.adjusted_mode);
> >  
> >  	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > index 2da4388e1540..8b78ae0c39a0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
> >  static bool
> >  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> >  				    struct intel_sdvo_connector *intel_sdvo_connector,
> > -				    const struct drm_display_mode *mode,
> >  				    struct drm_display_mode *adjusted_mode)
> >  {
> >  	struct intel_sdvo_dtd input_dtd;
> > @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> >  
> >  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> >  						      intel_sdvo_connector,
> > -						      mode->clock / 10,
> > -						      mode->hdisplay,
> > -						      mode->vdisplay))
> > +						      adjusted_mode->clock / 10,
> > +						      adjusted_mode->hdisplay,
> > +						      adjusted_mode->vdisplay))
> >  		return false;
> >  
> >  	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> > @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> >  	struct intel_sdvo_connector *intel_sdvo_connector =
> >  		to_intel_sdvo_connector(conn_state->connector);
> >  	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> > -	struct drm_display_mode *mode = &pipe_config->hw.mode;
> >  
> >  	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> >  	pipe_config->pipe_bpp = 8*3;
> > @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> >  	 * the sequence to do it. Oh well.
> >  	 */
> >  	if (IS_TV(intel_sdvo_connector)) {
> > -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > +							     adjusted_mode))
> >  			return -EINVAL;
> >  
> >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> >  							   intel_sdvo_connector,
> > -							   mode,
> >  							   adjusted_mode);
> >  		pipe_config->sdvo_tv_clock = true;
> >  	} else if (IS_LVDS(intel_sdvo_connector)) {
> > @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> >  
> >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> >  							   intel_sdvo_connector,
> > -							   mode,
> >  							   adjusted_mode);
> >  	}
> >  
> > @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> >  		to_intel_sdvo_connector_state(conn_state);
> >  	const struct intel_sdvo_connector *intel_sdvo_connector =
> >  		to_intel_sdvo_connector(conn_state->connector);
> > -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
> >  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> >  	u32 sdvox;
> >  	struct intel_sdvo_in_out_map in_out;
> > @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> >  		intel_sdvo_get_dtd_from_mode(&output_dtd,
> >  					     intel_sdvo_connector->base.panel.fixed_mode);
> >  	else
> > -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
> >  	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
> >  		drm_info(&dev_priv->drm,
> >  			 "Setting output timings on %s failed\n",
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala Sept. 3, 2020, 6:40 p.m. UTC | #4

On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > 
> > > The members in hw.mode can be used from adjusted_mode as well,
> > > use that when available.
> > > 
> > > Some places that use hw.mode can be converted to use adjusted_mode
> > > as well.
> > > 
> > > v2:
> > > * Manual rebase (Manasi)
> > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > 
> > > v3:
> > > * Rebase on drm-tip (Manasi)
> > 
> > Previous review was apparently ignored. Or is there a better version
> > somewhere? If not, this still looks very wrong.
> 
> This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> What in particular looks wrong or what review comments were unaddressed here?

The dvo/sdvo changes.

> 
> @Maarten any feedback on Ville's unaddressed comments?
> 
> Manasi
> 
> > 
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
> > >  .../drm/i915/display/intel_display_types.h    |  2 +-
> > >  drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
> > >  drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
> > >  4 files changed, 23 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 729ec6e0d43a..8652a7c6bf11 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> > >  	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
> > >  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> > >  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> > > -
> > > -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> > > -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
> > >  }
> > >  
> > >  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > > @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> > >  		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
> > >  
> > >  	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> > > -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> > > +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
> > >  	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
> > >  	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
> > >  	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> > > @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> > >  {
> > >  	crtc_state->hw.enable = crtc_state->uapi.enable;
> > >  	crtc_state->hw.active = crtc_state->uapi.active;
> > > -	crtc_state->hw.mode = crtc_state->uapi.mode;
> > >  	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> > >  	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> > >  }
> > >  
> > > -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> > > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > +					     struct drm_display_mode *user_mode)
> > >  {
> > >  	crtc_state->uapi.enable = crtc_state->hw.enable;
> > >  	crtc_state->uapi.active = crtc_state->hw.active;
> > >  	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> > > +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
> > >  
> > >  	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> > >  
> > > @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> > >  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> > >  	kfree(saved_state);
> > >  
> > > +	/* Clear I915_MODE_FLAG_INHERITED */
> > > +	crtc_state->uapi.mode.private_flags = 0;
> > > +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> > > +
> > >  	intel_crtc_copy_uapi_to_hw_state(crtc_state);
> > >  
> > >  	return 0;
> > > @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > >  	 * computation to clearly distinguish it from the adjusted mode, which
> > >  	 * can be changed by the connectors in the below retry loop.
> > >  	 */
> > > -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> > > +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
> > >  			       &pipe_config->pipe_src_w,
> > >  			       &pipe_config->pipe_src_h);
> > >  
> > > @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > >  		int min_cdclk = 0;
> > >  
> > >  		if (crtc_state->hw.active) {
> > > -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > +			struct drm_display_mode mode;
> > >  
> > >  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> > >  						    crtc_state);
> > >  
> > > -			*mode = crtc_state->hw.adjusted_mode;
> > > -			mode->hdisplay = crtc_state->pipe_src_w;
> > > -			mode->vdisplay = crtc_state->pipe_src_h;
> > > -
> > >  			/*
> > >  			 * The initial mode needs to be set in order to keep
> > >  			 * the atomic core happy. It wants a valid mode if the
> > > @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > >  			 */
> > >  			crtc_state->inherited = true;
> > >  
> > > +			mode = crtc_state->hw.adjusted_mode;
> > > +			mode.hdisplay = crtc_state->pipe_src_w;
> > > +			mode.vdisplay = crtc_state->pipe_src_h;
> > > +
> > >  			intel_crtc_compute_pixel_rate(crtc_state);
> > >  
> > >  			intel_crtc_update_active_timings(crtc_state);
> > >  
> > > -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > > +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
> > >  		}
> > >  
> > >  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index e8f809161c75..f1e29d9a75d0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -807,7 +807,7 @@ struct intel_crtc_state {
> > >  	struct {
> > >  		bool active, enable;
> > >  		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> > > -		struct drm_display_mode mode, adjusted_mode;
> > > +		struct drm_display_mode adjusted_mode;
> > >  	} hw;
> > >  
> > >  	/**
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > index 307ed8ae9a19..0b9bf1fec0f4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
> > >  	u32 temp = intel_de_read(dev_priv, dvo_reg);
> > >  
> > >  	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> > > -					 &pipe_config->hw.mode,
> > > +					 &pipe_config->hw.adjusted_mode,
> > >  					 &pipe_config->hw.adjusted_mode);
> > >  
> > >  	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > index 2da4388e1540..8b78ae0c39a0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
> > >  static bool
> > >  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > >  				    struct intel_sdvo_connector *intel_sdvo_connector,
> > > -				    const struct drm_display_mode *mode,
> > >  				    struct drm_display_mode *adjusted_mode)
> > >  {
> > >  	struct intel_sdvo_dtd input_dtd;
> > > @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > >  
> > >  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > >  						      intel_sdvo_connector,
> > > -						      mode->clock / 10,
> > > -						      mode->hdisplay,
> > > -						      mode->vdisplay))
> > > +						      adjusted_mode->clock / 10,
> > > +						      adjusted_mode->hdisplay,
> > > +						      adjusted_mode->vdisplay))
> > >  		return false;
> > >  
> > >  	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> > > @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > >  	struct intel_sdvo_connector *intel_sdvo_connector =
> > >  		to_intel_sdvo_connector(conn_state->connector);
> > >  	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> > > -	struct drm_display_mode *mode = &pipe_config->hw.mode;
> > >  
> > >  	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> > >  	pipe_config->pipe_bpp = 8*3;
> > > @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > >  	 * the sequence to do it. Oh well.
> > >  	 */
> > >  	if (IS_TV(intel_sdvo_connector)) {
> > > -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > > +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > > +							     adjusted_mode))
> > >  			return -EINVAL;
> > >  
> > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > >  							   intel_sdvo_connector,
> > > -							   mode,
> > >  							   adjusted_mode);
> > >  		pipe_config->sdvo_tv_clock = true;
> > >  	} else if (IS_LVDS(intel_sdvo_connector)) {
> > > @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > >  
> > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > >  							   intel_sdvo_connector,
> > > -							   mode,
> > >  							   adjusted_mode);
> > >  	}
> > >  
> > > @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > >  		to_intel_sdvo_connector_state(conn_state);
> > >  	const struct intel_sdvo_connector *intel_sdvo_connector =
> > >  		to_intel_sdvo_connector(conn_state->connector);
> > > -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
> > >  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > >  	u32 sdvox;
> > >  	struct intel_sdvo_in_out_map in_out;
> > > @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > >  		intel_sdvo_get_dtd_from_mode(&output_dtd,
> > >  					     intel_sdvo_connector->base.panel.fixed_mode);
> > >  	else
> > > -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > > +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
> > >  	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
> > >  		drm_info(&dev_priv->drm,
> > >  			 "Setting output timings on %s failed\n",
> > > -- 
> > > 2.19.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Ville Syrjala Sept. 7, 2020, 12:35 p.m. UTC | #5

On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > 
> > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > use that when available.
> > > > 
> > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > as well.
> > > > 
> > > > v2:
> > > > * Manual rebase (Manasi)
> > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > 
> > > > v3:
> > > > * Rebase on drm-tip (Manasi)
> > > 
> > > Previous review was apparently ignored. Or is there a better version
> > > somewhere? If not, this still looks very wrong.
> > 
> > This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> > What in particular looks wrong or what review comments were unaddressed here?
> 
> The dvo/sdvo changes.

I recommend just dropping this patch entirely. It doesn't seem to have
anything to do with the bigjoiner anyway.

> 
> > 
> > @Maarten any feedback on Ville's unaddressed comments?
> > 
> > Manasi
> > 
> > > 
> > > > 
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
> > > >  .../drm/i915/display/intel_display_types.h    |  2 +-
> > > >  drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
> > > >  drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
> > > >  4 files changed, 23 insertions(+), 26 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 729ec6e0d43a..8652a7c6bf11 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> > > >  	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
> > > >  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> > > >  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> > > > -
> > > > -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> > > > -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
> > > >  }
> > > >  
> > > >  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > > > @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> > > >  		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
> > > >  
> > > >  	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> > > > -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> > > > +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
> > > >  	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
> > > >  	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
> > > >  	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> > > > @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> > > >  {
> > > >  	crtc_state->hw.enable = crtc_state->uapi.enable;
> > > >  	crtc_state->hw.active = crtc_state->uapi.active;
> > > > -	crtc_state->hw.mode = crtc_state->uapi.mode;
> > > >  	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> > > >  	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> > > >  }
> > > >  
> > > > -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> > > > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > > +					     struct drm_display_mode *user_mode)
> > > >  {
> > > >  	crtc_state->uapi.enable = crtc_state->hw.enable;
> > > >  	crtc_state->uapi.active = crtc_state->hw.active;
> > > >  	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > > -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> > > > +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
> > > >  
> > > >  	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> > > >  
> > > > @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> > > >  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> > > >  	kfree(saved_state);
> > > >  
> > > > +	/* Clear I915_MODE_FLAG_INHERITED */
> > > > +	crtc_state->uapi.mode.private_flags = 0;
> > > > +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> > > > +
> > > >  	intel_crtc_copy_uapi_to_hw_state(crtc_state);
> > > >  
> > > >  	return 0;
> > > > @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > > >  	 * computation to clearly distinguish it from the adjusted mode, which
> > > >  	 * can be changed by the connectors in the below retry loop.
> > > >  	 */
> > > > -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> > > > +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
> > > >  			       &pipe_config->pipe_src_w,
> > > >  			       &pipe_config->pipe_src_h);
> > > >  
> > > > @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > >  		int min_cdclk = 0;
> > > >  
> > > >  		if (crtc_state->hw.active) {
> > > > -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > +			struct drm_display_mode mode;
> > > >  
> > > >  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> > > >  						    crtc_state);
> > > >  
> > > > -			*mode = crtc_state->hw.adjusted_mode;
> > > > -			mode->hdisplay = crtc_state->pipe_src_w;
> > > > -			mode->vdisplay = crtc_state->pipe_src_h;
> > > > -
> > > >  			/*
> > > >  			 * The initial mode needs to be set in order to keep
> > > >  			 * the atomic core happy. It wants a valid mode if the
> > > > @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > >  			 */
> > > >  			crtc_state->inherited = true;
> > > >  
> > > > +			mode = crtc_state->hw.adjusted_mode;
> > > > +			mode.hdisplay = crtc_state->pipe_src_w;
> > > > +			mode.vdisplay = crtc_state->pipe_src_h;
> > > > +
> > > >  			intel_crtc_compute_pixel_rate(crtc_state);
> > > >  
> > > >  			intel_crtc_update_active_timings(crtc_state);
> > > >  
> > > > -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > > > +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
> > > >  		}
> > > >  
> > > >  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index e8f809161c75..f1e29d9a75d0 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -807,7 +807,7 @@ struct intel_crtc_state {
> > > >  	struct {
> > > >  		bool active, enable;
> > > >  		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> > > > -		struct drm_display_mode mode, adjusted_mode;
> > > > +		struct drm_display_mode adjusted_mode;
> > > >  	} hw;
> > > >  
> > > >  	/**
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > index 307ed8ae9a19..0b9bf1fec0f4 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
> > > >  	u32 temp = intel_de_read(dev_priv, dvo_reg);
> > > >  
> > > >  	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> > > > -					 &pipe_config->hw.mode,
> > > > +					 &pipe_config->hw.adjusted_mode,
> > > >  					 &pipe_config->hw.adjusted_mode);
> > > >  
> > > >  	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > index 2da4388e1540..8b78ae0c39a0 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
> > > >  static bool
> > > >  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > >  				    struct intel_sdvo_connector *intel_sdvo_connector,
> > > > -				    const struct drm_display_mode *mode,
> > > >  				    struct drm_display_mode *adjusted_mode)
> > > >  {
> > > >  	struct intel_sdvo_dtd input_dtd;
> > > > @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > >  
> > > >  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > > >  						      intel_sdvo_connector,
> > > > -						      mode->clock / 10,
> > > > -						      mode->hdisplay,
> > > > -						      mode->vdisplay))
> > > > +						      adjusted_mode->clock / 10,
> > > > +						      adjusted_mode->hdisplay,
> > > > +						      adjusted_mode->vdisplay))
> > > >  		return false;
> > > >  
> > > >  	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> > > > @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > >  	struct intel_sdvo_connector *intel_sdvo_connector =
> > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > >  	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> > > > -	struct drm_display_mode *mode = &pipe_config->hw.mode;
> > > >  
> > > >  	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> > > >  	pipe_config->pipe_bpp = 8*3;
> > > > @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > >  	 * the sequence to do it. Oh well.
> > > >  	 */
> > > >  	if (IS_TV(intel_sdvo_connector)) {
> > > > -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > > > +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > > > +							     adjusted_mode))
> > > >  			return -EINVAL;
> > > >  
> > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > >  							   intel_sdvo_connector,
> > > > -							   mode,
> > > >  							   adjusted_mode);
> > > >  		pipe_config->sdvo_tv_clock = true;
> > > >  	} else if (IS_LVDS(intel_sdvo_connector)) {
> > > > @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > >  
> > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > >  							   intel_sdvo_connector,
> > > > -							   mode,
> > > >  							   adjusted_mode);
> > > >  	}
> > > >  
> > > > @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > >  		to_intel_sdvo_connector_state(conn_state);
> > > >  	const struct intel_sdvo_connector *intel_sdvo_connector =
> > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > > -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > >  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > > >  	u32 sdvox;
> > > >  	struct intel_sdvo_in_out_map in_out;
> > > > @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > >  		intel_sdvo_get_dtd_from_mode(&output_dtd,
> > > >  					     intel_sdvo_connector->base.panel.fixed_mode);
> > > >  	else
> > > > -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > > > +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
> > > >  	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
> > > >  		drm_info(&dev_priv->drm,
> > > >  			 "Setting output timings on %s failed\n",
> > > > -- 
> > > > 2.19.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi Sept. 14, 2020, 6:32 p.m. UTC | #6

On Mon, Sep 07, 2020 at 03:35:23PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > 
> > > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > > use that when available.
> > > > > 
> > > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > > as well.
> > > > > 
> > > > > v2:
> > > > > * Manual rebase (Manasi)
> > > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > > 
> > > > > v3:
> > > > > * Rebase on drm-tip (Manasi)
> > > > 
> > > > Previous review was apparently ignored. Or is there a better version
> > > > somewhere? If not, this still looks very wrong.
> > > 
> > > This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> > > What in particular looks wrong or what review comments were unaddressed here?
> > 
> > The dvo/sdvo changes.
> 
> I recommend just dropping this patch entirely. It doesn't seem to have
> anything to do with the bigjoiner anyway.

So for the dvo/svdo changes, no need to use the adjusted_mode instead keep using hw.mode?
How about other cleanups like: intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode); and
static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
+					     struct drm_display_mode *user_mode)

You think we dont need mode as an argument there either?

Manasi
> 
> > 
> > > 
> > > @Maarten any feedback on Ville's unaddressed comments?
> > > 
> > > Manasi
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
> > > > >  .../drm/i915/display/intel_display_types.h    |  2 +-
> > > > >  drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
> > > > >  drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
> > > > >  4 files changed, 23 insertions(+), 26 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 729ec6e0d43a..8652a7c6bf11 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> > > > >  	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
> > > > >  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> > > > >  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> > > > > -
> > > > > -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> > > > > -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
> > > > >  }
> > > > >  
> > > > >  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > > > > @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> > > > >  		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
> > > > >  
> > > > >  	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> > > > > -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> > > > > +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
> > > > >  	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
> > > > >  	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
> > > > >  	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> > > > > @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> > > > >  {
> > > > >  	crtc_state->hw.enable = crtc_state->uapi.enable;
> > > > >  	crtc_state->hw.active = crtc_state->uapi.active;
> > > > > -	crtc_state->hw.mode = crtc_state->uapi.mode;
> > > > >  	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> > > > >  	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> > > > >  }
> > > > >  
> > > > > -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> > > > > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > > > +					     struct drm_display_mode *user_mode)
> > > > >  {
> > > > >  	crtc_state->uapi.enable = crtc_state->hw.enable;
> > > > >  	crtc_state->uapi.active = crtc_state->hw.active;
> > > > >  	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > > > -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> > > > > +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
> > > > >  
> > > > >  	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> > > > >  
> > > > > @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> > > > >  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> > > > >  	kfree(saved_state);
> > > > >  
> > > > > +	/* Clear I915_MODE_FLAG_INHERITED */
> > > > > +	crtc_state->uapi.mode.private_flags = 0;
> > > > > +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> > > > > +
> > > > >  	intel_crtc_copy_uapi_to_hw_state(crtc_state);
> > > > >  
> > > > >  	return 0;
> > > > > @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > > > >  	 * computation to clearly distinguish it from the adjusted mode, which
> > > > >  	 * can be changed by the connectors in the below retry loop.
> > > > >  	 */
> > > > > -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> > > > > +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
> > > > >  			       &pipe_config->pipe_src_w,
> > > > >  			       &pipe_config->pipe_src_h);
> > > > >  
> > > > > @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > >  		int min_cdclk = 0;
> > > > >  
> > > > >  		if (crtc_state->hw.active) {
> > > > > -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > > +			struct drm_display_mode mode;
> > > > >  
> > > > >  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> > > > >  						    crtc_state);
> > > > >  
> > > > > -			*mode = crtc_state->hw.adjusted_mode;
> > > > > -			mode->hdisplay = crtc_state->pipe_src_w;
> > > > > -			mode->vdisplay = crtc_state->pipe_src_h;
> > > > > -
> > > > >  			/*
> > > > >  			 * The initial mode needs to be set in order to keep
> > > > >  			 * the atomic core happy. It wants a valid mode if the
> > > > > @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > >  			 */
> > > > >  			crtc_state->inherited = true;
> > > > >  
> > > > > +			mode = crtc_state->hw.adjusted_mode;
> > > > > +			mode.hdisplay = crtc_state->pipe_src_w;
> > > > > +			mode.vdisplay = crtc_state->pipe_src_h;
> > > > > +
> > > > >  			intel_crtc_compute_pixel_rate(crtc_state);
> > > > >  
> > > > >  			intel_crtc_update_active_timings(crtc_state);
> > > > >  
> > > > > -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > > > > +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
> > > > >  		}
> > > > >  
> > > > >  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > index e8f809161c75..f1e29d9a75d0 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > @@ -807,7 +807,7 @@ struct intel_crtc_state {
> > > > >  	struct {
> > > > >  		bool active, enable;
> > > > >  		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> > > > > -		struct drm_display_mode mode, adjusted_mode;
> > > > > +		struct drm_display_mode adjusted_mode;
> > > > >  	} hw;
> > > > >  
> > > > >  	/**
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > index 307ed8ae9a19..0b9bf1fec0f4 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
> > > > >  	u32 temp = intel_de_read(dev_priv, dvo_reg);
> > > > >  
> > > > >  	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> > > > > -					 &pipe_config->hw.mode,
> > > > > +					 &pipe_config->hw.adjusted_mode,
> > > > >  					 &pipe_config->hw.adjusted_mode);
> > > > >  
> > > > >  	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > index 2da4388e1540..8b78ae0c39a0 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
> > > > >  static bool
> > > > >  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > > >  				    struct intel_sdvo_connector *intel_sdvo_connector,
> > > > > -				    const struct drm_display_mode *mode,
> > > > >  				    struct drm_display_mode *adjusted_mode)
> > > > >  {
> > > > >  	struct intel_sdvo_dtd input_dtd;
> > > > > @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > > >  
> > > > >  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > > > >  						      intel_sdvo_connector,
> > > > > -						      mode->clock / 10,
> > > > > -						      mode->hdisplay,
> > > > > -						      mode->vdisplay))
> > > > > +						      adjusted_mode->clock / 10,
> > > > > +						      adjusted_mode->hdisplay,
> > > > > +						      adjusted_mode->vdisplay))
> > > > >  		return false;
> > > > >  
> > > > >  	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> > > > > @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > >  	struct intel_sdvo_connector *intel_sdvo_connector =
> > > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > > >  	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> > > > > -	struct drm_display_mode *mode = &pipe_config->hw.mode;
> > > > >  
> > > > >  	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> > > > >  	pipe_config->pipe_bpp = 8*3;
> > > > > @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > >  	 * the sequence to do it. Oh well.
> > > > >  	 */
> > > > >  	if (IS_TV(intel_sdvo_connector)) {
> > > > > -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > > > > +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > > > > +							     adjusted_mode))
> > > > >  			return -EINVAL;
> > > > >  
> > > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > >  							   intel_sdvo_connector,
> > > > > -							   mode,
> > > > >  							   adjusted_mode);
> > > > >  		pipe_config->sdvo_tv_clock = true;
> > > > >  	} else if (IS_LVDS(intel_sdvo_connector)) {
> > > > > @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > >  
> > > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > >  							   intel_sdvo_connector,
> > > > > -							   mode,
> > > > >  							   adjusted_mode);
> > > > >  	}
> > > > >  
> > > > > @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > > >  		to_intel_sdvo_connector_state(conn_state);
> > > > >  	const struct intel_sdvo_connector *intel_sdvo_connector =
> > > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > > > -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > >  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > > > >  	u32 sdvox;
> > > > >  	struct intel_sdvo_in_out_map in_out;
> > > > > @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > > >  		intel_sdvo_get_dtd_from_mode(&output_dtd,
> > > > >  					     intel_sdvo_connector->base.panel.fixed_mode);
> > > > >  	else
> > > > > -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > > > > +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
> > > > >  	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
> > > > >  		drm_info(&dev_priv->drm,
> > > > >  			 "Setting output timings on %s failed\n",
> > > > > -- 
> > > > > 2.19.1
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala Sept. 14, 2020, 6:52 p.m. UTC | #7

On Mon, Sep 14, 2020 at 11:32:48AM -0700, Navare, Manasi wrote:
> On Mon, Sep 07, 2020 at 03:35:23PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > > > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > 
> > > > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > > > use that when available.
> > > > > > 
> > > > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > > > as well.
> > > > > > 
> > > > > > v2:
> > > > > > * Manual rebase (Manasi)
> > > > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > > > 
> > > > > > v3:
> > > > > > * Rebase on drm-tip (Manasi)
> > > > > 
> > > > > Previous review was apparently ignored. Or is there a better version
> > > > > somewhere? If not, this still looks very wrong.
> > > > 
> > > > This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> > > > What in particular looks wrong or what review comments were unaddressed here?
> > > 
> > > The dvo/sdvo changes.
> > 
> > I recommend just dropping this patch entirely. It doesn't seem to have
> > anything to do with the bigjoiner anyway.
> 
> So for the dvo/svdo changes, no need to use the adjusted_mode instead keep using hw.mode?
> How about other cleanups like: intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode); and
> static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> +					     struct drm_display_mode *user_mode)
> 
> You think we dont need mode as an argument there either?

Not in this patch if all the other stuff disappears. No idea if some
later patch might need something like it.

> 
> Manasi
> > 
> > > 
> > > > 
> > > > @Maarten any feedback on Ville's unaddressed comments?
> > > > 
> > > > Manasi
> > > > 
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
> > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +-
> > > > > >  drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
> > > > > >  drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
> > > > > >  4 files changed, 23 insertions(+), 26 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > index 729ec6e0d43a..8652a7c6bf11 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> > > > > >  	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
> > > > > >  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> > > > > >  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> > > > > > -
> > > > > > -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> > > > > > -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
> > > > > >  }
> > > > > >  
> > > > > >  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > > > > > @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> > > > > >  		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
> > > > > >  
> > > > > >  	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> > > > > > -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> > > > > > +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
> > > > > >  	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
> > > > > >  	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
> > > > > >  	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> > > > > > @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> > > > > >  {
> > > > > >  	crtc_state->hw.enable = crtc_state->uapi.enable;
> > > > > >  	crtc_state->hw.active = crtc_state->uapi.active;
> > > > > > -	crtc_state->hw.mode = crtc_state->uapi.mode;
> > > > > >  	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> > > > > >  	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> > > > > >  }
> > > > > >  
> > > > > > -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> > > > > > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > > > > +					     struct drm_display_mode *user_mode)
> > > > > >  {
> > > > > >  	crtc_state->uapi.enable = crtc_state->hw.enable;
> > > > > >  	crtc_state->uapi.active = crtc_state->hw.active;
> > > > > >  	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > > > > -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> > > > > > +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
> > > > > >  
> > > > > >  	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> > > > > >  
> > > > > > @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> > > > > >  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> > > > > >  	kfree(saved_state);
> > > > > >  
> > > > > > +	/* Clear I915_MODE_FLAG_INHERITED */
> > > > > > +	crtc_state->uapi.mode.private_flags = 0;
> > > > > > +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> > > > > > +
> > > > > >  	intel_crtc_copy_uapi_to_hw_state(crtc_state);
> > > > > >  
> > > > > >  	return 0;
> > > > > > @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > > > > >  	 * computation to clearly distinguish it from the adjusted mode, which
> > > > > >  	 * can be changed by the connectors in the below retry loop.
> > > > > >  	 */
> > > > > > -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> > > > > > +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
> > > > > >  			       &pipe_config->pipe_src_w,
> > > > > >  			       &pipe_config->pipe_src_h);
> > > > > >  
> > > > > > @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > > >  		int min_cdclk = 0;
> > > > > >  
> > > > > >  		if (crtc_state->hw.active) {
> > > > > > -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > > > +			struct drm_display_mode mode;
> > > > > >  
> > > > > >  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> > > > > >  						    crtc_state);
> > > > > >  
> > > > > > -			*mode = crtc_state->hw.adjusted_mode;
> > > > > > -			mode->hdisplay = crtc_state->pipe_src_w;
> > > > > > -			mode->vdisplay = crtc_state->pipe_src_h;
> > > > > > -
> > > > > >  			/*
> > > > > >  			 * The initial mode needs to be set in order to keep
> > > > > >  			 * the atomic core happy. It wants a valid mode if the
> > > > > > @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > > >  			 */
> > > > > >  			crtc_state->inherited = true;
> > > > > >  
> > > > > > +			mode = crtc_state->hw.adjusted_mode;
> > > > > > +			mode.hdisplay = crtc_state->pipe_src_w;
> > > > > > +			mode.vdisplay = crtc_state->pipe_src_h;
> > > > > > +
> > > > > >  			intel_crtc_compute_pixel_rate(crtc_state);
> > > > > >  
> > > > > >  			intel_crtc_update_active_timings(crtc_state);
> > > > > >  
> > > > > > -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > > > > > +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
> > > > > >  		}
> > > > > >  
> > > > > >  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > index e8f809161c75..f1e29d9a75d0 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > @@ -807,7 +807,7 @@ struct intel_crtc_state {
> > > > > >  	struct {
> > > > > >  		bool active, enable;
> > > > > >  		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> > > > > > -		struct drm_display_mode mode, adjusted_mode;
> > > > > > +		struct drm_display_mode adjusted_mode;
> > > > > >  	} hw;
> > > > > >  
> > > > > >  	/**
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > index 307ed8ae9a19..0b9bf1fec0f4 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
> > > > > >  	u32 temp = intel_de_read(dev_priv, dvo_reg);
> > > > > >  
> > > > > >  	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> > > > > > -					 &pipe_config->hw.mode,
> > > > > > +					 &pipe_config->hw.adjusted_mode,
> > > > > >  					 &pipe_config->hw.adjusted_mode);
> > > > > >  
> > > > > >  	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > index 2da4388e1540..8b78ae0c39a0 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
> > > > > >  static bool
> > > > > >  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > > > >  				    struct intel_sdvo_connector *intel_sdvo_connector,
> > > > > > -				    const struct drm_display_mode *mode,
> > > > > >  				    struct drm_display_mode *adjusted_mode)
> > > > > >  {
> > > > > >  	struct intel_sdvo_dtd input_dtd;
> > > > > > @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > > > >  
> > > > > >  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > > > > >  						      intel_sdvo_connector,
> > > > > > -						      mode->clock / 10,
> > > > > > -						      mode->hdisplay,
> > > > > > -						      mode->vdisplay))
> > > > > > +						      adjusted_mode->clock / 10,
> > > > > > +						      adjusted_mode->hdisplay,
> > > > > > +						      adjusted_mode->vdisplay))
> > > > > >  		return false;
> > > > > >  
> > > > > >  	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> > > > > > @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > > >  	struct intel_sdvo_connector *intel_sdvo_connector =
> > > > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > > > >  	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> > > > > > -	struct drm_display_mode *mode = &pipe_config->hw.mode;
> > > > > >  
> > > > > >  	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> > > > > >  	pipe_config->pipe_bpp = 8*3;
> > > > > > @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > > >  	 * the sequence to do it. Oh well.
> > > > > >  	 */
> > > > > >  	if (IS_TV(intel_sdvo_connector)) {
> > > > > > -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > > > > > +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > > > > > +							     adjusted_mode))
> > > > > >  			return -EINVAL;
> > > > > >  
> > > > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > > >  							   intel_sdvo_connector,
> > > > > > -							   mode,
> > > > > >  							   adjusted_mode);
> > > > > >  		pipe_config->sdvo_tv_clock = true;
> > > > > >  	} else if (IS_LVDS(intel_sdvo_connector)) {
> > > > > > @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > > >  
> > > > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > > >  							   intel_sdvo_connector,
> > > > > > -							   mode,
> > > > > >  							   adjusted_mode);
> > > > > >  	}
> > > > > >  
> > > > > > @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > > > >  		to_intel_sdvo_connector_state(conn_state);
> > > > > >  	const struct intel_sdvo_connector *intel_sdvo_connector =
> > > > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > > > > -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > > >  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > > > > >  	u32 sdvox;
> > > > > >  	struct intel_sdvo_in_out_map in_out;
> > > > > > @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > > > >  		intel_sdvo_get_dtd_from_mode(&output_dtd,
> > > > > >  					     intel_sdvo_connector->base.panel.fixed_mode);
> > > > > >  	else
> > > > > > -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > > > > > +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
> > > > > >  	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
> > > > > >  		drm_info(&dev_priv->drm,
> > > > > >  			 "Setting output timings on %s failed\n",
> > > > > > -- 
> > > > > > 2.19.1
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Navare, Manasi Sept. 21, 2020, 9:01 p.m. UTC | #8

On Mon, Sep 14, 2020 at 09:52:57PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 14, 2020 at 11:32:48AM -0700, Navare, Manasi wrote:
> > On Mon, Sep 07, 2020 at 03:35:23PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > > > > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > > > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > 
> > > > > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > > > > use that when available.
> > > > > > > 
> > > > > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > > > > as well.
> > > > > > > 
> > > > > > > v2:
> > > > > > > * Manual rebase (Manasi)
> > > > > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > > > > 
> > > > > > > v3:
> > > > > > > * Rebase on drm-tip (Manasi)
> > > > > > 
> > > > > > Previous review was apparently ignored. Or is there a better version
> > > > > > somewhere? If not, this still looks very wrong.
> > > > > 
> > > > > This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> > > > > What in particular looks wrong or what review comments were unaddressed here?
> > > > 
> > > > The dvo/sdvo changes.
> > > 
> > > I recommend just dropping this patch entirely. It doesn't seem to have
> > > anything to do with the bigjoiner anyway.
> > 
> > So for the dvo/svdo changes, no need to use the adjusted_mode instead keep using hw.mode?
> > How about other cleanups like: intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode); and
> > static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > +					     struct drm_display_mode *user_mode)
> > 
> > You think we dont need mode as an argument there either?
> 
> Not in this patch if all the other stuff disappears. No idea if some
> later patch might need something like it.

Hi Ville,

So this patch basically removes the hw.mode and just keeps hw.adjusted_mode
So no need to remove that? 
But basically from this patch onwards we say that there is hw.pipe_mode
and hw.adjusted_mode, there is no hw.mode.
Are you suggesting keeping hw.mode as well? Would this be replacing hw.pipe_mode then?

Manasi

> 
> > 
> > Manasi
> > > 
> > > > 
> > > > > 
> > > > > @Maarten any feedback on Ville's unaddressed comments?
> > > > > 
> > > > > Manasi
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/display/intel_display.c  | 29 ++++++++++---------
> > > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +-
> > > > > > >  drivers/gpu/drm/i915/display/intel_dvo.c      |  2 +-
> > > > > > >  drivers/gpu/drm/i915/display/intel_sdvo.c     | 16 ++++------
> > > > > > >  4 files changed, 23 insertions(+), 26 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > index 729ec6e0d43a..8652a7c6bf11 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> > > > > > >  	tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
> > > > > > >  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> > > > > > >  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> > > > > > > -
> > > > > > > -	pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> > > > > > > -	pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
> > > > > > >  }
> > > > > > >  
> > > > > > >  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > > > > > > @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> > > > > > >  		intel_dump_dp_vsc_sdp(dev_priv, &pipe_config->infoframes.vsc);
> > > > > > >  
> > > > > > >  	drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> > > > > > > -	drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> > > > > > > +	drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
> > > > > > >  	drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
> > > > > > >  	drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
> > > > > > >  	intel_dump_crtc_timings(dev_priv, &pipe_config->hw.adjusted_mode);
> > > > > > > @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> > > > > > >  {
> > > > > > >  	crtc_state->hw.enable = crtc_state->uapi.enable;
> > > > > > >  	crtc_state->hw.active = crtc_state->uapi.active;
> > > > > > > -	crtc_state->hw.mode = crtc_state->uapi.mode;
> > > > > > >  	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> > > > > > >  	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> > > > > > >  }
> > > > > > >  
> > > > > > > -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> > > > > > > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > > > > > +					     struct drm_display_mode *user_mode)
> > > > > > >  {
> > > > > > >  	crtc_state->uapi.enable = crtc_state->hw.enable;
> > > > > > >  	crtc_state->uapi.active = crtc_state->hw.active;
> > > > > > >  	drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > > > > > -		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> > > > > > > +		    drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
> > > > > > >  
> > > > > > >  	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> > > > > > >  
> > > > > > > @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> > > > > > >  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> > > > > > >  	kfree(saved_state);
> > > > > > >  
> > > > > > > +	/* Clear I915_MODE_FLAG_INHERITED */
> > > > > > > +	crtc_state->uapi.mode.private_flags = 0;
> > > > > > > +	crtc_state->uapi.adjusted_mode.private_flags = 0;
> > > > > > > +
> > > > > > >  	intel_crtc_copy_uapi_to_hw_state(crtc_state);
> > > > > > >  
> > > > > > >  	return 0;
> > > > > > > @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > > > > > >  	 * computation to clearly distinguish it from the adjusted mode, which
> > > > > > >  	 * can be changed by the connectors in the below retry loop.
> > > > > > >  	 */
> > > > > > > -	drm_mode_get_hv_timing(&pipe_config->hw.mode,
> > > > > > > +	drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
> > > > > > >  			       &pipe_config->pipe_src_w,
> > > > > > >  			       &pipe_config->pipe_src_h);
> > > > > > >  
> > > > > > > @@ -18461,15 +18462,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > > > >  		int min_cdclk = 0;
> > > > > > >  
> > > > > > >  		if (crtc_state->hw.active) {
> > > > > > > -			struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > > > > +			struct drm_display_mode mode;
> > > > > > >  
> > > > > > >  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> > > > > > >  						    crtc_state);
> > > > > > >  
> > > > > > > -			*mode = crtc_state->hw.adjusted_mode;
> > > > > > > -			mode->hdisplay = crtc_state->pipe_src_w;
> > > > > > > -			mode->vdisplay = crtc_state->pipe_src_h;
> > > > > > > -
> > > > > > >  			/*
> > > > > > >  			 * The initial mode needs to be set in order to keep
> > > > > > >  			 * the atomic core happy. It wants a valid mode if the
> > > > > > > @@ -18481,11 +18478,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > > > >  			 */
> > > > > > >  			crtc_state->inherited = true;
> > > > > > >  
> > > > > > > +			mode = crtc_state->hw.adjusted_mode;
> > > > > > > +			mode.hdisplay = crtc_state->pipe_src_w;
> > > > > > > +			mode.vdisplay = crtc_state->pipe_src_h;
> > > > > > > +
> > > > > > >  			intel_crtc_compute_pixel_rate(crtc_state);
> > > > > > >  
> > > > > > >  			intel_crtc_update_active_timings(crtc_state);
> > > > > > >  
> > > > > > > -			intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > > > > > > +			intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode);
> > > > > > >  		}
> > > > > > >  
> > > > > > >  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > index e8f809161c75..f1e29d9a75d0 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > @@ -807,7 +807,7 @@ struct intel_crtc_state {
> > > > > > >  	struct {
> > > > > > >  		bool active, enable;
> > > > > > >  		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> > > > > > > -		struct drm_display_mode mode, adjusted_mode;
> > > > > > > +		struct drm_display_mode adjusted_mode;
> > > > > > >  	} hw;
> > > > > > >  
> > > > > > >  	/**
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > > index 307ed8ae9a19..0b9bf1fec0f4 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > > @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct intel_atomic_state *state,
> > > > > > >  	u32 temp = intel_de_read(dev_priv, dvo_reg);
> > > > > > >  
> > > > > > >  	intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> > > > > > > -					 &pipe_config->hw.mode,
> > > > > > > +					 &pipe_config->hw.adjusted_mode,
> > > > > > >  					 &pipe_config->hw.adjusted_mode);
> > > > > > >  
> > > > > > >  	intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > > index 2da4388e1540..8b78ae0c39a0 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > > @@ -1223,7 +1223,6 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
> > > > > > >  static bool
> > > > > > >  intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > > > > >  				    struct intel_sdvo_connector *intel_sdvo_connector,
> > > > > > > -				    const struct drm_display_mode *mode,
> > > > > > >  				    struct drm_display_mode *adjusted_mode)
> > > > > > >  {
> > > > > > >  	struct intel_sdvo_dtd input_dtd;
> > > > > > > @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > > > > >  
> > > > > > >  	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > > > > > >  						      intel_sdvo_connector,
> > > > > > > -						      mode->clock / 10,
> > > > > > > -						      mode->hdisplay,
> > > > > > > -						      mode->vdisplay))
> > > > > > > +						      adjusted_mode->clock / 10,
> > > > > > > +						      adjusted_mode->hdisplay,
> > > > > > > +						      adjusted_mode->vdisplay))
> > > > > > >  		return false;
> > > > > > >  
> > > > > > >  	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> > > > > > > @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > > > >  	struct intel_sdvo_connector *intel_sdvo_connector =
> > > > > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > > > > >  	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> > > > > > > -	struct drm_display_mode *mode = &pipe_config->hw.mode;
> > > > > > >  
> > > > > > >  	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> > > > > > >  	pipe_config->pipe_bpp = 8*3;
> > > > > > > @@ -1324,12 +1322,12 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > > > >  	 * the sequence to do it. Oh well.
> > > > > > >  	 */
> > > > > > >  	if (IS_TV(intel_sdvo_connector)) {
> > > > > > > -		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > > > > > > +		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > > > > > > +							     adjusted_mode))
> > > > > > >  			return -EINVAL;
> > > > > > >  
> > > > > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > > > >  							   intel_sdvo_connector,
> > > > > > > -							   mode,
> > > > > > >  							   adjusted_mode);
> > > > > > >  		pipe_config->sdvo_tv_clock = true;
> > > > > > >  	} else if (IS_LVDS(intel_sdvo_connector)) {
> > > > > > > @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > > > >  
> > > > > > >  		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > > > >  							   intel_sdvo_connector,
> > > > > > > -							   mode,
> > > > > > >  							   adjusted_mode);
> > > > > > >  	}
> > > > > > >  
> > > > > > > @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > > > > >  		to_intel_sdvo_connector_state(conn_state);
> > > > > > >  	const struct intel_sdvo_connector *intel_sdvo_connector =
> > > > > > >  		to_intel_sdvo_connector(conn_state->connector);
> > > > > > > -	const struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > > > >  	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > > > > > >  	u32 sdvox;
> > > > > > >  	struct intel_sdvo_in_out_map in_out;
> > > > > > > @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct intel_atomic_state *state,
> > > > > > >  		intel_sdvo_get_dtd_from_mode(&output_dtd,
> > > > > > >  					     intel_sdvo_connector->base.panel.fixed_mode);
> > > > > > >  	else
> > > > > > > -		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > > > > > > +		intel_sdvo_get_dtd_from_mode(&output_dtd, adjusted_mode);
> > > > > > >  	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
> > > > > > >  		drm_info(&dev_priv->drm,
> > > > > > >  			 "Setting output timings on %s failed\n",
> > > > > > > -- 
> > > > > > > 2.19.1
> > > > > > > 
> > > > > > > _______________________________________________
> > > > > > > Intel-gfx mailing list
> > > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > > 
> > > > > > -- 
> > > > > > Ville Syrjälä
> > > > > > Intel
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala Sept. 22, 2020, 10:19 a.m. UTC | #9

On Mon, Sep 21, 2020 at 02:01:25PM -0700, Navare, Manasi wrote:
> On Mon, Sep 14, 2020 at 09:52:57PM +0300, Ville Syrjälä wrote:
> > On Mon, Sep 14, 2020 at 11:32:48AM -0700, Navare, Manasi wrote:
> > > On Mon, Sep 07, 2020 at 03:35:23PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> > > > > On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > > > > > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > > > > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > > > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > 
> > > > > > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > > > > > use that when available.
> > > > > > > > 
> > > > > > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > > > > > as well.
> > > > > > > > 
> > > > > > > > v2:
> > > > > > > > * Manual rebase (Manasi)
> > > > > > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > > > > > 
> > > > > > > > v3:
> > > > > > > > * Rebase on drm-tip (Manasi)
> > > > > > > 
> > > > > > > Previous review was apparently ignored. Or is there a better version
> > > > > > > somewhere? If not, this still looks very wrong.
> > > > > > 
> > > > > > This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> > > > > > What in particular looks wrong or what review comments were unaddressed here?
> > > > > 
> > > > > The dvo/sdvo changes.
> > > > 
> > > > I recommend just dropping this patch entirely. It doesn't seem to have
> > > > anything to do with the bigjoiner anyway.
> > > 
> > > So for the dvo/svdo changes, no need to use the adjusted_mode instead keep using hw.mode?
> > > How about other cleanups like: intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode); and
> > > static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > +					     struct drm_display_mode *user_mode)
> > > 
> > > You think we dont need mode as an argument there either?
> > 
> > Not in this patch if all the other stuff disappears. No idea if some
> > later patch might need something like it.
> 
> Hi Ville,
> 
> So this patch basically removes the hw.mode and just keeps hw.adjusted_mode
> So no need to remove that? 
> But basically from this patch onwards we say that there is hw.pipe_mode
> and hw.adjusted_mode, there is no hw.mode.
> Are you suggesting keeping hw.mode as well? Would this be replacing hw.pipe_mode then?

No. hw.mode is the original timings, adjusted_mode is the output timings,
pipe_mode is the the pipe timings.

Navare, Manasi Sept. 22, 2020, 6:52 p.m. UTC | #10

On Tue, Sep 22, 2020 at 01:19:15PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 21, 2020 at 02:01:25PM -0700, Navare, Manasi wrote:
> > On Mon, Sep 14, 2020 at 09:52:57PM +0300, Ville Syrjälä wrote:
> > > On Mon, Sep 14, 2020 at 11:32:48AM -0700, Navare, Manasi wrote:
> > > > On Mon, Sep 07, 2020 at 03:35:23PM +0300, Ville Syrjälä wrote:
> > > > > On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> > > > > > On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > > > > > > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > > > > > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > > > > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > > 
> > > > > > > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > > > > > > use that when available.
> > > > > > > > > 
> > > > > > > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > > > > > > as well.
> > > > > > > > > 
> > > > > > > > > v2:
> > > > > > > > > * Manual rebase (Manasi)
> > > > > > > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > > > > > > 
> > > > > > > > > v3:
> > > > > > > > > * Rebase on drm-tip (Manasi)
> > > > > > > > 
> > > > > > > > Previous review was apparently ignored. Or is there a better version
> > > > > > > > somewhere? If not, this still looks very wrong.
> > > > > > > 
> > > > > > > This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> > > > > > > What in particular looks wrong or what review comments were unaddressed here?
> > > > > > 
> > > > > > The dvo/sdvo changes.
> > > > > 
> > > > > I recommend just dropping this patch entirely. It doesn't seem to have
> > > > > anything to do with the bigjoiner anyway.
> > > > 
> > > > So for the dvo/svdo changes, no need to use the adjusted_mode instead keep using hw.mode?
> > > > How about other cleanups like: intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode); and
> > > > static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > > +					     struct drm_display_mode *user_mode)
> > > > 
> > > > You think we dont need mode as an argument there either?
> > > 
> > > Not in this patch if all the other stuff disappears. No idea if some
> > > later patch might need something like it.
> > 
> > Hi Ville,
> > 
> > So this patch basically removes the hw.mode and just keeps hw.adjusted_mode
> > So no need to remove that? 
> > But basically from this patch onwards we say that there is hw.pipe_mode
> > and hw.adjusted_mode, there is no hw.mode.
> > Are you suggesting keeping hw.mode as well? Would this be replacing hw.pipe_mode then?
> 
> No. hw.mode is the original timings, adjusted_mode is the output timings,
> pipe_mode is the the pipe timings.

So is the suggestion to keep hw.mode so the original timings as well as adjusted_mode and
then have pipe_mode for per pipe timings.
So get rid of this patch meaning do not remove hw.mode?

Manasi

> 
> -- 
> Ville Syrjälä
> Intel

Navare, Manasi Sept. 23, 2020, 2:54 p.m. UTC | #11

Hi Ville,

So to confirm I am skipping this patch completely.
So basically keeping hw.mode as well

Manasi

On Tue, Sep 22, 2020 at 11:52:09AM -0700, Navare, Manasi wrote:
> On Tue, Sep 22, 2020 at 01:19:15PM +0300, Ville Syrjälä wrote:
> > On Mon, Sep 21, 2020 at 02:01:25PM -0700, Navare, Manasi wrote:
> > > On Mon, Sep 14, 2020 at 09:52:57PM +0300, Ville Syrjälä wrote:
> > > > On Mon, Sep 14, 2020 at 11:32:48AM -0700, Navare, Manasi wrote:
> > > > > On Mon, Sep 07, 2020 at 03:35:23PM +0300, Ville Syrjälä wrote:
> > > > > > On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> > > > > > > On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > > > > > > > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > > > > > > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > > > > > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > > > 
> > > > > > > > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > > > > > > > use that when available.
> > > > > > > > > > 
> > > > > > > > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > > > > > > > as well.
> > > > > > > > > > 
> > > > > > > > > > v2:
> > > > > > > > > > * Manual rebase (Manasi)
> > > > > > > > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > > > > > > > 
> > > > > > > > > > v3:
> > > > > > > > > > * Rebase on drm-tip (Manasi)
> > > > > > > > > 
> > > > > > > > > Previous review was apparently ignored. Or is there a better version
> > > > > > > > > somewhere? If not, this still looks very wrong.
> > > > > > > > 
> > > > > > > > This was the latest rev that Maarten had in his local tree which he said should address all the review comments.
> > > > > > > > What in particular looks wrong or what review comments were unaddressed here?
> > > > > > > 
> > > > > > > The dvo/sdvo changes.
> > > > > > 
> > > > > > I recommend just dropping this patch entirely. It doesn't seem to have
> > > > > > anything to do with the bigjoiner anyway.
> > > > > 
> > > > > So for the dvo/svdo changes, no need to use the adjusted_mode instead keep using hw.mode?
> > > > > How about other cleanups like: intel_crtc_copy_hw_to_uapi_state(crtc_state, &mode); and
> > > > > static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > > > +					     struct drm_display_mode *user_mode)
> > > > > 
> > > > > You think we dont need mode as an argument there either?
> > > > 
> > > > Not in this patch if all the other stuff disappears. No idea if some
> > > > later patch might need something like it.
> > > 
> > > Hi Ville,
> > > 
> > > So this patch basically removes the hw.mode and just keeps hw.adjusted_mode
> > > So no need to remove that? 
> > > But basically from this patch onwards we say that there is hw.pipe_mode
> > > and hw.adjusted_mode, there is no hw.mode.
> > > Are you suggesting keeping hw.mode as well? Would this be replacing hw.pipe_mode then?
> > 
> > No. hw.mode is the original timings, adjusted_mode is the output timings,
> > pipe_mode is the the pipe timings.
> 
> So is the suggestion to keep hw.mode so the original timings as well as adjusted_mode and
> then have pipe_mode for per pipe timings.
> So get rid of this patch meaning do not remove hw.mode?
> 
> Manasi
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[v6,02/11] drm/i915: Remove hw.mode

Commit Message

Comments

Patch