Re: [Intel-gfx] [PATCH v7] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color
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 wrote: > > > > From: Clinton Taylor > > > > > > > > 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 > > > > Signed-off-by: Aditya Swarup > > > > Cc: Ville Syrjälä > > > > Cc: Jani Nikula > > > > Cc: Manasi Navare > > > > Reviewed-by: Ville Syrjälä > > > > --- > > > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color
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 wrote: > > > From: Clinton Taylor > > > > > > 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 > > > Signed-off-by: Aditya Swarup > > > Cc: Ville Syrjälä > > > Cc: Jani Nikula > > > Cc: Manasi Navare > > > Reviewed-by: Ville Syrjälä > > > --- > > > 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. -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color
On Wed, Apr 17, 2019 at 12:57:44PM +0300, Jani Nikula wrote: > On Fri, 05 Apr 2019, Aditya Swarup wrote: > > From: Clinton Taylor > > > > 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 > > Signed-off-by: Aditya Swarup > > Cc: Ville Syrjälä > > Cc: Jani Nikula > > Cc: Manasi Navare > > Reviewed-by: Ville Syrjälä > > --- > > 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. > > > BR, > Jani. > > > > + 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 */ > > -- > Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color
On Fri, 05 Apr 2019, Aditya Swarup wrote: > From: Clinton Taylor > > 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 > Signed-off-by: Aditya Swarup > Cc: Ville Syrjälä > Cc: Jani Nikula > Cc: Manasi Navare > Reviewed-by: Ville Syrjälä > --- > 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. 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? BR, Jani. > + 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 */ -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v7] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color
From: Clinton Taylor 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 Signed-off-by: Aditya Swarup Cc: Ville Syrjälä Cc: Jani Nikula Cc: Manasi Navare Reviewed-by: Ville Syrjälä --- 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. +*/ + 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 */ -- 2.17.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v7] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color
From: Clinton Taylor 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 Signed-off-by: Aditya Swarup Cc: Ville Syrjälä Cc: Jani Nikula Cc: Manasi Navare Reviewed by: Ville Syrjälä --- 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. +*/ + 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 */ -- 2.17.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
