Re: [Intel-gfx] [PATCH 1/5] drm/i915: Clean up ADPA pipe select bits
On Thu, May 17, 2018 at 12:31:59PM +0300, Jani Nikula wrote: > On Mon, 14 May 2018, Ville Syrjalawrote: > > From: Ville Syrjälä > > > > Clean up the ADPA pipe select bits. To make the whole situation a bit > > less ugly we'll start to share the same code between .get_hw_state() > > and the port state asserts. > > > > v2: Order the defines shift,mask,value (Jani) > > > > Reviewed-by: Jani Nikula > > Yup, the series looks good. Cool. Pushed to dinq. > > BR, > Jani. > > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/i915_reg.h | 11 +- > > drivers/gpu/drm/i915/intel_crt.c | 40 > > ++-- > > drivers/gpu/drm/i915/intel_display.c | 24 +- > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > 4 files changed, 33 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index f11bb213ec07..ae3c26216996 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -4133,11 +4133,12 @@ enum { > > > > #define ADPA_DAC_ENABLE (1<<31) > > #define ADPA_DAC_DISABLE 0 > > -#define ADPA_PIPE_SELECT_MASK(1<<30) > > -#define ADPA_PIPE_A_SELECT 0 > > -#define ADPA_PIPE_B_SELECT (1<<30) > > -#define ADPA_PIPE_SELECT(pipe) ((pipe) << 30) > > -/* CPT uses bits 29:30 for pch transcoder select */ > > +#define ADPA_PIPE_SEL_SHIFT 30 > > +#define ADPA_PIPE_SEL_MASK (1<<30) > > +#define ADPA_PIPE_SEL(pipe) ((pipe) << 30) > > +#define ADPA_PIPE_SEL_SHIFT_CPT 29 > > +#define ADPA_PIPE_SEL_MASK_CPT (3<<29) > > +#define ADPA_PIPE_SEL_CPT(pipe) ((pipe) << 29) > > #define ADPA_CRT_HOTPLUG_MASK 0x03ff /* bit 25-16 */ > > #define ADPA_CRT_HOTPLUG_MONITOR_NONE (0<<24) > > #define ADPA_CRT_HOTPLUG_MONITOR_MASK (3<<24) > > diff --git a/drivers/gpu/drm/i915/intel_crt.c > > b/drivers/gpu/drm/i915/intel_crt.c > > index de0e22322c76..211d601cd1b1 100644 > > --- a/drivers/gpu/drm/i915/intel_crt.c > > +++ b/drivers/gpu/drm/i915/intel_crt.c > > @@ -63,33 +63,35 @@ static struct intel_crt *intel_attached_crt(struct > > drm_connector *connector) > > return intel_encoder_to_crt(intel_attached_encoder(connector)); > > } > > > > +bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, > > + i915_reg_t adpa_reg, enum pipe *pipe) > > +{ > > + u32 val; > > + > > + val = I915_READ(adpa_reg); > > + > > + /* asserts want to know the pipe even if the port is disabled */ > > + if (HAS_PCH_CPT(dev_priv)) > > + *pipe = (val & ADPA_PIPE_SEL_MASK_CPT) >> > > ADPA_PIPE_SEL_SHIFT_CPT; > > + else > > + *pipe = (val & ADPA_PIPE_SEL_MASK) >> ADPA_PIPE_SEL_SHIFT; > > + > > + return val & ADPA_DAC_ENABLE; > > +} > > + > > static bool intel_crt_get_hw_state(struct intel_encoder *encoder, > >enum pipe *pipe) > > { > > - struct drm_device *dev = encoder->base.dev; > > - struct drm_i915_private *dev_priv = to_i915(dev); > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_crt *crt = intel_encoder_to_crt(encoder); > > - u32 tmp; > > bool ret; > > > > if (!intel_display_power_get_if_enabled(dev_priv, > > encoder->power_domain)) > > return false; > > > > - ret = false; > > - > > - tmp = I915_READ(crt->adpa_reg); > > - > > - if (!(tmp & ADPA_DAC_ENABLE)) > > - goto out; > > + ret = intel_crt_port_enabled(dev_priv, crt->adpa_reg, pipe); > > > > - if (HAS_PCH_CPT(dev_priv)) > > - *pipe = PORT_TO_PIPE_CPT(tmp); > > - else > > - *pipe = PORT_TO_PIPE(tmp); > > - > > - ret = true; > > -out: > > intel_display_power_put(dev_priv, encoder->power_domain); > > > > return ret; > > @@ -168,11 +170,9 @@ static void intel_crt_set_dpms(struct intel_encoder > > *encoder, > > if (HAS_PCH_LPT(dev_priv)) > > ; /* Those bits don't exist here */ > > else if (HAS_PCH_CPT(dev_priv)) > > - adpa |= PORT_TRANS_SEL_CPT(crtc->pipe); > > - else if (crtc->pipe == 0) > > - adpa |= ADPA_PIPE_A_SELECT; > > + adpa |= ADPA_PIPE_SEL_CPT(crtc->pipe); > > else > > - adpa |= ADPA_PIPE_B_SELECT; > > + adpa |= ADPA_PIPE_SEL(crtc->pipe); > > > > if (!HAS_PCH_SPLIT(dev_priv)) > > I915_WRITE(BCLRPAT(crtc->pipe), 0); > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index ad588d564198..6daa8d97a0aa 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1360,21 +1360,6 @@ static bool lvds_pipe_enabled(struct > > drm_i915_private *dev_priv, > >
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Clean up ADPA pipe select bits
On Mon, 14 May 2018, Ville Syrjalawrote: > From: Ville Syrjälä > > Clean up the ADPA pipe select bits. To make the whole situation a bit > less ugly we'll start to share the same code between .get_hw_state() > and the port state asserts. > > v2: Order the defines shift,mask,value (Jani) > > Reviewed-by: Jani Nikula Yup, the series looks good. BR, Jani. > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/i915_reg.h | 11 +- > drivers/gpu/drm/i915/intel_crt.c | 40 > ++-- > drivers/gpu/drm/i915/intel_display.c | 24 +- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > 4 files changed, 33 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index f11bb213ec07..ae3c26216996 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4133,11 +4133,12 @@ enum { > > #define ADPA_DAC_ENABLE(1<<31) > #define ADPA_DAC_DISABLE 0 > -#define ADPA_PIPE_SELECT_MASK (1<<30) > -#define ADPA_PIPE_A_SELECT 0 > -#define ADPA_PIPE_B_SELECT (1<<30) > -#define ADPA_PIPE_SELECT(pipe) ((pipe) << 30) > -/* CPT uses bits 29:30 for pch transcoder select */ > +#define ADPA_PIPE_SEL_SHIFT30 > +#define ADPA_PIPE_SEL_MASK (1<<30) > +#define ADPA_PIPE_SEL(pipe)((pipe) << 30) > +#define ADPA_PIPE_SEL_SHIFT_CPT29 > +#define ADPA_PIPE_SEL_MASK_CPT (3<<29) > +#define ADPA_PIPE_SEL_CPT(pipe)((pipe) << 29) > #define ADPA_CRT_HOTPLUG_MASK 0x03ff /* bit 25-16 */ > #define ADPA_CRT_HOTPLUG_MONITOR_NONE (0<<24) > #define ADPA_CRT_HOTPLUG_MONITOR_MASK (3<<24) > diff --git a/drivers/gpu/drm/i915/intel_crt.c > b/drivers/gpu/drm/i915/intel_crt.c > index de0e22322c76..211d601cd1b1 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -63,33 +63,35 @@ static struct intel_crt *intel_attached_crt(struct > drm_connector *connector) > return intel_encoder_to_crt(intel_attached_encoder(connector)); > } > > +bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, > + i915_reg_t adpa_reg, enum pipe *pipe) > +{ > + u32 val; > + > + val = I915_READ(adpa_reg); > + > + /* asserts want to know the pipe even if the port is disabled */ > + if (HAS_PCH_CPT(dev_priv)) > + *pipe = (val & ADPA_PIPE_SEL_MASK_CPT) >> > ADPA_PIPE_SEL_SHIFT_CPT; > + else > + *pipe = (val & ADPA_PIPE_SEL_MASK) >> ADPA_PIPE_SEL_SHIFT; > + > + return val & ADPA_DAC_ENABLE; > +} > + > static bool intel_crt_get_hw_state(struct intel_encoder *encoder, > enum pipe *pipe) > { > - struct drm_device *dev = encoder->base.dev; > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_crt *crt = intel_encoder_to_crt(encoder); > - u32 tmp; > bool ret; > > if (!intel_display_power_get_if_enabled(dev_priv, > encoder->power_domain)) > return false; > > - ret = false; > - > - tmp = I915_READ(crt->adpa_reg); > - > - if (!(tmp & ADPA_DAC_ENABLE)) > - goto out; > + ret = intel_crt_port_enabled(dev_priv, crt->adpa_reg, pipe); > > - if (HAS_PCH_CPT(dev_priv)) > - *pipe = PORT_TO_PIPE_CPT(tmp); > - else > - *pipe = PORT_TO_PIPE(tmp); > - > - ret = true; > -out: > intel_display_power_put(dev_priv, encoder->power_domain); > > return ret; > @@ -168,11 +170,9 @@ static void intel_crt_set_dpms(struct intel_encoder > *encoder, > if (HAS_PCH_LPT(dev_priv)) > ; /* Those bits don't exist here */ > else if (HAS_PCH_CPT(dev_priv)) > - adpa |= PORT_TRANS_SEL_CPT(crtc->pipe); > - else if (crtc->pipe == 0) > - adpa |= ADPA_PIPE_A_SELECT; > + adpa |= ADPA_PIPE_SEL_CPT(crtc->pipe); > else > - adpa |= ADPA_PIPE_B_SELECT; > + adpa |= ADPA_PIPE_SEL(crtc->pipe); > > if (!HAS_PCH_SPLIT(dev_priv)) > I915_WRITE(BCLRPAT(crtc->pipe), 0); > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index ad588d564198..6daa8d97a0aa 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1360,21 +1360,6 @@ static bool lvds_pipe_enabled(struct drm_i915_private > *dev_priv, > return true; > } > > -static bool adpa_pipe_enabled(struct drm_i915_private *dev_priv, > - enum pipe pipe, u32 val) > -{ > - if ((val & ADPA_DAC_ENABLE) == 0) > - return false; > - if (HAS_PCH_CPT(dev_priv))