Re: [PATCH] drm/i915: Updating the CPU_TRANSCODER for BXT DSI

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 23 Feb 2016, Ramalingam C <ramalingam.c@intel.com> wrote:
> In case of BXT DSI we are updating the CPU_TRANSCODER
> with appropriate value. We are defining the POWER DOMAIN
> bits for the MIPI transcoders.

This doesn't actually say anything about *why* this change is needed.

> V2: Adding the power domain bits and mapping them to the
> 	corresponding Powerwell.

This doesn't apply on top of drm-intel-nightly, because it depends on
other patches. CI fails to run. Please include the entire series.

More importantly, what I've tried, and apparently failed, to explain
time and time again, you just can't add new transcoder enums, stick them
to ->cpu_transcoder, and expect things to work.

For example, just a little bit further in haswell_get_pipe_config(),
you'll hit

	if (pipe_config->cpu_transcoder != TRANSCODER_EDP) {
		pipe_config->pixel_multiplier =
			I915_READ(PIPE_MULT(pipe_config->cpu_transcoder)) + 1;
	} else {
		pipe_config->pixel_multiplier = 1;
	}

which will end up reading some bogus MMIO offset.

I've tried to explain that, if you think you need to add DSI transcoder
enums, you actually have to review *every* place they're used, and
figure out if they're relevant and need to be updated. Not all of them
need to. It gets tricky.

To give you an idea,

$ git grep cpu_transcoder -- drivers/gpu/drm/i915 | wc -l
147
$ git grep "enum transcoder" -- drivers/gpu/drm/i915 | wc -l
49

I am not even sure the cpu_transcoder is the right abstraction for DSI
transcoders. Are they totally independent?


BR,
Jani.


>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
> Previous review link: https://lists.freedesktop.org/archives/intel-gfx/2016-February/088304.html
>
>  drivers/gpu/drm/i915/i915_drv.h         |    4 ++++
>  drivers/gpu/drm/i915/intel_display.c    |    5 +++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |    6 ++++++
>  3 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6644c2e..7c177e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -122,6 +122,8 @@ enum transcoder {
>  	TRANSCODER_B,
>  	TRANSCODER_C,
>  	TRANSCODER_EDP,
> +	TRANSCODER_MIPI_A,
> +	TRANSCODER_MIPI_C,
>  	I915_MAX_TRANSCODERS
>  };
>  #define transcoder_name(t) ((t) + 'A')
> @@ -176,6 +178,8 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_TRANSCODER_B,
>  	POWER_DOMAIN_TRANSCODER_C,
>  	POWER_DOMAIN_TRANSCODER_EDP,
> +	POWER_DOMAIN_TRANSCODER_MIPI_A,
> +	POWER_DOMAIN_TRANSCODER_MIPI_C,
>  	POWER_DOMAIN_PORT_DDI_A_LANES,
>  	POWER_DOMAIN_PORT_DDI_B_LANES,
>  	POWER_DOMAIN_PORT_DDI_C_LANES,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 86ed9fd..961a968 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7825,6 +7825,11 @@ static void intel_get_dsi_pipe_timings(struct intel_crtc *crtc,
>  				enc_to_intel_dsi(&encoder->base);
>  			pipe_config->pipe_bpp =
>  				dsi_pixel_format_bpp(intel_dsi->pixel_format);
> +
> +			if (intel_dsi->ports & (1 << PORT_A))
> +				pipe_config->cpu_transcoder = TRANSCODER_MIPI_A;
> +			else
> +				pipe_config->cpu_transcoder = TRANSCODER_MIPI_C;
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index a2e367c..1ed3f5e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -89,6 +89,10 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "TRANSCODER_C";
>  	case POWER_DOMAIN_TRANSCODER_EDP:
>  		return "TRANSCODER_EDP";
> +	case POWER_DOMAIN_TRANSCODER_MIPI_A:
> +		return "TRANSCODER_MIPI_A";
> +	case POWER_DOMAIN_TRANSCODER_MIPI_C:
> +		return "TRANSCODER_MIPI_C";
>  	case POWER_DOMAIN_PORT_DDI_A_LANES:
>  		return "PORT_DDI_A_LANES";
>  	case POWER_DOMAIN_PORT_DDI_B_LANES:
> @@ -403,6 +407,8 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  	BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
>  	BIT(POWER_DOMAIN_PIPE_A) |			\
>  	BIT(POWER_DOMAIN_TRANSCODER_EDP) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_MIPI_A) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_MIPI_C) |		\
>  	BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER) |		\
>  	BIT(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
>  	BIT(POWER_DOMAIN_AUX_A) |			\

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx