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

Fri Apr 26 18:53:54 UTC 2019

On Fri, Apr 26, 2019 at 09:41:06PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 26, 2019 at 11:22:02AM -0700, Aditya Swarup wrote:
> > 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. 
> 
> We don't have to send it for 24bpp even if the old hw supports doing so.
> 
> Well, the HDMI spec does say
> "Once a Source sends a GCP with non-zero CD to a sink, it should
>  continue sending GCPs with non-zero CD at least once per video
>  field even if reverting to 24-bit color, as long as the Sink
>  continues to support Deep Color."
> 
> so I guess we should maybe send it? But maybe that only applies if
> we would swich the color deph without a full modeset (can't really
> see how that would work in the first place considering the symbol
> clock needs to be adjusted too).
> 
> Anyways, I think it's better to just stick to a uniform behaviour
> across all platforms here.

I don't have any problems with that but according to

Bspec: 7854 

"This bit must be set when in deep color mode. It may optionally be
set for 24-bit mode. It must be set if the sink attached to the 
transcoder can receive GCP data."

The only thing that concerns me is that the spec says it "must" be set
if transcoder can receive GCP data.

If you still feel that it is not necessary, then I will remove the
platform checks.

> 
> > > 
> > > > +			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
> 
> -- 
> Ville Syrjälä
> Intel