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

[Clinton Taylor <Clinton.A.Taylor@xxxxxxxxx>]

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@xxxxxxxxx>
> > >
> > > 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.

-Clint


> We will file a SI bug against the current behavior.
>
> -Clint
>
>
> > > Signed-off-by: Clinton Taylor <Clinton.A.Taylor@xxxxxxxxx>
> > > Signed-off-by: Aditya Swarup <aditya.swarup@xxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > ---
> > >   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 */
> > > @@ -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx