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

[Aditya Swarup]

On Fri, Apr 26, 2019 at 01:12:58PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 25, 2019 at 01:44:37PM -0700, Aditya Swarup wrote:
> > On Wed, Apr 17, 2019 at 12:57:44PM +0300, Jani Nikula wrote:
> > > On Fri, 05 Apr 2019, Aditya Swarup <aditya.swarup at intel.com> 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
> > > >
> > > > 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 | 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.
> > 
> > You are correct, Clint and me spent time investigating this Spec/HW
> > monstrosity for the correct conditions required for sending
> > GCP_COLOR_INDICATION.
> > 
> > > 
> > > 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?
> > No we do need the check for sink, as that is the real check for
> > determining whether sink supports deep color or not. For some platforms,
> > we do send GCP_COLOR_INDICATION even for 8 bpc when the sink supports
> > deep color. This will be clear with the next version of the patch.
> 
> I think Jani is right. pipe_bpp will not be > 24 unless the sink support
> deep color.
I have described why we need the check for sink on the next revision of
the patch.
> 
> -- 
> Ville Syrjälä
> Intel