Re: [Intel-gfx] [PATCH v3 3/8] drm/i915/display: Add new member to configure PCON color conversion

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 11, 2022 at 12:04:42PM +0530, Ankit Nautiyal wrote:
> The decision to use DFP output format conversion capabilities should be
> during compute_config phase.
> 
> This patch adds new member to crtc_state to represent the final
> output_format to the sink. In case of a DFP this can be different than
> the output_format, as per the format conversion done via the PCON.
> 
> This will help to store only the format conversion capabilities of the
> DP device in intel_dp->dfp, and use crtc_state to compute and store the
> configuration for color/format conversion for a given mode.
> 
> v2: modified the new member to crtc_state to represent the final
> output_format that eaches the sink, after possible conversion by
> PCON kind of devices. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c               | 1 +
>  drivers/gpu/drm/i915/display/intel_crt.c             | 1 +
>  drivers/gpu/drm/i915/display/intel_crtc_state_dump.c | 5 +++--
>  drivers/gpu/drm/i915/display/intel_display_types.h   | 3 +++
>  drivers/gpu/drm/i915/display/intel_dp.c              | 7 +++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c          | 1 +
>  drivers/gpu/drm/i915/display/intel_dvo.c             | 1 +
>  drivers/gpu/drm/i915/display/intel_hdmi.c            | 3 +++
>  drivers/gpu/drm/i915/display/intel_lvds.c            | 1 +
>  drivers/gpu/drm/i915/display/intel_tv.c              | 1 +
>  drivers/gpu/drm/i915/display/vlv_dsi.c               | 1 +
>  11 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 47f13750f6fa..5defafb6b9df 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -1667,6 +1667,7 @@ static int gen11_dsi_compute_config(struct intel_encoder *encoder,
>  	int ret;
>  
>  	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> +	pipe_config->sink_format = pipe_config->output_format;
>  
>  	ret = intel_panel_compute_config(intel_connector, adjusted_mode);
>  	if (ret)
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> index 94d0a5e1dd03..a6e7cf21e6e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -392,6 +392,7 @@ static int intel_crt_compute_config(struct intel_encoder *encoder,
>  		return -EINVAL;
>  
>  	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> +	pipe_config->sink_format = pipe_config->output_format;
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index e9212f69c360..ed427b9cbf09 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -163,10 +163,11 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
>  
>  	snprintf_output_types(buf, sizeof(buf), pipe_config->output_types);
>  	drm_dbg_kms(&i915->drm,
> -		    "active: %s, output_types: %s (0x%x), output format: %s\n",
> +		    "active: %s, output_types: %s (0x%x), output format: %s, sink format: %s\n",
>  		    str_yes_no(pipe_config->hw.active),
>  		    buf, pipe_config->output_types,
> -		    output_formats(pipe_config->output_format));
> +		    output_formats(pipe_config->output_format),
> +		    output_formats(pipe_config->sink_format));
>  
>  	drm_dbg_kms(&i915->drm,
>  		    "cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e2b853e9e51d..69a68a70ac00 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1312,6 +1312,9 @@ struct intel_crtc_state {
>  
>  	/* for loading single buffered registers during vblank */
>  	struct drm_vblank_work vblank_work;
> +
> +	/* Sink output format */
> +	enum intel_output_format sink_format;

Please put this next to the output_format. Probably should try to
clarify the comments for each to indicate output_format is what's
coming out the end of the pipe, and sink_format what's going into
the sink.

>  };
>  
>  enum intel_pipe_crc_source {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 359884617fdc..99d72b345907 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1990,8 +1990,14 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
>  		drm_dbg_kms(&i915->drm,
>  			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
>  		crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;
> +		crtc_state->sink_format = crtc_state->output_format;
>  	}
>  
> +	else if (ycbcr_420_only)
> +		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> +	else
> +		crtc_state->sink_format = crtc_state->output_format;

Hmm. This feels a bit backwards. I think it would make more sense to
start with the sink format and then compute the output_format based
on that. Could add intel_dp_sink_format() helper, and then pass the
result from that into intel_dp_output_format().

> +
>  	ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
>  					   respect_downstream_limits);
>  	if (ret) {
> @@ -2001,6 +2007,7 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
>  			return ret;
>  
>  		crtc_state->output_format = intel_dp_output_format(connector, true);
> +		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
>  		ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
>  						   respect_downstream_limits);
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index cd4e61026d98..cd625c7b8693 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -148,6 +148,7 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  		return -EINVAL;
>  
>  	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> +	pipe_config->sink_format = pipe_config->output_format;

If we compute sink_format first for DP, I'd do the same for all these
other cases too. I think just setting both to RGB explicity would be 
fine.


We seem to missing the readout part entirely. While we can't hook it
into the state checker (due to reliance on the protocol connverters)
it would at least give us slightly more accurate state dump for the
initial state readout in case the GOP has chosen to use native
YCbCr 4:2:0 output (not sure it ever does that though).

-- 
Ville Syrjälä
Intel