| Message ID | 20221011063447.904649-4-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handle BPC for HDMI2.1 PCON without DSC1.2 sink and other fixes | expand |
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).
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@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
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@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 >
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; }; 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; + 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; pipe_config->has_pch_encoder = false; if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO) diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c index 595087288922..070cc7292e6b 100644 --- a/drivers/gpu/drm/i915/display/intel_dvo.c +++ b/drivers/gpu/drm/i915/display/intel_dvo.c @@ -279,6 +279,7 @@ static int intel_dvo_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_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 93519fb23d9d..6a3e9a971513 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2219,6 +2219,8 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder, crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB; } + crtc_state->sink_format = crtc_state->output_format; + ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits); if (ret) { if (intel_hdmi_is_ycbcr420(crtc_state) || @@ -2227,6 +2229,7 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder, return ret; crtc_state->output_format = intel_hdmi_output_format(connector, true); + crtc_state->sink_format = crtc_state->output_format; ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits); } diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c index e97e24f690a9..d1f9b1a46e4a 100644 --- a/drivers/gpu/drm/i915/display/intel_lvds.c +++ b/drivers/gpu/drm/i915/display/intel_lvds.c @@ -440,6 +440,7 @@ static int intel_lvds_compute_config(struct intel_encoder *intel_encoder, } pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; + pipe_config->sink_format = pipe_config->output_format; /* * We have timings from the BIOS for the panel, put them in diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c index b2e93c2ad8f3..97c3d9234a66 100644 --- a/drivers/gpu/drm/i915/display/intel_tv.c +++ b/drivers/gpu/drm/i915/display/intel_tv.c @@ -1205,6 +1205,7 @@ intel_tv_compute_config(struct intel_encoder *encoder, return -EINVAL; pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; + pipe_config->sink_format = pipe_config->output_format; drm_dbg_kms(&dev_priv->drm, "forcing bpc to 8 for TV\n"); pipe_config->pipe_bpp = 8*3; diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c index dee0147a316c..c22d94861502 100644 --- a/drivers/gpu/drm/i915/display/vlv_dsi.c +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c @@ -280,6 +280,7 @@ static int intel_dsi_compute_config(struct intel_encoder *encoder, drm_dbg_kms(&dev_priv->drm, "\n"); 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)
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(-)