On Wed, 24 Aug 2022, "Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx> wrote:
> On 8/23/2022 4:33 PM, Jani Nikula wrote:
>> On Mon, 22 Aug 2022, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> wrote:
>>> The decision to use DFP output format conversion capabilities should be
>>> during compute_config phase.
>>>
>>> This patch:
>>> -uses the members of intel_dp->dfp to only store the
>>> format conversion capabilities of the DP device.
>>> -adds new members to crtc_state to help configure the DFP
>>> output related conversions.
>>> -pulls the decision making to use DFP conversion capabilities
>>> for every mode during compute config.
>> The fact that you have a list here probably indicates it's doing too
>> much at once.
>>
>> BR,
>> Jani.
>
> Alright, perhaps adding new members as a separate patch and using them
> in another patch will be better.
>
> Will split this into smaller patches.
You are also changing function parameters, rearranging stuff to new
functions, whitespace changes, functional logic changes, all together.
The point is, if an existing use case regressed and a user bisect
pointed at this commit, would you be able to say what went wrong?
BR,
Jani.
>
> Thanks & Regards,
>
> Ankit
>
>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>>> ---
>>> .../drm/i915/display/intel_display_types.h | 7 ++
>>> drivers/gpu/drm/i915/display/intel_dp.c | 88 +++++++++++--------
>>> 2 files changed, 59 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> index 0da9b208d56e..065ed19a5dd3 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> @@ -1311,6 +1311,12 @@ struct intel_crtc_state {
>>>
>>> /* for loading single buffered registers during vblank */
>>> struct drm_vblank_work vblank_work;
>>> +
>>> + /* DP DFP color configuration */
>>> + struct {
>>> + bool rgb_to_ycbcr;
>>> + bool ycbcr_444_to_420;
>>> + } dp_dfp_config;
>>> };
>>>
>>> enum intel_pipe_crc_source {
>>> @@ -1704,6 +1710,7 @@ struct intel_dp {
>>> int pcon_max_frl_bw;
>>> u8 max_bpc;
>>> bool ycbcr_444_to_420;
>>> + bool ycbcr420_passthrough;
>>> bool rgb_to_ycbcr;
>>> } dfp;
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index fc082a933d59..8ccbe591b9e2 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -1201,19 +1201,21 @@ static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
>>> drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd);
>>> }
>>>
>>> -static bool intel_dp_is_ycbcr420(struct intel_dp *intel_dp,
>>> - const struct intel_crtc_state *crtc_state)
>>> +static bool intel_dp_is_ycbcr420(const struct intel_crtc_state *crtc_state)
>>> {
>>> return crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>>> (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 &&
>>> - intel_dp->dfp.ycbcr_444_to_420);
>>> + crtc_state->dp_dfp_config.ycbcr_444_to_420) ||
>>> + (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB &&
>>> + crtc_state->dp_dfp_config.ycbcr_444_to_420 &&
>>> + crtc_state->dp_dfp_config.rgb_to_ycbcr);
>>> }
>>>
>>> static int intel_dp_hdmi_compute_bpc(struct intel_dp *intel_dp,
>>> const struct intel_crtc_state *crtc_state,
>>> int bpc, bool respect_downstream_limits)
>>> {
>>> - bool ycbcr420_output = intel_dp_is_ycbcr420(intel_dp, crtc_state);
>>> + bool ycbcr420_output = intel_dp_is_ycbcr420(crtc_state);
>>> int clock = crtc_state->hw.adjusted_mode.crtc_clock;
>>>
>>> /*
>>> @@ -1966,6 +1968,30 @@ static bool intel_dp_has_audio(struct intel_encoder *encoder,
>>> return intel_conn_state->force_audio == HDMI_AUDIO_ON;
>>> }
>>>
>>> +static void
>>> +intel_dp_compute_dfp_ycbcr420(struct intel_encoder *encoder,
>>> + struct intel_crtc_state *crtc_state)
>>> +{
>>> + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>> +
>>> + if (!drm_dp_is_branch(intel_dp->dpcd))
>>> + return;
>>> +
>>> + /* Mode is YCBCR420, output_format is also YCBCR420: Passthrough */
>>> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
>>> + return;
>>> +
>>> + /* Mode is YCBCR420, output_format is YCBCR444: Downsample */
>>> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
>>> + crtc_state->dp_dfp_config.ycbcr_444_to_420 = true;
>>> + return;
>>> + }
>>> +
>>> + /* Mode is YCBCR420, output_format is RGB: Convert to YCBCR444 and Downsample */
>>> + crtc_state->dp_dfp_config.rgb_to_ycbcr = true;
>>> + crtc_state->dp_dfp_config.ycbcr_444_to_420 = true;
>>> +}
>>> +
>>> static int
>>> intel_dp_compute_output_format(struct intel_encoder *encoder,
>>> struct intel_crtc_state *crtc_state,
>>> @@ -1984,7 +2010,10 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
>>>
>>> crtc_state->output_format = intel_dp_output_format(connector, ycbcr_420_only);
>>>
>>> - if (ycbcr_420_only && !intel_dp_is_ycbcr420(intel_dp, crtc_state)) {
>>> + if (ycbcr_420_only)
>>> + intel_dp_compute_dfp_ycbcr420(encoder, crtc_state);
>>> +
>>> + if (ycbcr_420_only && !intel_dp_is_ycbcr420(crtc_state)) {
>>> 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;
>>> @@ -1993,12 +2022,13 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
>>> ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
>>> respect_downstream_limits);
>>> if (ret) {
>>> - if (intel_dp_is_ycbcr420(intel_dp, crtc_state) ||
>>> + if (intel_dp_is_ycbcr420(crtc_state) ||
>>> !connector->base.ycbcr_420_allowed ||
>>> !drm_mode_is_420_also(info, adjusted_mode))
>>> return ret;
>>>
>>> crtc_state->output_format = intel_dp_output_format(connector, true);
>>> + intel_dp_compute_dfp_ycbcr420(encoder, crtc_state);
>>> ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
>>> respect_downstream_limits);
>>> }
>>> @@ -2668,8 +2698,7 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp,
>>> drm_dbg_kms(&i915->drm, "Failed to %s protocol converter HDMI mode\n",
>>> str_enable_disable(intel_dp->has_hdmi_sink));
>>>
>>> - tmp = crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 &&
>>> - intel_dp->dfp.ycbcr_444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0;
>>> + tmp = crtc_state->dp_dfp_config.ycbcr_444_to_420 ? DP_CONVERSION_TO_YCBCR420_ENABLE : 0;
>>>
>>> if (drm_dp_dpcd_writeb(&intel_dp->aux,
>>> DP_PROTOCOL_CONVERTER_CONTROL_1, tmp) != 1)
>>> @@ -2677,7 +2706,7 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp,
>>> "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n",
>>> str_enable_disable(intel_dp->dfp.ycbcr_444_to_420));
>>>
>>> - tmp = intel_dp->dfp.rgb_to_ycbcr ?
>>> + tmp = crtc_state->dp_dfp_config.rgb_to_ycbcr ?
>>> DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0;
>>>
>>> if (drm_dp_pcon_convert_rgb_to_ycbcr(&intel_dp->aux, tmp) < 0)
>>> @@ -2686,7 +2715,6 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp,
>>> str_enable_disable(tmp));
>>> }
>>>
>>> -
>>> bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
>>> {
>>> u8 dprx = 0;
>>> @@ -4534,7 +4562,6 @@ intel_dp_update_420(struct intel_dp *intel_dp)
>>> {
>>> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> struct intel_connector *connector = intel_dp->attached_connector;
>>> - bool is_branch, ycbcr_420_passthrough, ycbcr_444_to_420, rgb_to_ycbcr;
>>>
>>> /* No YCbCr output support on gmch platforms */
>>> if (HAS_GMCH(i915))
>>> @@ -4547,39 +4574,28 @@ intel_dp_update_420(struct intel_dp *intel_dp)
>>> if (IS_IRONLAKE(i915))
>>> return;
>>>
>>> - is_branch = drm_dp_is_branch(intel_dp->dpcd);
>>> - ycbcr_420_passthrough =
>>> + if (!drm_dp_is_branch(intel_dp->dpcd)) {
>>> + connector->base.ycbcr_420_allowed = true;
>>> + return;
>>> + }
>>> +
>>> + intel_dp->dfp.ycbcr420_passthrough =
>>> drm_dp_downstream_420_passthrough(intel_dp->dpcd,
>>> intel_dp->downstream_ports);
>>> +
>>> /* on-board LSPCON always assumed to support 4:4:4->4:2:0 conversion */
>>> - ycbcr_444_to_420 =
>>> + intel_dp->dfp.ycbcr_444_to_420 =
>>> dp_to_dig_port(intel_dp)->lspcon.active ||
>>> drm_dp_downstream_444_to_420_conversion(intel_dp->dpcd,
>>> intel_dp->downstream_ports);
>>> - rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd,
>>> - intel_dp->downstream_ports,
>>> - DP_DS_HDMI_BT709_RGB_YCBCR_CONV);
>>> -
>>> - if (DISPLAY_VER(i915) >= 11) {
>>> - /* Let PCON convert from RGB->YCbCr if possible */
>>> - if (is_branch && rgb_to_ycbcr && ycbcr_444_to_420) {
>>> - intel_dp->dfp.rgb_to_ycbcr = true;
>>> - intel_dp->dfp.ycbcr_444_to_420 = true;
>>> - connector->base.ycbcr_420_allowed = true;
>>> - } else {
>>> - /* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */
>>> - intel_dp->dfp.ycbcr_444_to_420 =
>>> - ycbcr_444_to_420 && !ycbcr_420_passthrough;
>>>
>>> - connector->base.ycbcr_420_allowed =
>>> - !is_branch || ycbcr_444_to_420 || ycbcr_420_passthrough;
>>> - }
>>> - } else {
>>> - /* 4:4:4->4:2:0 conversion is the only way */
>>> - intel_dp->dfp.ycbcr_444_to_420 = ycbcr_444_to_420;
>>> + intel_dp->dfp.rgb_to_ycbcr =
>>> + drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd,
>>> + intel_dp->downstream_ports,
>>> + DP_DS_HDMI_BT709_RGB_YCBCR_CONV);
>>>
>>> - connector->base.ycbcr_420_allowed = ycbcr_444_to_420;
>>> - }
>>> + if (intel_dp->dfp.ycbcr420_passthrough || intel_dp->dfp.ycbcr_444_to_420)
>>> + connector->base.ycbcr_420_allowed = true;
>>>
>>> drm_dbg_kms(&i915->drm,
>>> "[CONNECTOR:%d:%s] RGB->YcbCr conversion? %s, YCbCr 4:2:0 allowed? %s, YCbCr 4:4:4->4:2:0 conversion? %s\n",
--
Jani Nikula, Intel Open Source Graphics Center
