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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Apr 02, 2019 at 09:25:58AM -0700, Clinton Taylor wrote:
> 
> On 4/2/19 8:54 AM, Clinton Taylor wrote:
> >
> > On 4/2/19 5:53 AM, Ville Syrjälä wrote:
> >> On Tue, Apr 02, 2019 at 05:14:39AM -0700, Aditya Swarup 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.
> >> Why not? Table 6-1 in HDMI 1.4a spec clearly lists 8bpc as a
> >> valid setting for GCP.
> >
> > This appears to be a undocumented SI issue. According to the spec 8 
> > bit is a valid GCP value. However, if you enable GCP indication and 8 
> > bit is current then 10 bit is transmit via GCP confusing monitors. The 
> > GCP is being validated using 2 different HDMI Analyzer's confirming 
> > the results.
> 
> After double checking BSPEC (50524) this behavior appears to be as as 
> designed. Under Restriction for bit 2: This bit must be set when in HDMI 
> deep color (>8 BPC) mode. I don't agree with this restriction as 8 bit 
> is a valid GCP indication.

That doesn't say we musn't enable it with 8bpc.

However current Bspec has this to say:
"GCP is only supported with HDMI when the bits per color is not equal
 to 8. GCP must be enabled prior to enabling TRANS_DDI_FUNC_CTL for HDMI
 with bits per color not equal to 8 and disabled after disabling
 TRANS_DDI_FUNC_CTL"

So yeah, looks like this isn't supported by our current hw.

It was supported in the past. Eg. CPT bspec still has this to say:
"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."

But HSW bspec already forbids it. So looks like we have to disable this 
on HSW+ at least. Probably easier to just disable it across the board.
The spec seems to suggest that we should disable GCP entirely in 8bpc
mode, but there are some changlog entries in the HSW docs that
suggest to me that setting CD=0 should be sufficient. I guess that
agrees with your patch.

> 
> -Clint
> 
> 
> >
> > We will file a SI bug against the current behavior.
> >
> > -Clint
> >
> >
> >>
> >>> 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>
> >>> ---
> >>>   drivers/gpu/drm/i915/intel_hdmi.c | 31 
> >>> +++++++++++++++----------------
> >>>   1 file changed, 15 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
> >>> b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> index 5ccb305a6e1c..4760462f84ca 100644
> >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> @@ -963,7 +963,9 @@ static void 
> >>> intel_hdmi_compute_gcp_infoframe(struct intel_encoder *encoder,
> >>> 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))
> >>> +
> >>> +    if (hdmi_sink_is_deep_color(conn_state) &&
> >>> +        (crtc_state->pipe_bpp > 24))
> >>>           crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION;
> >>>         /* Enable default_phase whenever the display mode is 
> >>> suitably aligned */

So I think this hunk on its own should be sufficient. I'd like to see
the pointless parens eliminated, and a comment added that explains
that 8bpc + color depth indication is no longer supported on HSW+.

> >>> @@ -2260,6 +2262,7 @@ int intel_hdmi_compute_config(struct 
> >>> intel_encoder *encoder,
> >>>       int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> >>>       int clock_10bpc = clock_8bpc * 5 / 4;
> >>>       int clock_12bpc = clock_8bpc * 3 / 2;
> >>> +    int dc_clock = clock_12bpc;
> >>>       int desired_bpp;
> >>>       bool force_dvi = intel_conn_state->force_audio == 
> >>> HDMI_AUDIO_OFF_DVI;
> >>>   @@ -2314,22 +2317,18 @@ int intel_hdmi_compute_config(struct 
> >>> intel_encoder *encoder,
> >>>        * Note that g4x/vlv don't support 12bpc hdmi outputs. We also 
> >>> need
> >>>        * to check that the higher clock still fits within limits.
> >>>        */
> >>> -    if (hdmi_deep_color_possible(pipe_config, 12) &&
> >>> -        hdmi_port_clock_valid(intel_hdmi, clock_12bpc,
> >>> +    if (pipe_config->pipe_bpp == 30)
> >>> +        dc_clock = clock_10bpc;
> >>> +
> >>> +    if (hdmi_deep_color_possible(pipe_config, pipe_config->pipe_bpp 
> >>> / 3) &&
> >>> +        hdmi_port_clock_valid(intel_hdmi, dc_clock,
> >>>                     true, force_dvi) == MODE_OK) {
> >>> -        DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
> >>> -        desired_bpp = 12*3;
> >>> -
> >>> -        /* Need to adjust the port link by 1.5x for 12bpc. */
> >>> -        pipe_config->port_clock = clock_12bpc;
> >>> -    } else if (hdmi_deep_color_possible(pipe_config, 10) &&
> >>> -           hdmi_port_clock_valid(intel_hdmi, clock_10bpc,
> >>> -                     true, force_dvi) == MODE_OK) {
> >>> -        DRM_DEBUG_KMS("picking bpc to 10 for HDMI output\n");
> >>> -        desired_bpp = 10 * 3;
> >>> -
> >>> -        /* Need to adjust the port link by 1.25x for 10bpc. */
> >>> -        pipe_config->port_clock = clock_10bpc;
> >>> +        DRM_DEBUG_KMS("picking bpc to %d for HDMI output\n",
> >>> +                 pipe_config->pipe_bpp / 3);
> >>> +        desired_bpp = pipe_config->pipe_bpp;
> >>> +
> >>> +        /* Need to adjust the port link dc modes. */
> >>> +        pipe_config->port_clock = dc_clock;
> >> This part has nothing to do with the commit message.
> >>
> >>
> >>>       } else {
> >>>           DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> >>>           desired_bpp = 8*3;
> >>> -- 
> >>> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx