| Message ID | 1357756604-9268-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 9, 2013 at 7:36 PM, <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The RGB color range select bit on the DP/SDVO/HDMI registers > disappeared when PCH was introduced, and instead a new PIPECONF bit > was added that performs the same function. > > Add a new intel_encoder function pointer to query the encoder whether > limited or full range should be selected, and set the PIPECONF bit 13 > accordingly. > > Experimentation showed that simply toggling the bit while the pipe is > active doesn't work. We need to restart the pipe, which luckily already > happens. > > The DP/SDVO/HDMI bit 8 is marked MBZ in the docs, so avoid setting it, > although it doesn't seem to do any harm in practice. > > TODO: > - the PIPECONF bit too seems to have disappeared from HSW. Need a > volunteer to test if it's just a documentation issue or if it's really > gone. If the bit is gone and no easy replacement is found, then I suppose > we may need to use the pipe CSC unit to perform the range compression. > - move color_range to intel_encoder to avoid the silly func pointer? > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=46800 > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Can't we just add another flag to mod->private_flags like we do for 6bpc dithering? I know that that's ugly, and we need to extend this into a more generic pipe configuration struct, but we have way to many bits&pieces where encoders want to control/influence pipe state like that, so adding new virtual functions like this wont scale. -Daniel > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_hdmi.c | 8 ++++++++ > drivers/gpu/drm/i915/intel_sdvo.c | 10 +++++++++- > 6 files changed, 48 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 3b039f4..a653b64 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2650,6 +2650,7 @@ > #define PIPECONF_INTERLACED_DBL_ILK (4 << 21) /* ilk/snb only */ > #define PIPECONF_PFIT_PF_INTERLACED_DBL_ILK (5 << 21) /* ilk/snb only */ > #define PIPECONF_CXSR_DOWNCLOCK (1<<16) > +#define PIPECONF_COLOR_RANGE_SELECT (1 << 13) > #define PIPECONF_BPC_MASK (0x7 << 5) > #define PIPECONF_8BPC (0<<5) > #define PIPECONF_10BPC (1<<5) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cf0c713..4023383 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5053,6 +5053,20 @@ static int ironlake_get_refclk(struct drm_crtc *crtc) > return 120000; > } > > +static bool intel_limited_color_range(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + struct intel_encoder *intel_encoder; > + bool limited_color_range = false; > + > + for_each_encoder_on_crtc(dev, crtc, intel_encoder) { > + if (intel_encoder->limited_color_range) > + limited_color_range |= intel_encoder->limited_color_range(intel_encoder); > + } > + > + return limited_color_range; > +} > + > static void ironlake_set_pipeconf(struct drm_crtc *crtc, > struct drm_display_mode *adjusted_mode, > bool dither) > @@ -5093,6 +5107,11 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc, > else > val |= PIPECONF_PROGRESSIVE; > > + if (intel_limited_color_range(crtc)) > + val |= PIPECONF_COLOR_RANGE_SELECT; > + else > + val &= ~PIPECONF_COLOR_RANGE_SELECT; > + > I915_WRITE(PIPECONF(pipe), val); > POSTING_READ(PIPECONF(pipe)); > } > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 5f12eb2..14cb3d2 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -967,7 +967,8 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, > else > intel_dp->DP |= DP_PLL_FREQ_270MHZ; > } else if (!HAS_PCH_CPT(dev) || is_cpu_edp(intel_dp)) { > - intel_dp->DP |= intel_dp->color_range; > + if (!HAS_PCH_SPLIT(dev)) > + intel_dp->DP |= intel_dp->color_range; > > if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC) > intel_dp->DP |= DP_SYNC_HS_HIGH; > @@ -1392,6 +1393,13 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder, > return true; > } > > +static bool intel_dp_limited_color_range(struct intel_encoder *encoder) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > + > + return intel_dp->color_range; > +} > + > static void intel_disable_dp(struct intel_encoder *encoder) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > @@ -2913,6 +2921,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > intel_encoder->disable = intel_disable_dp; > intel_encoder->post_disable = intel_post_disable_dp; > intel_encoder->get_hw_state = intel_dp_get_hw_state; > + intel_encoder->limited_color_range = intel_dp_limited_color_range; > > intel_dig_port->port = port; > intel_dig_port->dp.output_reg = output_reg; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 54a034c..1f9de7c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -162,6 +162,7 @@ struct intel_encoder { > * the encoder is active. If the encoder is enabled it also set the pipe > * it is connected to in the pipe parameter. */ > bool (*get_hw_state)(struct intel_encoder *, enum pipe *pipe); > + bool (*limited_color_range)(struct intel_encoder *); > int crtc_mask; > }; > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 6387f9b..7ef617c 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -646,6 +646,13 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, > return true; > } > > +static bool intel_hdmi_limited_color_range(struct intel_encoder *encoder) > +{ > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > + > + return intel_hdmi->color_range; > +} > + > static void intel_enable_hdmi(struct intel_encoder *encoder) > { > struct drm_device *dev = encoder->base.dev; > @@ -1062,6 +1069,7 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg, enum port port) > intel_encoder->enable = intel_enable_hdmi; > intel_encoder->disable = intel_disable_hdmi; > intel_encoder->get_hw_state = intel_hdmi_get_hw_state; > + intel_encoder->limited_color_range = intel_hdmi_limited_color_range; > > intel_encoder->type = INTEL_OUTPUT_HDMI; > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 153377b..bfe855b 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -1153,7 +1153,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder, > /* The real mode polarity is set by the SDVO commands, using > * struct intel_sdvo_dtd. */ > sdvox = SDVO_VSYNC_ACTIVE_HIGH | SDVO_HSYNC_ACTIVE_HIGH; > - if (intel_sdvo->is_hdmi) > + if (!HAS_PCH_SPLIT(dev) && intel_sdvo->is_hdmi) > sdvox |= intel_sdvo->color_range; > if (INTEL_INFO(dev)->gen < 5) > sdvox |= SDVO_BORDER_ENABLE; > @@ -1228,6 +1228,13 @@ static bool intel_sdvo_get_hw_state(struct intel_encoder *encoder, > return true; > } > > +static bool intel_sdvo_limited_color_range(struct intel_encoder *encoder) > +{ > + struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base); > + > + return intel_sdvo->color_range; > +} > + > static void intel_disable_sdvo(struct intel_encoder *encoder) > { > struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > @@ -2746,6 +2753,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob) > intel_encoder->disable = intel_disable_sdvo; > intel_encoder->enable = intel_enable_sdvo; > intel_encoder->get_hw_state = intel_sdvo_get_hw_state; > + intel_encoder->limited_color_range = intel_sdvo_limited_color_range; > > /* In default case sdvo lvds is false */ > if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps)) > -- > 1.7.8.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jan 09, 2013 at 07:49:07PM +0100, Daniel Vetter wrote: > On Wed, Jan 9, 2013 at 7:36 PM, <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The RGB color range select bit on the DP/SDVO/HDMI registers > > disappeared when PCH was introduced, and instead a new PIPECONF bit > > was added that performs the same function. > > > > Add a new intel_encoder function pointer to query the encoder whether > > limited or full range should be selected, and set the PIPECONF bit 13 > > accordingly. > > > > Experimentation showed that simply toggling the bit while the pipe is > > active doesn't work. We need to restart the pipe, which luckily already > > happens. > > > > The DP/SDVO/HDMI bit 8 is marked MBZ in the docs, so avoid setting it, > > although it doesn't seem to do any harm in practice. > > > > TODO: > > - the PIPECONF bit too seems to have disappeared from HSW. Need a > > volunteer to test if it's just a documentation issue or if it's really > > gone. If the bit is gone and no easy replacement is found, then I suppose > > we may need to use the pipe CSC unit to perform the range compression. > > - move color_range to intel_encoder to avoid the silly func pointer? > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=46800 > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Can't we just add another flag to mod->private_flags like we do for > 6bpc dithering? I know that that's ugly, and we need to extend this > into a more generic pipe configuration struct, but we have way to many > bits&pieces where encoders want to control/influence pipe state like > that, so adding new virtual functions like this wont scale. Right. private_flags seems like a decent way to handle this. v2 coming up soon.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3b039f4..a653b64 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2650,6 +2650,7 @@ #define PIPECONF_INTERLACED_DBL_ILK (4 << 21) /* ilk/snb only */ #define PIPECONF_PFIT_PF_INTERLACED_DBL_ILK (5 << 21) /* ilk/snb only */ #define PIPECONF_CXSR_DOWNCLOCK (1<<16) +#define PIPECONF_COLOR_RANGE_SELECT (1 << 13) #define PIPECONF_BPC_MASK (0x7 << 5) #define PIPECONF_8BPC (0<<5) #define PIPECONF_10BPC (1<<5) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cf0c713..4023383 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5053,6 +5053,20 @@ static int ironlake_get_refclk(struct drm_crtc *crtc) return 120000; } +static bool intel_limited_color_range(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct intel_encoder *intel_encoder; + bool limited_color_range = false; + + for_each_encoder_on_crtc(dev, crtc, intel_encoder) { + if (intel_encoder->limited_color_range) + limited_color_range |= intel_encoder->limited_color_range(intel_encoder); + } + + return limited_color_range; +} + static void ironlake_set_pipeconf(struct drm_crtc *crtc, struct drm_display_mode *adjusted_mode, bool dither) @@ -5093,6 +5107,11 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc, else val |= PIPECONF_PROGRESSIVE; + if (intel_limited_color_range(crtc)) + val |= PIPECONF_COLOR_RANGE_SELECT; + else + val &= ~PIPECONF_COLOR_RANGE_SELECT; + I915_WRITE(PIPECONF(pipe), val); POSTING_READ(PIPECONF(pipe)); } diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5f12eb2..14cb3d2 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -967,7 +967,8 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, else intel_dp->DP |= DP_PLL_FREQ_270MHZ; } else if (!HAS_PCH_CPT(dev) || is_cpu_edp(intel_dp)) { - intel_dp->DP |= intel_dp->color_range; + if (!HAS_PCH_SPLIT(dev)) + intel_dp->DP |= intel_dp->color_range; if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC) intel_dp->DP |= DP_SYNC_HS_HIGH; @@ -1392,6 +1393,13 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder, return true; } +static bool intel_dp_limited_color_range(struct intel_encoder *encoder) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); + + return intel_dp->color_range; +} + static void intel_disable_dp(struct intel_encoder *encoder) { struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); @@ -2913,6 +2921,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) intel_encoder->disable = intel_disable_dp; intel_encoder->post_disable = intel_post_disable_dp; intel_encoder->get_hw_state = intel_dp_get_hw_state; + intel_encoder->limited_color_range = intel_dp_limited_color_range; intel_dig_port->port = port; intel_dig_port->dp.output_reg = output_reg; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 54a034c..1f9de7c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -162,6 +162,7 @@ struct intel_encoder { * the encoder is active. If the encoder is enabled it also set the pipe * it is connected to in the pipe parameter. */ bool (*get_hw_state)(struct intel_encoder *, enum pipe *pipe); + bool (*limited_color_range)(struct intel_encoder *); int crtc_mask; }; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 6387f9b..7ef617c 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -646,6 +646,13 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, return true; } +static bool intel_hdmi_limited_color_range(struct intel_encoder *encoder) +{ + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); + + return intel_hdmi->color_range; +} + static void intel_enable_hdmi(struct intel_encoder *encoder) { struct drm_device *dev = encoder->base.dev; @@ -1062,6 +1069,7 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg, enum port port) intel_encoder->enable = intel_enable_hdmi; intel_encoder->disable = intel_disable_hdmi; intel_encoder->get_hw_state = intel_hdmi_get_hw_state; + intel_encoder->limited_color_range = intel_hdmi_limited_color_range; intel_encoder->type = INTEL_OUTPUT_HDMI; intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 153377b..bfe855b 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1153,7 +1153,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder, /* The real mode polarity is set by the SDVO commands, using * struct intel_sdvo_dtd. */ sdvox = SDVO_VSYNC_ACTIVE_HIGH | SDVO_HSYNC_ACTIVE_HIGH; - if (intel_sdvo->is_hdmi) + if (!HAS_PCH_SPLIT(dev) && intel_sdvo->is_hdmi) sdvox |= intel_sdvo->color_range; if (INTEL_INFO(dev)->gen < 5) sdvox |= SDVO_BORDER_ENABLE; @@ -1228,6 +1228,13 @@ static bool intel_sdvo_get_hw_state(struct intel_encoder *encoder, return true; } +static bool intel_sdvo_limited_color_range(struct intel_encoder *encoder) +{ + struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base); + + return intel_sdvo->color_range; +} + static void intel_disable_sdvo(struct intel_encoder *encoder) { struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; @@ -2746,6 +2753,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob) intel_encoder->disable = intel_disable_sdvo; intel_encoder->enable = intel_enable_sdvo; intel_encoder->get_hw_state = intel_sdvo_get_hw_state; + intel_encoder->limited_color_range = intel_sdvo_limited_color_range; /* In default case sdvo lvds is false */ if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))