| Message ID | 20190426011950.15136-1-aditya.swarup@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v8] drm/i915/icl: Set GCP_COLOR_INDICATION only for 10/12 bit deep color | expand |
On Thu, Apr 25, 2019 at 06:19:50PM -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. > > 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) > > Co-Developed-by: Aditya Swarup <aditya.swarup@intel.com> > Co-Developed-by: Ville Syrjälä <ville.syrjala@linux.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> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index e1005d7b75fd..620bc89e2120 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -973,9 +973,18 @@ 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)) > - crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > + /* Indicate color depth whenever the sink supports deep color: > + * For Gen 7.5 or later platforms, indicate color depth only for deep > + * color modes. > + * Pre DDI platforms, indicate color depth if deep color is supported > + * by sink. > + * Exception: CHERRYVIEW behaves like Pre DDI platforms. > + */ > + if (hdmi_sink_is_deep_color(conn_state)) { > + if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) || > + crtc_state->pipe_bpp > 24) I prefer the earlier version. Less special casing. > + crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > + } > > /* Enable default_phase whenever the display mode is suitably aligned */ > if (gcp_default_phase_possible(crtc_state->pipe_bpp, > @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > if (bpc == 10 && INTEL_GEN(dev_priv) < 11) > return false; > > - if (crtc_state->pipe_bpp <= 8*3) > + if (crtc_state->pipe_bpp < bpc*3) This should be a separate patch. > return false; > > if (!crtc_state->has_hdmi_sink) > -- > 2.17.1
On Fri, Apr 26, 2019 at 01:14:44PM +0300, Ville Syrjälä wrote: > On Thu, Apr 25, 2019 at 06:19:50PM -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. > > > > 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) > > > > Co-Developed-by: Aditya Swarup <aditya.swarup@intel.com> > > Co-Developed-by: Ville Syrjälä <ville.syrjala@linux.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> > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > index e1005d7b75fd..620bc89e2120 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -973,9 +973,18 @@ 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)) > > - crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > + /* Indicate color depth whenever the sink supports deep color: > > + * For Gen 7.5 or later platforms, indicate color depth only for deep > > + * color modes. > > + * Pre DDI platforms, indicate color depth if deep color is supported > > + * by sink. > > + * Exception: CHERRYVIEW behaves like Pre DDI platforms. > > + */ > > + if (hdmi_sink_is_deep_color(conn_state)) { > > + if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) || > > + crtc_state->pipe_bpp > 24) > > I prefer the earlier version. Less special casing. Then we won't be following spec for pre DDI platforms and CHV. These conditions are required according to Bspec pages mentioned in the commit message. Also, hdmi_sink_is_deep_color check is required for pre DDI platforms, since we send GCP_COLOR_INDICATION for 24 bit color as long as display supports deep color. > > > + crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > + } > > > > /* Enable default_phase whenever the display mode is suitably aligned */ > > if (gcp_default_phase_possible(crtc_state->pipe_bpp, > > @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > > if (bpc == 10 && INTEL_GEN(dev_priv) < 11) > > return false; > > > > - if (crtc_state->pipe_bpp <= 8*3) > > + if (crtc_state->pipe_bpp < bpc*3) > > This should be a separate patch. I will create a new patch for this. > > > return false; > > > > if (!crtc_state->has_hdmi_sink) > > -- > > 2.17.1 > > -- > Ville Syrjälä > Intel
On Fri, Apr 26, 2019 at 11:22:02AM -0700, Aditya Swarup wrote: > On Fri, Apr 26, 2019 at 01:14:44PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 25, 2019 at 06:19:50PM -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. > > > > > > 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) > > > > > > Co-Developed-by: Aditya Swarup <aditya.swarup@intel.com> > > > Co-Developed-by: Ville Syrjälä <ville.syrjala@linux.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> > > > --- > > > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++---- > > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > > index e1005d7b75fd..620bc89e2120 100644 > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > @@ -973,9 +973,18 @@ 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)) > > > - crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > > + /* Indicate color depth whenever the sink supports deep color: > > > + * For Gen 7.5 or later platforms, indicate color depth only for deep > > > + * color modes. > > > + * Pre DDI platforms, indicate color depth if deep color is supported > > > + * by sink. > > > + * Exception: CHERRYVIEW behaves like Pre DDI platforms. > > > + */ > > > + if (hdmi_sink_is_deep_color(conn_state)) { > > > + if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) || > > > + crtc_state->pipe_bpp > 24) > > > > I prefer the earlier version. Less special casing. > Then we won't be following spec for pre DDI platforms and CHV. These > conditions are required according to Bspec pages mentioned in the commit > message. > > Also, hdmi_sink_is_deep_color check is required for pre DDI platforms, > since we send GCP_COLOR_INDICATION for 24 bit color as long as display > supports deep color. We don't have to send it for 24bpp even if the old hw supports doing so. Well, the HDMI spec does say "Once a Source sends a GCP with non-zero CD to a sink, it should continue sending GCPs with non-zero CD at least once per video field even if reverting to 24-bit color, as long as the Sink continues to support Deep Color." so I guess we should maybe send it? But maybe that only applies if we would swich the color deph without a full modeset (can't really see how that would work in the first place considering the symbol clock needs to be adjusted too). Anyways, I think it's better to just stick to a uniform behaviour across all platforms here. > > > > > + crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > > + } > > > > > > /* Enable default_phase whenever the display mode is suitably aligned */ > > > if (gcp_default_phase_possible(crtc_state->pipe_bpp, > > > @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > > > if (bpc == 10 && INTEL_GEN(dev_priv) < 11) > > > return false; > > > > > > - if (crtc_state->pipe_bpp <= 8*3) > > > + if (crtc_state->pipe_bpp < bpc*3) > > > > This should be a separate patch. > I will create a new patch for this. > > > > > return false; > > > > > > if (!crtc_state->has_hdmi_sink) > > > -- > > > 2.17.1 > > > > -- > > Ville Syrjälä > > Intel
On Fri, Apr 26, 2019 at 09:41:06PM +0300, Ville Syrjälä wrote: > On Fri, Apr 26, 2019 at 11:22:02AM -0700, Aditya Swarup wrote: > > On Fri, Apr 26, 2019 at 01:14:44PM +0300, Ville Syrjälä wrote: > > > On Thu, Apr 25, 2019 at 06:19:50PM -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. > > > > > > > > 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) > > > > > > > > Co-Developed-by: Aditya Swarup <aditya.swarup@intel.com> > > > > Co-Developed-by: Ville Syrjälä <ville.syrjala@linux.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> > > > > --- > > > > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++---- > > > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > > > index e1005d7b75fd..620bc89e2120 100644 > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > > @@ -973,9 +973,18 @@ 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)) > > > > - crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > > > + /* Indicate color depth whenever the sink supports deep color: > > > > + * For Gen 7.5 or later platforms, indicate color depth only for deep > > > > + * color modes. > > > > + * Pre DDI platforms, indicate color depth if deep color is supported > > > > + * by sink. > > > > + * Exception: CHERRYVIEW behaves like Pre DDI platforms. > > > > + */ > > > > + if (hdmi_sink_is_deep_color(conn_state)) { > > > > + if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) || > > > > + crtc_state->pipe_bpp > 24) > > > > > > I prefer the earlier version. Less special casing. > > Then we won't be following spec for pre DDI platforms and CHV. These > > conditions are required according to Bspec pages mentioned in the commit > > message. > > > > Also, hdmi_sink_is_deep_color check is required for pre DDI platforms, > > since we send GCP_COLOR_INDICATION for 24 bit color as long as display > > supports deep color. > > We don't have to send it for 24bpp even if the old hw supports doing so. > > Well, the HDMI spec does say > "Once a Source sends a GCP with non-zero CD to a sink, it should > continue sending GCPs with non-zero CD at least once per video > field even if reverting to 24-bit color, as long as the Sink > continues to support Deep Color." > > so I guess we should maybe send it? But maybe that only applies if > we would swich the color deph without a full modeset (can't really > see how that would work in the first place considering the symbol > clock needs to be adjusted too). > > Anyways, I think it's better to just stick to a uniform behaviour > across all platforms here. I don't have any problems with that but according to Bspec: 7854 "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." The only thing that concerns me is that the spec says it "must" be set if transcoder can receive GCP data. If you still feel that it is not necessary, then I will remove the platform checks. > > > > > > > > + crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > > > + } > > > > > > > > /* Enable default_phase whenever the display mode is suitably aligned */ > > > > if (gcp_default_phase_possible(crtc_state->pipe_bpp, > > > > @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > > > > if (bpc == 10 && INTEL_GEN(dev_priv) < 11) > > > > return false; > > > > > > > > - if (crtc_state->pipe_bpp <= 8*3) > > > > + if (crtc_state->pipe_bpp < bpc*3) > > > > > > This should be a separate patch. > > I will create a new patch for this. > > > > > > > return false; > > > > > > > > if (!crtc_state->has_hdmi_sink) > > > > -- > > > > 2.17.1 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Fri, Apr 26, 2019 at 11:53:54AM -0700, Aditya Swarup wrote: > On Fri, Apr 26, 2019 at 09:41:06PM +0300, Ville Syrjälä wrote: > > On Fri, Apr 26, 2019 at 11:22:02AM -0700, Aditya Swarup wrote: > > > On Fri, Apr 26, 2019 at 01:14:44PM +0300, Ville Syrjälä wrote: > > > > On Thu, Apr 25, 2019 at 06:19:50PM -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. > > > > > > > > > > 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) > > > > > > > > > > Co-Developed-by: Aditya Swarup <aditya.swarup@intel.com> > > > > > Co-Developed-by: Ville Syrjälä <ville.syrjala@linux.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> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++---- > > > > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > > > > index e1005d7b75fd..620bc89e2120 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > > > @@ -973,9 +973,18 @@ 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)) > > > > > - crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > > > > + /* Indicate color depth whenever the sink supports deep color: > > > > > + * For Gen 7.5 or later platforms, indicate color depth only for deep > > > > > + * color modes. > > > > > + * Pre DDI platforms, indicate color depth if deep color is supported > > > > > + * by sink. > > > > > + * Exception: CHERRYVIEW behaves like Pre DDI platforms. > > > > > + */ > > > > > + if (hdmi_sink_is_deep_color(conn_state)) { > > > > > + if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) || > > > > > + crtc_state->pipe_bpp > 24) > > > > > > > > I prefer the earlier version. Less special casing. > > > Then we won't be following spec for pre DDI platforms and CHV. These > > > conditions are required according to Bspec pages mentioned in the commit > > > message. > > > > > > Also, hdmi_sink_is_deep_color check is required for pre DDI platforms, > > > since we send GCP_COLOR_INDICATION for 24 bit color as long as display > > > supports deep color. > > > > We don't have to send it for 24bpp even if the old hw supports doing so. > > > > Well, the HDMI spec does say > > "Once a Source sends a GCP with non-zero CD to a sink, it should > > continue sending GCPs with non-zero CD at least once per video > > field even if reverting to 24-bit color, as long as the Sink > > continues to support Deep Color." > > > > so I guess we should maybe send it? But maybe that only applies if > > we would swich the color deph without a full modeset (can't really > > see how that would work in the first place considering the symbol > > clock needs to be adjusted too). > > > > Anyways, I think it's better to just stick to a uniform behaviour > > across all platforms here. > > I don't have any problems with that but according to > > Bspec: 7854 > > "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." > > The only thing that concerns me is that the spec says it "must" be set > if transcoder can receive GCP data. That note is contradicting itself so clearly it can't be trusted. > > If you still feel that it is not necessary, then I will remove the > platform checks. We can't set it on hsw+ anyway, so someone definitely came to the conclusion that it's not needed. > > > > > > > > > > > > + crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > > > > > + } > > > > > > > > > > /* Enable default_phase whenever the display mode is suitably aligned */ > > > > > if (gcp_default_phase_possible(crtc_state->pipe_bpp, > > > > > @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > > > > > if (bpc == 10 && INTEL_GEN(dev_priv) < 11) > > > > > return false; > > > > > > > > > > - if (crtc_state->pipe_bpp <= 8*3) > > > > > + if (crtc_state->pipe_bpp < bpc*3) > > > > > > > > This should be a separate patch. > > > I will create a new patch for this. > > > > > > > > > return false; > > > > > > > > > > if (!crtc_state->has_hdmi_sink) > > > > > -- > > > > > 2.17.1 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
On Thu, 25 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) > > Co-Developed-by: Aditya Swarup <aditya.swarup@intel.com> > Co-Developed-by: Ville Syrjälä <ville.syrjala@linux.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> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index e1005d7b75fd..620bc89e2120 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -973,9 +973,18 @@ 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)) > - crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > + /* Indicate color depth whenever the sink supports deep color: Please fix the comment formatting for multi-line comments: /* * First comment line. * ... * Last comment line. */ > + * For Gen 7.5 or later platforms, indicate color depth only for deep > + * color modes. > + * Pre DDI platforms, indicate color depth if deep color is supported > + * by sink. > + * Exception: CHERRYVIEW behaves like Pre DDI platforms. > + */ > + if (hdmi_sink_is_deep_color(conn_state)) { > + if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) || > + crtc_state->pipe_bpp > 24) > + crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; > + } > > /* Enable default_phase whenever the display mode is suitably aligned */ > if (gcp_default_phase_possible(crtc_state->pipe_bpp, > @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, > if (bpc == 10 && INTEL_GEN(dev_priv) < 11) > return false; > > - if (crtc_state->pipe_bpp <= 8*3) > + if (crtc_state->pipe_bpp < bpc*3) > return false; > > if (!crtc_state->has_hdmi_sink)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index e1005d7b75fd..620bc89e2120 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -973,9 +973,18 @@ 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)) - crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; + /* Indicate color depth whenever the sink supports deep color: + * For Gen 7.5 or later platforms, indicate color depth only for deep + * color modes. + * Pre DDI platforms, indicate color depth if deep color is supported + * by sink. + * Exception: CHERRYVIEW behaves like Pre DDI platforms. + */ + if (hdmi_sink_is_deep_color(conn_state)) { + if(!HAS_DDI(dev_priv) || IS_CHERRYVIEW(dev_priv) || + crtc_state->pipe_bpp > 24) + crtc_state->infoframes.gcp |= GCP_COLOR_INDICATION; + } /* Enable default_phase whenever the display mode is suitably aligned */ if (gcp_default_phase_possible(crtc_state->pipe_bpp, @@ -2172,7 +2181,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state, if (bpc == 10 && INTEL_GEN(dev_priv) < 11) return false; - if (crtc_state->pipe_bpp <= 8*3) + if (crtc_state->pipe_bpp < bpc*3) return false; if (!crtc_state->has_hdmi_sink)