[v4,1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

Message ID	1517304963-27932-2-git-send-email-shashank.sharma@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Shashank Sharma <shashank.sharma@intel.com> To: intel-gfx@lists.freedesktop.org Date: Tue, 30 Jan 2018 15:05:57 +0530 Message-Id: <1517304963-27932-2-git-send-email-shashank.sharma@intel.com> In-Reply-To: <1517304963-27932-1-git-send-email-shashank.sharma@intel.com> References: <1517304963-27932-1-git-send-email-shashank.sharma@intel.com> Subject: [Intel-gfx] [PATCH v4 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0 Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Sharma, Shashank Jan. 30, 2018, 9:35 a.m. UTC

From: "Sharma, Shashank" <shashank.sharma@intel.com>

Currently, we are using a bool in CRTC state (state->ycbcr420),
to indicate modeset, that the output format is YCBCR 4:2:0. Now in
order to support other YCBCR formats, we will need more such flags.

The idea behind this patch is to replace this bool with an enum,
and plug this in in the existing YCBCR 4:2:0 framework in such a
way that this can be re-used for YCBCR 4:4:4 and other output formats too.

This patch adds a new enum for CRTC output formats, and then plugs it
in the existing modeset framework.

V3: Added this patch in the series, to address review comments from
    second patchset.
V4: Added r-b from Maarten (on v3)
    Addressed review comments from Ville:
	- Change the enum name to intel_output_format from crtc_output_format.
	- Start the enum value (INVALID) from 0 instaed of 1.
        - Set the crtc's output_format to RGB in encoder's compute_config.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_color.c   |  2 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
 drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
 drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
 drivers/gpu/drm/i915/intel_panel.c   |  2 +-
 6 files changed, 61 insertions(+), 29 deletions(-)

Jani Nikula Jan. 30, 2018, 10:23 a.m. UTC | #1

On Tue, 30 Jan 2018, Shashank Sharma <shashank.sharma@intel.com> wrote:
> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>
> Currently, we are using a bool in CRTC state (state->ycbcr420),
> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
> order to support other YCBCR formats, we will need more such flags.
>
> The idea behind this patch is to replace this bool with an enum,
> and plug this in in the existing YCBCR 4:2:0 framework in such a
> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>
> This patch adds a new enum for CRTC output formats, and then plugs it
> in the existing modeset framework.
>
> V3: Added this patch in the series, to address review comments from
>     second patchset.
> V4: Added r-b from Maarten (on v3)
>     Addressed review comments from Ville:
> 	- Change the enum name to intel_output_format from crtc_output_format.
> 	- Start the enum value (INVALID) from 0 instaed of 1.
>         - Set the crtc's output_format to RGB in encoder's compute_config.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   |  2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>  drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
>  drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
>  drivers/gpu/drm/i915/intel_panel.c   |  2 +-
>  6 files changed, 61 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index aa66e95..99e32cb 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  	uint16_t coeffs[9] = { 0, };
>  	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>  
> -	if (intel_crtc_state->ycbcr420) {
> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>  		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>  		return;
>  	} else if (crtc_state->ctm) {
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index e51559b..d565e28 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>  	else
>  		dotclock = pipe_config->port_clock;
>  
> -	if (pipe_config->ycbcr420)
> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>  		dotclock *= 2;
>  
>  	if (pipe_config->pixel_multiplier)
> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
> +
>  	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>  		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>  	else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 83de43c..877336d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
> +	    scaler_user == SKL_CRTC_INDEX)
>  		need_scaling = true;
>  
>  	/*
> @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
> +	    pipe_config->base.ctm) {
>  		/*
>  		 * There is only one pipe CSC unit per pipe, and we need that
>  		 * for output conversion from RGB->YCBCR. So if CTM is already
> @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  		if (intel_crtc->config->dither)
>  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
> -		if (config->ycbcr420) {
> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> -				PIPEMISC_YUV420_ENABLE |
> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +			val |= PIPEMISC_YUV420_ENABLE |
> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;
>  		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +static const char * const output_format_str[] = {
> +	"Invalid",
> +	"RGB",
> +	"YCBCR4:2:0",

This should really be:

	[CRTC_OUTPUT_INVALID] = "Invalid",
	[CRTC_OUTPUT_RGB] = "RGB",
	[CRTC_OUTPUT_YCBCR420] = "YCBCR4:2:0",

BR,
Jani.

> +};
> +
> +static const char *output_formats(enum intel_output_format format)
> +{
> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> +		format = CRTC_OUTPUT_INVALID;
> +	return output_format_str[format];
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -9211,19 +9226,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  
>  	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>  		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
> -
> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
> -			bool blend_mode_420 = tmp &
> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
> +		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
> +
> +		if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
> +			bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
> +			bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
> +
> +			if (ycbcr420_enabled) {
> +				/* We support 4:2:0 in full blend mode only */
> +				if (!blend)
> +					output_format = CRTC_OUTPUT_INVALID;
> +				else if (!(IS_GEMINILAKE(dev_priv) ||
> +					   INTEL_GEN(dev_priv) >= 10))
> +					output_format = CRTC_OUTPUT_INVALID;
> +				else
> +					output_format = CRTC_OUTPUT_YCBCR420;
> +			}
>  
> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
> -			    pipe_config->ycbcr420 != blend_mode_420)
> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
> -		} else if (clrspace_yuv) {
> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>  		}
> +
> +		pipe_config->output_format = output_format;
> +		DRM_DEBUG_KMS("Output format %s\n",
> +				output_formats(output_format));
>  	}
>  
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> @@ -10578,6 +10602,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>  		      buf, pipe_config->output_types);
>  
> +	DRM_DEBUG_KMS("output format: %s\n",
> +		output_formats(pipe_config->output_format));
> +
>  	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>  		      transcoder_name(pipe_config->cpu_transcoder),
>  		      pipe_config->pipe_bpp, pipe_config->dither);
> @@ -10587,9 +10614,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  				      pipe_config->fdi_lanes,
>  				      &pipe_config->fdi_m_n);
>  
> -	if (pipe_config->ycbcr420)
> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
> -
>  	if (intel_crtc_has_dp_encoder(pipe_config)) {
>  		intel_dump_m_n_config(pipe_config, "dp m_n",
>  				pipe_config->lane_count, &pipe_config->dp_m_n);
> @@ -11163,6 +11187,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>  
>  	PIPE_CONF_CHECK_I(pixel_multiplier);
> +	PIPE_CONF_CHECK_I(output_format);
>  	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> @@ -11171,7 +11196,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>  	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>  	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
>  
>  	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3cee54b..35be78e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>  	bool need_postvbl_update;
>  };
>  
> +enum intel_output_format {
> +	CRTC_OUTPUT_INVALID,
> +	CRTC_OUTPUT_RGB,
> +	CRTC_OUTPUT_YCBCR420,
> +};
> +
>  struct intel_crtc_state {
>  	struct drm_crtc_state base;
>  
> @@ -873,8 +879,8 @@ struct intel_crtc_state {
>  	/* HDMI High TMDS char rate ratio */
>  	bool hdmi_high_tmds_clock_ratio;
>  
> -	/* output format is YCBCR 4:2:0 */
> -	bool ycbcr420;
> +	/* Output format RGB/YCBCR etc */
> +	enum intel_output_format output_format;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 978f22b..6700ed6 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -479,7 +479,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  		return;
>  	}
>  
> -	if (crtc_state->ycbcr420)
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>  		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> @@ -1615,7 +1615,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> -		if (crtc_state->ycbcr420) {
> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>  			const struct drm_hdmi_info *hdmi = &info->hdmi;
>  
>  			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
> @@ -1650,7 +1650,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>  	config->port_clock /= 2;
>  	*clock_12bpc /= 2;
>  	*clock_8bpc /= 2;
> -	config->ycbcr420 = true;
> +	config->output_format = CRTC_OUTPUT_YCBCR420;
>  
>  	/* YCBCR 420 output conversion needs a scaler */
>  	if (skl_update_scaler_crtc(config)) {
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index e702a64..17025bc 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>  	/* Native modes don't need fitting */
>  	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>  	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
> -	    !pipe_config->ycbcr420)
> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>  		goto done;
>  
>  	switch (fitting_mode) {

Sharma, Shashank Jan. 30, 2018, 5:04 p.m. UTC | #2

Regards

Shashank


On 1/30/2018 3:53 PM, Jani Nikula wrote:
> On Tue, 30 Jan 2018, Shashank Sharma <shashank.sharma@intel.com> wrote:
>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>
>> Currently, we are using a bool in CRTC state (state->ycbcr420),
>> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
>> order to support other YCBCR formats, we will need more such flags.
>>
>> The idea behind this patch is to replace this bool with an enum,
>> and plug this in in the existing YCBCR 4:2:0 framework in such a
>> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>>
>> This patch adds a new enum for CRTC output formats, and then plugs it
>> in the existing modeset framework.
>>
>> V3: Added this patch in the series, to address review comments from
>>      second patchset.
>> V4: Added r-b from Maarten (on v3)
>>      Addressed review comments from Ville:
>> 	- Change the enum name to intel_output_format from crtc_output_format.
>> 	- Start the enum value (INVALID) from 0 instaed of 1.
>>          - Set the crtc's output_format to RGB in encoder's compute_config.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   |  2 +-
>>   drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>>   drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
>>   drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
>>   drivers/gpu/drm/i915/intel_panel.c   |  2 +-
>>   6 files changed, 61 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index aa66e95..99e32cb 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>>   	uint16_t coeffs[9] = { 0, };
>>   	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>   
>> -	if (intel_crtc_state->ycbcr420) {
>> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>   		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>>   		return;
>>   	} else if (crtc_state->ctm) {
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index e51559b..d565e28 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>>   	else
>>   		dotclock = pipe_config->port_clock;
>>   
>> -	if (pipe_config->ycbcr420)
>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>>   		dotclock *= 2;
>>   
>>   	if (pipe_config->pixel_multiplier)
>> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>>   	if (port == PORT_A)
>>   		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>>   
>> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
>> +
>>   	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>>   		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>>   	else
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 83de43c..877336d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>   	 */
>>   	need_scaling = src_w != dst_w || src_h != dst_h;
>>   
>> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
>> +	    scaler_user == SKL_CRTC_INDEX)
>>   		need_scaling = true;
>>   
>>   	/*
>> @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
>> +	    pipe_config->base.ctm) {
>>   		/*
>>   		 * There is only one pipe CSC unit per pipe, and we need that
>>   		 * for output conversion from RGB->YCBCR. So if CTM is already
>> @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>>   		if (intel_crtc->config->dither)
>>   			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>   
>> -		if (config->ycbcr420) {
>> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
>> -				PIPEMISC_YUV420_ENABLE |
>> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
>> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> +			val |= PIPEMISC_YUV420_ENABLE |
>> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;
>>   		}
>>   
>>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>>   	}
>>   }
>>   
>> +static const char * const output_format_str[] = {
>> +	"Invalid",
>> +	"RGB",
>> +	"YCBCR4:2:0",
> This should really be:
>
> 	[CRTC_OUTPUT_INVALID] = "Invalid",
> 	[CRTC_OUTPUT_RGB] = "RGB",
> 	[CRTC_OUTPUT_YCBCR420] = "YCBCR4:2:0",
Hey sure, thanks. Just after the comment I realized that now when have 
started the enum from 0 (instead of -1) I can do this.
- Shashank
>
> BR,
> Jani.
>
>> +};
>> +
>> +static const char *output_formats(enum intel_output_format format)
>> +{
>> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>> +		format = CRTC_OUTPUT_INVALID;
>> +	return output_format_str[format];
>> +}
>> +
>>   static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   				    struct intel_crtc_state *pipe_config)
>>   {
>> @@ -9211,19 +9226,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   
>>   	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>   		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> -
>> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
>> -			bool blend_mode_420 = tmp &
>> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
>> +
>> +		if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
>> +			bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
>> +			bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +
>> +			if (ycbcr420_enabled) {
>> +				/* We support 4:2:0 in full blend mode only */
>> +				if (!blend)
>> +					output_format = CRTC_OUTPUT_INVALID;
>> +				else if (!(IS_GEMINILAKE(dev_priv) ||
>> +					   INTEL_GEN(dev_priv) >= 10))
>> +					output_format = CRTC_OUTPUT_INVALID;
>> +				else
>> +					output_format = CRTC_OUTPUT_YCBCR420;
>> +			}
>>   
>> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
>> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
>> -			    pipe_config->ycbcr420 != blend_mode_420)
>> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
>> -		} else if (clrspace_yuv) {
>> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>>   		}
>> +
>> +		pipe_config->output_format = output_format;
>> +		DRM_DEBUG_KMS("Output format %s\n",
>> +				output_formats(output_format));
>>   	}
>>   
>>   	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>> @@ -10578,6 +10602,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>>   		      buf, pipe_config->output_types);
>>   
>> +	DRM_DEBUG_KMS("output format: %s\n",
>> +		output_formats(pipe_config->output_format));
>> +
>>   	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>>   		      transcoder_name(pipe_config->cpu_transcoder),
>>   		      pipe_config->pipe_bpp, pipe_config->dither);
>> @@ -10587,9 +10614,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   				      pipe_config->fdi_lanes,
>>   				      &pipe_config->fdi_m_n);
>>   
>> -	if (pipe_config->ycbcr420)
>> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
>> -
>>   	if (intel_crtc_has_dp_encoder(pipe_config)) {
>>   		intel_dump_m_n_config(pipe_config, "dp m_n",
>>   				pipe_config->lane_count, &pipe_config->dp_m_n);
>> @@ -11163,6 +11187,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>>   
>>   	PIPE_CONF_CHECK_I(pixel_multiplier);
>> +	PIPE_CONF_CHECK_I(output_format);
>>   	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>>   	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>>   	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> @@ -11171,7 +11196,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>   	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>>   	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
>> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
>>   
>>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 3cee54b..35be78e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>>   	bool need_postvbl_update;
>>   };
>>   
>> +enum intel_output_format {
>> +	CRTC_OUTPUT_INVALID,
>> +	CRTC_OUTPUT_RGB,
>> +	CRTC_OUTPUT_YCBCR420,
>> +};
>> +
>>   struct intel_crtc_state {
>>   	struct drm_crtc_state base;
>>   
>> @@ -873,8 +879,8 @@ struct intel_crtc_state {
>>   	/* HDMI High TMDS char rate ratio */
>>   	bool hdmi_high_tmds_clock_ratio;
>>   
>> -	/* output format is YCBCR 4:2:0 */
>> -	bool ycbcr420;
>> +	/* Output format RGB/YCBCR etc */
>> +	enum intel_output_format output_format;
>>   };
>>   
>>   struct intel_crtc {
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 978f22b..6700ed6 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -479,7 +479,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>>   		return;
>>   	}
>>   
>> -	if (crtc_state->ycbcr420)
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>>   		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>   	else
>>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> @@ -1615,7 +1615,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>>   		if (connector_state->crtc != crtc_state->base.crtc)
>>   			continue;
>>   
>> -		if (crtc_state->ycbcr420) {
>> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>   			const struct drm_hdmi_info *hdmi = &info->hdmi;
>>   
>>   			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
>> @@ -1650,7 +1650,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>>   	config->port_clock /= 2;
>>   	*clock_12bpc /= 2;
>>   	*clock_8bpc /= 2;
>> -	config->ycbcr420 = true;
>> +	config->output_format = CRTC_OUTPUT_YCBCR420;
>>   
>>   	/* YCBCR 420 output conversion needs a scaler */
>>   	if (skl_update_scaler_crtc(config)) {
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index e702a64..17025bc 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>>   	/* Native modes don't need fitting */
>>   	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>>   	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
>> -	    !pipe_config->ycbcr420)
>> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>>   		goto done;
>>   
>>   	switch (fitting_mode) {

Ville Syrjala Feb. 1, 2018, 7:09 p.m. UTC | #3

On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> 
> Currently, we are using a bool in CRTC state (state->ycbcr420),
> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
> order to support other YCBCR formats, we will need more such flags.
> 
> The idea behind this patch is to replace this bool with an enum,
> and plug this in in the existing YCBCR 4:2:0 framework in such a
> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
> 
> This patch adds a new enum for CRTC output formats, and then plugs it
> in the existing modeset framework.
> 
> V3: Added this patch in the series, to address review comments from
>     second patchset.
> V4: Added r-b from Maarten (on v3)
>     Addressed review comments from Ville:
> 	- Change the enum name to intel_output_format from crtc_output_format.
> 	- Start the enum value (INVALID) from 0 instaed of 1.
>         - Set the crtc's output_format to RGB in encoder's compute_config.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   |  2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>  drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
>  drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
>  drivers/gpu/drm/i915/intel_panel.c   |  2 +-
>  6 files changed, 61 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index aa66e95..99e32cb 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  	uint16_t coeffs[9] = { 0, };
>  	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>  
> -	if (intel_crtc_state->ycbcr420) {
> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>  		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>  		return;
>  	} else if (crtc_state->ctm) {
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index e51559b..d565e28 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>  	else
>  		dotclock = pipe_config->port_clock;
>  
> -	if (pipe_config->ycbcr420)
> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>  		dotclock *= 2;
>  
>  	if (pipe_config->pixel_multiplier)
> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> +	pipe_config->output_format = CRTC_OUTPUT_RGB;

We seem to be missing this for most platforms/encoder types.

> +
>  	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>  		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>  	else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 83de43c..877336d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
> +	    scaler_user == SKL_CRTC_INDEX)
>  		need_scaling = true;
>  
>  	/*
> @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
> +	    pipe_config->base.ctm) {
>  		/*
>  		 * There is only one pipe CSC unit per pipe, and we need that
>  		 * for output conversion from RGB->YCBCR. So if CTM is already
> @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  		if (intel_crtc->config->dither)
>  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
> -		if (config->ycbcr420) {
> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> -				PIPEMISC_YUV420_ENABLE |
> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +			val |= PIPEMISC_YUV420_ENABLE |
> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;

Why two |= ?

>  		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +static const char * const output_format_str[] = {
> +	"Invalid",
> +	"RGB",
> +	"YCBCR4:2:0",
> +};
> +
> +static const char *output_formats(enum intel_output_format format)
> +{
> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> +		format = CRTC_OUTPUT_INVALID;
> +	return output_format_str[format];
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -9211,19 +9226,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  
>  	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>  		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
> -
> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
> -			bool blend_mode_420 = tmp &
> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
> +		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
> +
> +		if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
> +			bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
> +			bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
> +
> +			if (ycbcr420_enabled) {
> +				/* We support 4:2:0 in full blend mode only */
> +				if (!blend)
> +					output_format = CRTC_OUTPUT_INVALID;
> +				else if (!(IS_GEMINILAKE(dev_priv) ||
> +					   INTEL_GEN(dev_priv) >= 10))
> +					output_format = CRTC_OUTPUT_INVALID;
> +				else
> +					output_format = CRTC_OUTPUT_YCBCR420;
> +			}
>  
> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
> -			    pipe_config->ycbcr420 != blend_mode_420)
> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
> -		} else if (clrspace_yuv) {
> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>  		}
> +
> +		pipe_config->output_format = output_format;
> +		DRM_DEBUG_KMS("Output format %s\n",
> +				output_formats(output_format));

Useless debug print.

>  	}
>  
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> @@ -10578,6 +10602,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>  		      buf, pipe_config->output_types);
>  
> +	DRM_DEBUG_KMS("output format: %s\n",
> +		output_formats(pipe_config->output_format));
> +
>  	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>  		      transcoder_name(pipe_config->cpu_transcoder),
>  		      pipe_config->pipe_bpp, pipe_config->dither);
> @@ -10587,9 +10614,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  				      pipe_config->fdi_lanes,
>  				      &pipe_config->fdi_m_n);
>  
> -	if (pipe_config->ycbcr420)
> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
> -
>  	if (intel_crtc_has_dp_encoder(pipe_config)) {
>  		intel_dump_m_n_config(pipe_config, "dp m_n",
>  				pipe_config->lane_count, &pipe_config->dp_m_n);
> @@ -11163,6 +11187,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>  
>  	PIPE_CONF_CHECK_I(pixel_multiplier);
> +	PIPE_CONF_CHECK_I(output_format);
>  	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> @@ -11171,7 +11196,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>  	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>  	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
>  
>  	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3cee54b..35be78e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>  	bool need_postvbl_update;
>  };
>  
> +enum intel_output_format {
> +	CRTC_OUTPUT_INVALID,
> +	CRTC_OUTPUT_RGB,
> +	CRTC_OUTPUT_YCBCR420,
> +};
> +
>  struct intel_crtc_state {
>  	struct drm_crtc_state base;
>  
> @@ -873,8 +879,8 @@ struct intel_crtc_state {
>  	/* HDMI High TMDS char rate ratio */
>  	bool hdmi_high_tmds_clock_ratio;
>  
> -	/* output format is YCBCR 4:2:0 */
> -	bool ycbcr420;
> +	/* Output format RGB/YCBCR etc */
> +	enum intel_output_format output_format;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 978f22b..6700ed6 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -479,7 +479,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  		return;
>  	}
>  
> -	if (crtc_state->ycbcr420)
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>  		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> @@ -1615,7 +1615,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> -		if (crtc_state->ycbcr420) {
> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>  			const struct drm_hdmi_info *hdmi = &info->hdmi;
>  
>  			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
> @@ -1650,7 +1650,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>  	config->port_clock /= 2;
>  	*clock_12bpc /= 2;
>  	*clock_8bpc /= 2;
> -	config->ycbcr420 = true;
> +	config->output_format = CRTC_OUTPUT_YCBCR420;
>  
>  	/* YCBCR 420 output conversion needs a scaler */
>  	if (skl_update_scaler_crtc(config)) {
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index e702a64..17025bc 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>  	/* Native modes don't need fitting */
>  	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>  	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
> -	    !pipe_config->ycbcr420)
> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>  		goto done;
>  
>  	switch (fitting_mode) {
> -- 
> 2.7.4

Sharma, Shashank Feb. 2, 2018, 6:08 a.m. UTC | #4

Thanks for the comments, mine inline.

Regards
Shashank
On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
> On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>
>> Currently, we are using a bool in CRTC state (state->ycbcr420),
>> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
>> order to support other YCBCR formats, we will need more such flags.
>>
>> The idea behind this patch is to replace this bool with an enum,
>> and plug this in in the existing YCBCR 4:2:0 framework in such a
>> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>>
>> This patch adds a new enum for CRTC output formats, and then plugs it
>> in the existing modeset framework.
>>
>> V3: Added this patch in the series, to address review comments from
>>      second patchset.
>> V4: Added r-b from Maarten (on v3)
>>      Addressed review comments from Ville:
>> 	- Change the enum name to intel_output_format from crtc_output_format.
>> 	- Start the enum value (INVALID) from 0 instaed of 1.
>>          - Set the crtc's output_format to RGB in encoder's compute_config.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   |  2 +-
>>   drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>>   drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
>>   drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
>>   drivers/gpu/drm/i915/intel_panel.c   |  2 +-
>>   6 files changed, 61 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index aa66e95..99e32cb 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>>   	uint16_t coeffs[9] = { 0, };
>>   	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>   
>> -	if (intel_crtc_state->ycbcr420) {
>> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>   		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>>   		return;
>>   	} else if (crtc_state->ctm) {
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index e51559b..d565e28 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>>   	else
>>   		dotclock = pipe_config->port_clock;
>>   
>> -	if (pipe_config->ycbcr420)
>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>>   		dotclock *= 2;
>>   
>>   	if (pipe_config->pixel_multiplier)
>> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>>   	if (port == PORT_A)
>>   		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>>   
>> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
> We seem to be missing this for most platforms/encoder types.
I thought this would be required only for DDI interfaces, do you suggest 
we should set this per encoder basis ?
>
>> +
>>   	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>>   		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>>   	else
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 83de43c..877336d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>   	 */
>>   	need_scaling = src_w != dst_w || src_h != dst_h;
>>   
>> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
>> +	    scaler_user == SKL_CRTC_INDEX)
>>   		need_scaling = true;
>>   
>>   	/*
>> @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
>> +	    pipe_config->base.ctm) {
>>   		/*
>>   		 * There is only one pipe CSC unit per pipe, and we need that
>>   		 * for output conversion from RGB->YCBCR. So if CTM is already
>> @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>>   		if (intel_crtc->config->dither)
>>   			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>   
>> -		if (config->ycbcr420) {
>> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
>> -				PIPEMISC_YUV420_ENABLE |
>> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
>> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> +			val |= PIPEMISC_YUV420_ENABLE |
>> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;
> Why two |= ?
Wanna make it clear that first set is for all YCBCR outputs(420 and 444) 
whereas other two are explicitly for 420.
>>   		}
>>   
>>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>>   	}
>>   }
>>   
>> +static const char * const output_format_str[] = {
>> +	"Invalid",
>> +	"RGB",
>> +	"YCBCR4:2:0",
>> +};
>> +
>> +static const char *output_formats(enum intel_output_format format)
>> +{
>> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>> +		format = CRTC_OUTPUT_INVALID;
>> +	return output_format_str[format];
>> +}
>> +
>>   static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   				    struct intel_crtc_state *pipe_config)
>>   {
>> @@ -9211,19 +9226,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   
>>   	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>   		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> -
>> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
>> -			bool blend_mode_420 = tmp &
>> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
>> +
>> +		if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
>> +			bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
>> +			bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +
>> +			if (ycbcr420_enabled) {
>> +				/* We support 4:2:0 in full blend mode only */
>> +				if (!blend)
>> +					output_format = CRTC_OUTPUT_INVALID;
>> +				else if (!(IS_GEMINILAKE(dev_priv) ||
>> +					   INTEL_GEN(dev_priv) >= 10))
>> +					output_format = CRTC_OUTPUT_INVALID;
>> +				else
>> +					output_format = CRTC_OUTPUT_YCBCR420;
>> +			}
>>   
>> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
>> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
>> -			    pipe_config->ycbcr420 != blend_mode_420)
>> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
>> -		} else if (clrspace_yuv) {
>> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>>   		}
>> +
>> +		pipe_config->output_format = output_format;
>> +		DRM_DEBUG_KMS("Output format %s\n",
>> +				output_formats(output_format));
> Useless debug print.
Its a _kms only, but can be removed if you think so.
- Shashank
>>   	}
>>   
>>   	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>> @@ -10578,6 +10602,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>>   		      buf, pipe_config->output_types);
>>   
>> +	DRM_DEBUG_KMS("output format: %s\n",
>> +		output_formats(pipe_config->output_format));
>> +
>>   	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>>   		      transcoder_name(pipe_config->cpu_transcoder),
>>   		      pipe_config->pipe_bpp, pipe_config->dither);
>> @@ -10587,9 +10614,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   				      pipe_config->fdi_lanes,
>>   				      &pipe_config->fdi_m_n);
>>   
>> -	if (pipe_config->ycbcr420)
>> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
>> -
>>   	if (intel_crtc_has_dp_encoder(pipe_config)) {
>>   		intel_dump_m_n_config(pipe_config, "dp m_n",
>>   				pipe_config->lane_count, &pipe_config->dp_m_n);
>> @@ -11163,6 +11187,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>>   
>>   	PIPE_CONF_CHECK_I(pixel_multiplier);
>> +	PIPE_CONF_CHECK_I(output_format);
>>   	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>>   	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>>   	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> @@ -11171,7 +11196,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>   	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>>   	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
>> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
>>   
>>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 3cee54b..35be78e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>>   	bool need_postvbl_update;
>>   };
>>   
>> +enum intel_output_format {
>> +	CRTC_OUTPUT_INVALID,
>> +	CRTC_OUTPUT_RGB,
>> +	CRTC_OUTPUT_YCBCR420,
>> +};
>> +
>>   struct intel_crtc_state {
>>   	struct drm_crtc_state base;
>>   
>> @@ -873,8 +879,8 @@ struct intel_crtc_state {
>>   	/* HDMI High TMDS char rate ratio */
>>   	bool hdmi_high_tmds_clock_ratio;
>>   
>> -	/* output format is YCBCR 4:2:0 */
>> -	bool ycbcr420;
>> +	/* Output format RGB/YCBCR etc */
>> +	enum intel_output_format output_format;
>>   };
>>   
>>   struct intel_crtc {
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 978f22b..6700ed6 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -479,7 +479,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>>   		return;
>>   	}
>>   
>> -	if (crtc_state->ycbcr420)
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>>   		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>   	else
>>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> @@ -1615,7 +1615,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>>   		if (connector_state->crtc != crtc_state->base.crtc)
>>   			continue;
>>   
>> -		if (crtc_state->ycbcr420) {
>> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>   			const struct drm_hdmi_info *hdmi = &info->hdmi;
>>   
>>   			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
>> @@ -1650,7 +1650,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>>   	config->port_clock /= 2;
>>   	*clock_12bpc /= 2;
>>   	*clock_8bpc /= 2;
>> -	config->ycbcr420 = true;
>> +	config->output_format = CRTC_OUTPUT_YCBCR420;
>>   
>>   	/* YCBCR 420 output conversion needs a scaler */
>>   	if (skl_update_scaler_crtc(config)) {
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index e702a64..17025bc 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>>   	/* Native modes don't need fitting */
>>   	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>>   	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
>> -	    !pipe_config->ycbcr420)
>> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>>   		goto done;
>>   
>>   	switch (fitting_mode) {
>> -- 
>> 2.7.4

Ville Syrjala Feb. 2, 2018, 2 p.m. UTC | #5

On Fri, Feb 02, 2018 at 11:38:07AM +0530, Sharma, Shashank wrote:
> Thanks for the comments, mine inline.
> 
> Regards
> Shashank
> On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
> > On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
> >> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> >>
> >> Currently, we are using a bool in CRTC state (state->ycbcr420),
> >> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
> >> order to support other YCBCR formats, we will need more such flags.
> >>
> >> The idea behind this patch is to replace this bool with an enum,
> >> and plug this in in the existing YCBCR 4:2:0 framework in such a
> >> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
> >>
> >> This patch adds a new enum for CRTC output formats, and then plugs it
> >> in the existing modeset framework.
> >>
> >> V3: Added this patch in the series, to address review comments from
> >>      second patchset.
> >> V4: Added r-b from Maarten (on v3)
> >>      Addressed review comments from Ville:
> >> 	- Change the enum name to intel_output_format from crtc_output_format.
> >> 	- Start the enum value (INVALID) from 0 instaed of 1.
> >>          - Set the crtc's output_format to RGB in encoder's compute_config.
> >>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_color.c   |  2 +-
> >>   drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
> >>   drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
> >>   drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
> >>   drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
> >>   drivers/gpu/drm/i915/intel_panel.c   |  2 +-
> >>   6 files changed, 61 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >> index aa66e95..99e32cb 100644
> >> --- a/drivers/gpu/drm/i915/intel_color.c
> >> +++ b/drivers/gpu/drm/i915/intel_color.c
> >> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
> >>   	uint16_t coeffs[9] = { 0, };
> >>   	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
> >>   
> >> -	if (intel_crtc_state->ycbcr420) {
> >> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
> >>   		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
> >>   		return;
> >>   	} else if (crtc_state->ctm) {
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index e51559b..d565e28 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
> >>   	else
> >>   		dotclock = pipe_config->port_clock;
> >>   
> >> -	if (pipe_config->ycbcr420)
> >> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
> >>   		dotclock *= 2;
> >>   
> >>   	if (pipe_config->pixel_multiplier)
> >> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> >>   	if (port == PORT_A)
> >>   		pipe_config->cpu_transcoder = TRANSCODER_EDP;
> >>   
> >> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
> > We seem to be missing this for most platforms/encoder types.
> I thought this would be required only for DDI interfaces, do you suggest 
> we should set this per encoder basis ?

Yes, I'd like to see every encoder set it. Otherwise our state dumps
will contain invalid information.

> >
> >> +
> >>   	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
> >>   		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
> >>   	else
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 83de43c..877336d 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
> >>   	 */
> >>   	need_scaling = src_w != dst_w || src_h != dst_h;
> >>   
> >> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> >> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
> >> +	    scaler_user == SKL_CRTC_INDEX)
> >>   		need_scaling = true;
> >>   
> >>   	/*
> >> @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
> >> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
> >> +	    pipe_config->base.ctm) {
> >>   		/*
> >>   		 * There is only one pipe CSC unit per pipe, and we need that
> >>   		 * for output conversion from RGB->YCBCR. So if CTM is already
> >> @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
> >>   		if (intel_crtc->config->dither)
> >>   			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
> >>   
> >> -		if (config->ycbcr420) {
> >> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> >> -				PIPEMISC_YUV420_ENABLE |
> >> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> >> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
> >> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> >> +			val |= PIPEMISC_YUV420_ENABLE |
> >> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;
> > Why two |= ?
> Wanna make it clear that first set is for all YCBCR outputs(420 and 444) 
> whereas other two are explicitly for 420.

Fair enough.

> >>   		}
> >>   
> >>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> >> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> >>   	}
> >>   }
> >>   
> >> +static const char * const output_format_str[] = {
> >> +	"Invalid",
> >> +	"RGB",
> >> +	"YCBCR4:2:0",
> >> +};
> >> +
> >> +static const char *output_formats(enum intel_output_format format)
> >> +{
> >> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> >> +		format = CRTC_OUTPUT_INVALID;

BTW >= ARRAY_SIZE(format_str)

would seem like a better check here.

> >> +	return output_format_str[format];
> >> +}
> >> +
> >>   static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >>   				    struct intel_crtc_state *pipe_config)
> >>   {
> >> @@ -9211,19 +9226,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >>   
> >>   	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
> >>   		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> >> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
> >> -
> >> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
> >> -			bool blend_mode_420 = tmp &
> >> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
> >> +		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
> >> +
> >> +		if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
> >> +			bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
> >> +			bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
> >> +
> >> +			if (ycbcr420_enabled) {
> >> +				/* We support 4:2:0 in full blend mode only */
> >> +				if (!blend)
> >> +					output_format = CRTC_OUTPUT_INVALID;
> >> +				else if (!(IS_GEMINILAKE(dev_priv) ||
> >> +					   INTEL_GEN(dev_priv) >= 10))
> >> +					output_format = CRTC_OUTPUT_INVALID;
> >> +				else
> >> +					output_format = CRTC_OUTPUT_YCBCR420;
> >> +			}
> >>   
> >> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
> >> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
> >> -			    pipe_config->ycbcr420 != blend_mode_420)
> >> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
> >> -		} else if (clrspace_yuv) {
> >> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
> >>   		}
> >> +
> >> +		pipe_config->output_format = output_format;
> >> +		DRM_DEBUG_KMS("Output format %s\n",
> >> +				output_formats(output_format));
> > Useless debug print.
> Its a _kms only, but can be removed if you think so.

Yes, we can't have everyone pollute the dmesg with their favorite detail
of the day. That'll lead to the whole thing being mostly noise.

I think the important stuff we want to print is mostly to do with 
error conditions and the modeset sequencing. Detais about the
crtc/plane/etc. states should be covered by the state dumps.

> - Shashank
> >>   	}
> >>   
> >>   	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> >> @@ -10578,6 +10602,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >>   	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
> >>   		      buf, pipe_config->output_types);
> >>   
> >> +	DRM_DEBUG_KMS("output format: %s\n",
> >> +		output_formats(pipe_config->output_format));
> >> +
> >>   	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
> >>   		      transcoder_name(pipe_config->cpu_transcoder),
> >>   		      pipe_config->pipe_bpp, pipe_config->dither);
> >> @@ -10587,9 +10614,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >>   				      pipe_config->fdi_lanes,
> >>   				      &pipe_config->fdi_m_n);
> >>   
> >> -	if (pipe_config->ycbcr420)
> >> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
> >> -
> >>   	if (intel_crtc_has_dp_encoder(pipe_config)) {
> >>   		intel_dump_m_n_config(pipe_config, "dp m_n",
> >>   				pipe_config->lane_count, &pipe_config->dp_m_n);
> >> @@ -11163,6 +11187,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
> >>   
> >>   	PIPE_CONF_CHECK_I(pixel_multiplier);
> >> +	PIPE_CONF_CHECK_I(output_format);
> >>   	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
> >>   	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
> >>   	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >> @@ -11171,7 +11196,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >>   	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
> >>   	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
> >>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
> >> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
> >>   
> >>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
> >>   
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 3cee54b..35be78e 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
> >>   	bool need_postvbl_update;
> >>   };
> >>   
> >> +enum intel_output_format {
> >> +	CRTC_OUTPUT_INVALID,
> >> +	CRTC_OUTPUT_RGB,
> >> +	CRTC_OUTPUT_YCBCR420,
> >> +};
> >> +
> >>   struct intel_crtc_state {
> >>   	struct drm_crtc_state base;
> >>   
> >> @@ -873,8 +879,8 @@ struct intel_crtc_state {
> >>   	/* HDMI High TMDS char rate ratio */
> >>   	bool hdmi_high_tmds_clock_ratio;
> >>   
> >> -	/* output format is YCBCR 4:2:0 */
> >> -	bool ycbcr420;
> >> +	/* Output format RGB/YCBCR etc */
> >> +	enum intel_output_format output_format;
> >>   };
> >>   
> >>   struct intel_crtc {
> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >> index 978f22b..6700ed6 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -479,7 +479,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> >>   		return;
> >>   	}
> >>   
> >> -	if (crtc_state->ycbcr420)
> >> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
> >>   		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> >>   	else
> >>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >> @@ -1615,7 +1615,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
> >>   		if (connector_state->crtc != crtc_state->base.crtc)
> >>   			continue;
> >>   
> >> -		if (crtc_state->ycbcr420) {
> >> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
> >>   			const struct drm_hdmi_info *hdmi = &info->hdmi;
> >>   
> >>   			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
> >> @@ -1650,7 +1650,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
> >>   	config->port_clock /= 2;
> >>   	*clock_12bpc /= 2;
> >>   	*clock_8bpc /= 2;
> >> -	config->ycbcr420 = true;
> >> +	config->output_format = CRTC_OUTPUT_YCBCR420;
> >>   
> >>   	/* YCBCR 420 output conversion needs a scaler */
> >>   	if (skl_update_scaler_crtc(config)) {
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> index e702a64..17025bc 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
> >>   	/* Native modes don't need fitting */
> >>   	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
> >>   	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
> >> -	    !pipe_config->ycbcr420)
> >> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
> >>   		goto done;
> >>   
> >>   	switch (fitting_mode) {
> >> -- 
> >> 2.7.4

Sharma, Shashank Feb. 6, 2018, 9:11 a.m. UTC | #6

Regards

Shashank


On 2/2/2018 7:30 PM, Ville Syrjälä wrote:
> On Fri, Feb 02, 2018 at 11:38:07AM +0530, Sharma, Shashank wrote:
>> Thanks for the comments, mine inline.
>>
>> Regards
>> Shashank
>> On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
>>> On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
>>>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>>>
>>>> Currently, we are using a bool in CRTC state (state->ycbcr420),
>>>> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
>>>> order to support other YCBCR formats, we will need more such flags.
>>>>
>>>> The idea behind this patch is to replace this bool with an enum,
>>>> and plug this in in the existing YCBCR 4:2:0 framework in such a
>>>> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>>>>
>>>> This patch adds a new enum for CRTC output formats, and then plugs it
>>>> in the existing modeset framework.
>>>>
>>>> V3: Added this patch in the series, to address review comments from
>>>>       second patchset.
>>>> V4: Added r-b from Maarten (on v3)
>>>>       Addressed review comments from Ville:
>>>> 	- Change the enum name to intel_output_format from crtc_output_format.
>>>> 	- Start the enum value (INVALID) from 0 instaed of 1.
>>>>           - Set the crtc's output_format to RGB in encoder's compute_config.
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_color.c   |  2 +-
>>>>    drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>>>>    drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>>>>    drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
>>>>    drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
>>>>    drivers/gpu/drm/i915/intel_panel.c   |  2 +-
>>>>    6 files changed, 61 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>> index aa66e95..99e32cb 100644
>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>>>>    	uint16_t coeffs[9] = { 0, };
>>>>    	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>>>    
>>>> -	if (intel_crtc_state->ycbcr420) {
>>>> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>>>    		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>>>>    		return;
>>>>    	} else if (crtc_state->ctm) {
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index e51559b..d565e28 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>>>>    	else
>>>>    		dotclock = pipe_config->port_clock;
>>>>    
>>>> -	if (pipe_config->ycbcr420)
>>>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>>>>    		dotclock *= 2;
>>>>    
>>>>    	if (pipe_config->pixel_multiplier)
>>>> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>>>>    	if (port == PORT_A)
>>>>    		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>>>>    
>>>> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
>>> We seem to be missing this for most platforms/encoder types.
>> I thought this would be required only for DDI interfaces, do you suggest
>> we should set this per encoder basis ?
> Yes, I'd like to see every encoder set it. Otherwise our state dumps
> will contain invalid information.
Got it.
>>>> +
>>>>    	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>>>>    		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>>>>    	else
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 83de43c..877336d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>>>    	 */
>>>>    	need_scaling = src_w != dst_w || src_h != dst_h;
>>>>    
>>>> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>>>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
>>>> +	    scaler_user == SKL_CRTC_INDEX)
>>>>    		need_scaling = true;
>>>>    
>>>>    	/*
>>>> @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>>>    		return -EINVAL;
>>>>    	}
>>>>    
>>>> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
>>>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
>>>> +	    pipe_config->base.ctm) {
>>>>    		/*
>>>>    		 * There is only one pipe CSC unit per pipe, and we need that
>>>>    		 * for output conversion from RGB->YCBCR. So if CTM is already
>>>> @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>>>>    		if (intel_crtc->config->dither)
>>>>    			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>>>    
>>>> -		if (config->ycbcr420) {
>>>> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
>>>> -				PIPEMISC_YUV420_ENABLE |
>>>> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
>>>> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
>>>> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>>>> +			val |= PIPEMISC_YUV420_ENABLE |
>>>> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;
>>> Why two |= ?
>> Wanna make it clear that first set is for all YCBCR outputs(420 and 444)
>> whereas other two are explicitly for 420.
> Fair enough.
>
>>>>    		}
>>>>    
>>>>    		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>>>> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>>>>    	}
>>>>    }
>>>>    
>>>> +static const char * const output_format_str[] = {
>>>> +	"Invalid",
>>>> +	"RGB",
>>>> +	"YCBCR4:2:0",
>>>> +};
>>>> +
>>>> +static const char *output_formats(enum intel_output_format format)
>>>> +{
>>>> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>>>> +		format = CRTC_OUTPUT_INVALID;
> BTW >= ARRAY_SIZE(format_str)
>
> would seem like a better check here.
Yep, agree.
>>>> +	return output_format_str[format];
>>>> +}
>>>> +
>>>>    static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>>    				    struct intel_crtc_state *pipe_config)
>>>>    {
>>>> @@ -9211,19 +9226,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>>    
>>>>    	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>>>    		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>>>> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
>>>> -
>>>> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
>>>> -			bool blend_mode_420 = tmp &
>>>> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
>>>> +		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
>>>> +
>>>> +		if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
>>>> +			bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
>>>> +			bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
>>>> +
>>>> +			if (ycbcr420_enabled) {
>>>> +				/* We support 4:2:0 in full blend mode only */
>>>> +				if (!blend)
>>>> +					output_format = CRTC_OUTPUT_INVALID;
>>>> +				else if (!(IS_GEMINILAKE(dev_priv) ||
>>>> +					   INTEL_GEN(dev_priv) >= 10))
>>>> +					output_format = CRTC_OUTPUT_INVALID;
>>>> +				else
>>>> +					output_format = CRTC_OUTPUT_YCBCR420;
>>>> +			}
>>>>    
>>>> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
>>>> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
>>>> -			    pipe_config->ycbcr420 != blend_mode_420)
>>>> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
>>>> -		} else if (clrspace_yuv) {
>>>> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>>>>    		}
>>>> +
>>>> +		pipe_config->output_format = output_format;
>>>> +		DRM_DEBUG_KMS("Output format %s\n",
>>>> +				output_formats(output_format));
>>> Useless debug print.
>> Its a _kms only, but can be removed if you think so.
> Yes, we can't have everyone pollute the dmesg with their favorite detail
> of the day. That'll lead to the whole thing being mostly noise.
>
> I think the important stuff we want to print is mostly to do with
> error conditions and the modeset sequencing. Detais about the
> crtc/plane/etc. states should be covered by the state dumps.
Ok, it will be gone.
- Shashank
>> - Shashank
>>>>    	}
>>>>    
>>>>    	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>>>> @@ -10578,6 +10602,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>>>    	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>>>>    		      buf, pipe_config->output_types);
>>>>    
>>>> +	DRM_DEBUG_KMS("output format: %s\n",
>>>> +		output_formats(pipe_config->output_format));
>>>> +
>>>>    	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>>>>    		      transcoder_name(pipe_config->cpu_transcoder),
>>>>    		      pipe_config->pipe_bpp, pipe_config->dither);
>>>> @@ -10587,9 +10614,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>>>    				      pipe_config->fdi_lanes,
>>>>    				      &pipe_config->fdi_m_n);
>>>>    
>>>> -	if (pipe_config->ycbcr420)
>>>> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
>>>> -
>>>>    	if (intel_crtc_has_dp_encoder(pipe_config)) {
>>>>    		intel_dump_m_n_config(pipe_config, "dp m_n",
>>>>    				pipe_config->lane_count, &pipe_config->dp_m_n);
>>>> @@ -11163,6 +11187,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>>>    	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>>>>    
>>>>    	PIPE_CONF_CHECK_I(pixel_multiplier);
>>>> +	PIPE_CONF_CHECK_I(output_format);
>>>>    	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>>>>    	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>>>>    	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>>> @@ -11171,7 +11196,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>>>    	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>>>>    	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>>>>    	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
>>>> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
>>>>    
>>>>    	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>>>>    
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 3cee54b..35be78e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>>>>    	bool need_postvbl_update;
>>>>    };
>>>>    
>>>> +enum intel_output_format {
>>>> +	CRTC_OUTPUT_INVALID,
>>>> +	CRTC_OUTPUT_RGB,
>>>> +	CRTC_OUTPUT_YCBCR420,
>>>> +};
>>>> +
>>>>    struct intel_crtc_state {
>>>>    	struct drm_crtc_state base;
>>>>    
>>>> @@ -873,8 +879,8 @@ struct intel_crtc_state {
>>>>    	/* HDMI High TMDS char rate ratio */
>>>>    	bool hdmi_high_tmds_clock_ratio;
>>>>    
>>>> -	/* output format is YCBCR 4:2:0 */
>>>> -	bool ycbcr420;
>>>> +	/* Output format RGB/YCBCR etc */
>>>> +	enum intel_output_format output_format;
>>>>    };
>>>>    
>>>>    struct intel_crtc {
>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> index 978f22b..6700ed6 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -479,7 +479,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>>>>    		return;
>>>>    	}
>>>>    
>>>> -	if (crtc_state->ycbcr420)
>>>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>>>>    		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>>    	else
>>>>    		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>> @@ -1615,7 +1615,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>>>>    		if (connector_state->crtc != crtc_state->base.crtc)
>>>>    			continue;
>>>>    
>>>> -		if (crtc_state->ycbcr420) {
>>>> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>>>    			const struct drm_hdmi_info *hdmi = &info->hdmi;
>>>>    
>>>>    			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
>>>> @@ -1650,7 +1650,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>>>>    	config->port_clock /= 2;
>>>>    	*clock_12bpc /= 2;
>>>>    	*clock_8bpc /= 2;
>>>> -	config->ycbcr420 = true;
>>>> +	config->output_format = CRTC_OUTPUT_YCBCR420;
>>>>    
>>>>    	/* YCBCR 420 output conversion needs a scaler */
>>>>    	if (skl_update_scaler_crtc(config)) {
>>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>>>> index e702a64..17025bc 100644
>>>> --- a/drivers/gpu/drm/i915/intel_panel.c
>>>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>>>> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>>>>    	/* Native modes don't need fitting */
>>>>    	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>>>>    	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
>>>> -	    !pipe_config->ycbcr420)
>>>> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>>>>    		goto done;
>>>>    
>>>>    	switch (fitting_mode) {
>>>> -- 
>>>> 2.7.4

[v4,1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

Commit Message

Comments

Patch