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

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

On Tue, 07 May 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, May 07, 2019 at 09:42:48AM +0300, Jani Nikula wrote:
>> On Mon, 29 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
>> >
>> > 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)
>> >
>> > v9: Maintain a consistent behavior for all platforms and support
>> > GCP_COLOR_INDICATION only when we are in deep color mode. Remove
>> > hdmi_sink_is_deep_color() - no longer needed as checking pipe_bpp > 24
>> > takes care of the deep color mode scenario.
>> >
>> > Separate patch for fixing switch from 12 bit to 10 bit deep color
>> > mode.
>> >
>> > Co-developed-by: Aditya Swarup <aditya.swarup@intel.com>
>> > 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>
>> 
>> Ville, does this apply to this version of the patch?
>
> Aye. lgtm

Thanks for the patch and review, pushed to dinq.

BR,
Jani.


>
>> 
>> BR,
>> Jani.
>> 
>> 
>> > ---
>> >  drivers/gpu/drm/i915/intel_hdmi.c | 17 ++---------------
>> >  1 file changed, 2 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> > index e1005d7b75fd..991eb362ef4f 100644
>> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> > @@ -856,19 +856,6 @@ static void g4x_set_infoframes(struct intel_encoder *encoder,
>> >  			      &crtc_state->infoframes.hdmi);
>> >  }
>> >  
>> > -static bool hdmi_sink_is_deep_color(const struct drm_connector_state *conn_state)
>> > -{
>> > -	struct drm_connector *connector = conn_state->connector;
>> > -
>> > -	/*
>> > -	 * HDMI cloning is only supported on g4x which doesn't
>> > -	 * support deep color or GCP infoframes anyway so no
>> > -	 * need to worry about multiple HDMI sinks here.
>> > -	 */
>> > -
>> > -	return connector->display_info.bpc > 8;
>> > -}
>> > -
>> >  /*
>> >   * Determine if default_phase=1 can be indicated in the GCP infoframe.
>> >   *
>> > @@ -973,8 +960,8 @@ 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 indication for deep color mode */
>> > +	if (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

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