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

Thu, 27 Oct 2022 23:17:13 -0700 

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.nauti...@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

Reply via email to