[Intel-gfx] [PATCH v8] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color

[Aditya Swarup]

On Fri, Apr 26, 2019 at 01:14:44PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 25, 2019 at 06:19:50PM -0700, Aditya Swarup wrote:
> > From: Clinton Taylor <Clinton.A.Taylor at 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
> > 
> > v8: Set GCP_COLOR_INDICATION based on spec:
> > For Gen 7.5 or later platforms, indicate color depth only for deep
> > color modes. Bspec: 8135,7751,50524
> > 
> > Pre DDI platforms, indicate color depth if deep color is supported
> > by sink. Bspec: 7854
> > 
> > Exception: CHERRYVIEW behaves like Pre DDI platforms.
> > Bspec: 15975
> > 
> > Check pipe_bpp is less than bpp * 3 in hdmi_deep_color_possible,
> > to not set 12 bit deep color for every modeset. This fixes the issue
> > where 12 bit color was selected even when user selected 10 bit.(Ville)
> > 
> > Co-Developed-by: Aditya Swarup <aditya.swarup at intel.com>
> > Co-Developed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Clinton Taylor <Clinton.A.Taylor at intel.com>
> > Signed-off-by: Aditya Swarup <aditya.swarup at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index e1005d7b75fd..620bc89e2120 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -973,9 +973,18 @@ 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))
> > -		crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION;
> > +	/* Indicate color depth whenever the sink supports deep color:
> > +	 * For Gen 7.5 or later platforms, indicate color depth only for deep
> > +	 * color modes.
> > +	 * Pre DDI platforms, indicate color depth if deep color is supported
> > +	 * by sink.
> > +	 * Exception: CHERRYVIEW behaves like Pre DDI platforms.
> > +	 */
> > +	if (hdmi_sink_is_deep_color(conn_state)) {
> > +		if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) ||
> > +		   crtc_state->pipe_bpp > 24)
> 
> I prefer the earlier version. Less special casing.
Then we won't be following spec for pre DDI platforms and CHV. These
conditions are required according to Bspec pages mentioned in the commit
message.

Also, hdmi_sink_is_deep_color check is required for pre DDI platforms,
since we send GCP_COLOR_INDICATION for 24 bit color as long as display
supports deep color. 
> 
> > +			crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION;
> > +	}
> >  
> >  	/* Enable default_phase whenever the display mode is suitably aligned */
> >  	if (gcp_default_phase_possible(crtc_state->pipe_bpp,
> > @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state,
> >  	if (bpc == 10 && INTEL_GEN(dev_priv) < 11)
> >  		return false;
> >  
> > -	if (crtc_state->pipe_bpp <= 8*3)
> > +	if (crtc_state->pipe_bpp < bpc*3)
> 
> This should be a separate patch.
I will create a new patch for this.
> 
> >  		return false;
> >  
> >  	if (!crtc_state->has_hdmi_sink)
> > -- 
> > 2.17.1
> 
> -- 
> Ville Syrjälä
> Intel