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

Fri Apr 26 19:06:54 UTC 2019

On Fri, Apr 26, 2019 at 11:53:54AM -0700, Aditya Swarup wrote:
> 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.

That note is contradicting itself so clearly it can't be trusted.

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

We can't set it on hsw+ anyway, so someone definitely came to
the conclusion that it's not needed.

> 
> > 
> > > > 
> > > > > +			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

-- 
Ville Syrjälä
Intel