[Intel-gfx] [PATCH v3 3/8] drm/i915/display: Add new member to configure PCON color conversion
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Oct 28 06:28:41 UTC 2022
On Fri, Oct 28, 2022 at 11:46:46AM +0530, Nautiyal, Ankit K wrote:
> 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,
Yeah, I was thinking just assigning 'sink_format = output_format'
in the .get_pipe_config() hooks. It'll at least cover the native
YCbCr 4:2:0 output case. And in all cases where the pipe is outputting
some kind of YCbCr we at least report that the sink also gets YCbCr
rather than RGB, which wouldn't make any sense.
Dunno if it makes sense to anything more than that. We already have
lots of noise in CI due to LSPCON readout failures...
>
> but for dp we call a function to check what is programmed in PCON color
> conversion DPCDs?
>
> Thanks & Regards,
>
> Ankit
>
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list