[Intel-gfx] [PATCH v3 3/8] drm/i915/display: Add new member to configure PCON color conversion
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Fri Oct 28 06:16:46 UTC 2022
Hi Ville,
Thanks for the reviews and suggestions.
I agree with the suggested changes and will be incorporating them in the
next version of the series.
I have prepared the changes (without the suggested readout part though)
still need to test on a panel with ycbcr420.
Please find my responses inline:
On 10/20/2022 10:21 PM, Ville Syrjälä wrote:
> 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 at 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.
Makes sense. Will do as suggested.
>> };
>>
>> 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().
Hmm.. well it does seem backwards.
We do want to set the sink format as YCBCR420/RGB first, and then set
output_format.
Will do compute sink_format first based on the constraints and then use
it to compute 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.
Agreed.
>
> 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).
Hmm so do we need to have new callbacks for read out in dig_port like
read_sink_format(encoder, crtc_state)
So for intel_dp with branch device we read from PCON color conversion
DPCDs. For others we just return crtc_state->output_format?
Or we just during readout we set sink_format as output_format for all
encoders,
but for dp we call a function to check what is programmed in PCON color
conversion DPCDs?
Thanks & Regards,
Ankit
More information about the Intel-gfx
mailing list