Re: [PATCH v7] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 05 Apr 2019, Aditya Swarup <aditya.swarup@intel.com> wrote:
> From: Clinton Taylor <Clinton.A.Taylor@intel.com>
>
> v2: Fix commit msg to reflect why issue occurs(Jani)
> Set GCP_COLOR_INDICATION only when we set 10/12 bit deep color.
>
> Changing settings from 10/12 bit deep color to 8 bit(& vice versa)
> doesn't work correctly using xrandr max bpc property. When we
> connect a monitor which supports deep color, the highest deep color
> setting is selected; which sets GCP_COLOR_INDICATION. When we change
> the setting to 8 bit color, we still set GCP_COLOR_INDICATION which
> doesn't allow the switch back to 8 bit color.
>
> v3,4: Add comments & drop changes in intel_hdmi_compute_config(Ville)
> Since HSW+, GCP_COLOR_INDICATION is not required for 8bpc.
>
> Drop the changes in intel_hdmi_compute_config as desired_bpp
> is needed to change values for pipe_bpp based on bw_constrained flag.
>
> v5: Fix missing logical && in condition for setting GCP_COLOR_INDICATION.
>
> v6: Fix comment formatting (Ville)
>
> v7: Add reviewed by Ville
>
> Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com>
> Signed-off-by: Aditya Swarup <aditya.swarup@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 5ccb305a6e1c..f2c0aba4371b 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -962,8 +962,11 @@ static void intel_hdmi_compute_gcp_infoframe(struct intel_encoder *encoder,
>  	crtc_state->infoframes.enable |=
>  		intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GENERAL_CONTROL);
>  
> -	/* Indicate color depth whenever the sink supports deep color */
> -	if (hdmi_sink_is_deep_color(conn_state))
> +	/* Indicate color depth whenever the sink supports deep color
> +	 * Also, 8bpc + color depth indication is no longer supported
> +	 * for HSW+ platforms.
> +	 */

Frankly the comment confuses me as the condition has nothing to do with
HSW+ and applies for pre-HSW as well. And the "whenever" in the first
line is no longer true.

I do understand the point here, we don't need to use color indication
when we're not using deep color anyway, and moreover this combo isn't
supported on HSW+.

The final question is, under what circumstances would we use pipe_bpp >
24 when the sink does *not* support bpc > 8?

IOW, could we simply use

	if (crtc_state->pipe_bpp > 24)

here?


BR,
Jani.


> +	if (hdmi_sink_is_deep_color(conn_state) && crtc_state->pipe_bpp > 24)
>  		crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION;
>  
>  	/* Enable default_phase whenever the display mode is suitably aligned */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx