| Message ID | 1355251711-2169-2-git-send-email-damien.lespiau@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/12/11 Damien Lespiau <damien.lespiau@gmail.com>: > From: Damien Lespiau <damien.lespiau@intel.com> > > Similarly to: > > commit 6a0d1df3d3a0d2370541164eb0595fe35dcd6de3 > Author: Damien Lespiau <damien.lespiau@intel.com> > Date: Tue Dec 11 15:18:28 2012 +0000 > > drm/i915: Preserve the FDI line reversal override bit on CPT > > DDI port support lane reversal to easy the PCB layouting work. Let's > preserve the bit configured by the BIOS (until we find how to correctly > retrieve the information from the VBT, but this does sound more fragile > then just relying on the BIOS that has, hopefully, been validated > already. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> Code looks correct, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> My only optional bikeshedding would be about the fact that we're not making optimal usage of the struct_to_another_struct() conversion macros. Whenever you declare "struct intel_digital_port" you can basically redefine all the other pointers with intel_dig_port->something instead of using the macros that maybe call container_of. > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_ddi.c | 19 ++++++++++++++++--- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index ceeac97..3f2e6cf 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4519,6 +4519,7 @@ > #define DDI_BUF_EMP_800MV_0DB_HSW (7<<24) /* Sel7 */ > #define DDI_BUF_EMP_800MV_3_5DB_HSW (8<<24) /* Sel8 */ > #define DDI_BUF_EMP_MASK (0xf<<24) > +#define DDI_BUF_PORT_REVERSAL (1<<16) > #define DDI_BUF_IS_IDLE (1<<7) > #define DDI_A_4_LANES (1<<4) > #define DDI_PORT_WIDTH_X1 (0<<1) > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index cb2dcc6..59b778d 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -201,7 +201,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc) > DP_TP_CTL_LINK_TRAIN_PAT1 | > DP_TP_CTL_ENABLE); > > - /* Configure and enable DDI_BUF_CTL for DDI E with next voltage */ > + /* Configure and enable DDI_BUF_CTL for DDI E with next voltage. > + * DDI E does not support port reversal, the functionality is > + * achieved on the PCH side in FDI_RX_CTL, so no need to set the > + * port reversal bit */ > I915_WRITE(DDI_BUF_CTL(PORT_E), > DDI_BUF_CTL_ENABLE | > ((intel_crtc->fdi_lanes - 1) << 1) | > @@ -675,8 +678,11 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder, > > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) { > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > + struct intel_digital_port *intel_dig_port = > + enc_to_dig_port(encoder); > > - intel_dp->DP = DDI_BUF_CTL_ENABLE | DDI_BUF_EMP_400MV_0DB_HSW; > + intel_dp->DP = intel_dig_port->port_reversal | > + DDI_BUF_CTL_ENABLE | DDI_BUF_EMP_400MV_0DB_HSW; > switch (intel_dp->lane_count) { > case 1: > intel_dp->DP |= DDI_PORT_WIDTH_X1; > @@ -1289,11 +1295,15 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) > int type = intel_encoder->type; > > if (type == INTEL_OUTPUT_HDMI) { > + struct intel_digital_port *intel_dig_port = > + enc_to_dig_port(encoder); > + > /* In HDMI/DVI mode, the port width, and swing/emphasis values > * are ignored so nothing special needs to be done besides > * enabling the port. > */ > - I915_WRITE(DDI_BUF_CTL(port), DDI_BUF_CTL_ENABLE); > + I915_WRITE(DDI_BUF_CTL(port), > + intel_dig_port->port_reversal | DDI_BUF_CTL_ENABLE); > } else if (type == INTEL_OUTPUT_EDP) { > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > @@ -1455,6 +1465,7 @@ static const struct drm_encoder_helper_funcs intel_ddi_helper_funcs = { > > void intel_ddi_init(struct drm_device *dev, enum port port) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_digital_port *intel_dig_port; > struct intel_encoder *intel_encoder; > struct drm_encoder *encoder; > @@ -1495,6 +1506,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port) > intel_encoder->get_hw_state = intel_ddi_get_hw_state; > > intel_dig_port->port = port; > + intel_dig_port->port_reversal = I915_READ(DDI_BUF_CTL(port)) & > + DDI_BUF_PORT_REVERSAL; > if (hdmi_connector) > intel_dig_port->hdmi.sdvox_reg = DDI_BUF_CTL(port); > else > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 8a1bd4a..afa45a9 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -377,6 +377,7 @@ struct intel_dp { > struct intel_digital_port { > struct intel_encoder base; > enum port port; > + u32 port_reversal; > struct intel_dp dp; > struct intel_hdmi hdmi; > }; > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Feb 14, 2013 at 06:15:58PM -0200, Paulo Zanoni wrote: > Hi > > 2012/12/11 Damien Lespiau <damien.lespiau@gmail.com>: > > From: Damien Lespiau <damien.lespiau@intel.com> > > > > Similarly to: > > > > commit 6a0d1df3d3a0d2370541164eb0595fe35dcd6de3 > > Author: Damien Lespiau <damien.lespiau@intel.com> > > Date: Tue Dec 11 15:18:28 2012 +0000 > > > > drm/i915: Preserve the FDI line reversal override bit on CPT > > > > DDI port support lane reversal to easy the PCB layouting work. Let's > > preserve the bit configured by the BIOS (until we find how to correctly > > retrieve the information from the VBT, but this does sound more fragile > > then just relying on the BIOS that has, hopefully, been validated > > already. > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > Code looks correct, so: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Queued for -next, thanks for the patch. -Daniel > > My only optional bikeshedding would be about the fact that we're not > making optimal usage of the struct_to_another_struct() conversion > macros. Whenever you declare "struct intel_digital_port" you can > basically redefine all the other pointers with > intel_dig_port->something instead of using the macros that maybe call > container_of. > > > --- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_ddi.c | 19 ++++++++++++++++--- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 3 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index ceeac97..3f2e6cf 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -4519,6 +4519,7 @@ > > #define DDI_BUF_EMP_800MV_0DB_HSW (7<<24) /* Sel7 */ > > #define DDI_BUF_EMP_800MV_3_5DB_HSW (8<<24) /* Sel8 */ > > #define DDI_BUF_EMP_MASK (0xf<<24) > > +#define DDI_BUF_PORT_REVERSAL (1<<16) > > #define DDI_BUF_IS_IDLE (1<<7) > > #define DDI_A_4_LANES (1<<4) > > #define DDI_PORT_WIDTH_X1 (0<<1) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index cb2dcc6..59b778d 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -201,7 +201,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc) > > DP_TP_CTL_LINK_TRAIN_PAT1 | > > DP_TP_CTL_ENABLE); > > > > - /* Configure and enable DDI_BUF_CTL for DDI E with next voltage */ > > + /* Configure and enable DDI_BUF_CTL for DDI E with next voltage. > > + * DDI E does not support port reversal, the functionality is > > + * achieved on the PCH side in FDI_RX_CTL, so no need to set the > > + * port reversal bit */ > > I915_WRITE(DDI_BUF_CTL(PORT_E), > > DDI_BUF_CTL_ENABLE | > > ((intel_crtc->fdi_lanes - 1) << 1) | > > @@ -675,8 +678,11 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder, > > > > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) { > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + struct intel_digital_port *intel_dig_port = > > + enc_to_dig_port(encoder); > > > > - intel_dp->DP = DDI_BUF_CTL_ENABLE | DDI_BUF_EMP_400MV_0DB_HSW; > > + intel_dp->DP = intel_dig_port->port_reversal | > > + DDI_BUF_CTL_ENABLE | DDI_BUF_EMP_400MV_0DB_HSW; > > switch (intel_dp->lane_count) { > > case 1: > > intel_dp->DP |= DDI_PORT_WIDTH_X1; > > @@ -1289,11 +1295,15 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) > > int type = intel_encoder->type; > > > > if (type == INTEL_OUTPUT_HDMI) { > > + struct intel_digital_port *intel_dig_port = > > + enc_to_dig_port(encoder); > > + > > /* In HDMI/DVI mode, the port width, and swing/emphasis values > > * are ignored so nothing special needs to be done besides > > * enabling the port. > > */ > > - I915_WRITE(DDI_BUF_CTL(port), DDI_BUF_CTL_ENABLE); > > + I915_WRITE(DDI_BUF_CTL(port), > > + intel_dig_port->port_reversal | DDI_BUF_CTL_ENABLE); > > } else if (type == INTEL_OUTPUT_EDP) { > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > > @@ -1455,6 +1465,7 @@ static const struct drm_encoder_helper_funcs intel_ddi_helper_funcs = { > > > > void intel_ddi_init(struct drm_device *dev, enum port port) > > { > > + struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_digital_port *intel_dig_port; > > struct intel_encoder *intel_encoder; > > struct drm_encoder *encoder; > > @@ -1495,6 +1506,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port) > > intel_encoder->get_hw_state = intel_ddi_get_hw_state; > > > > intel_dig_port->port = port; > > + intel_dig_port->port_reversal = I915_READ(DDI_BUF_CTL(port)) & > > + DDI_BUF_PORT_REVERSAL; > > if (hdmi_connector) > > intel_dig_port->hdmi.sdvox_reg = DDI_BUF_CTL(port); > > else > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 8a1bd4a..afa45a9 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -377,6 +377,7 @@ struct intel_dp { > > struct intel_digital_port { > > struct intel_encoder base; > > enum port port; > > + u32 port_reversal; > > struct intel_dp dp; > > struct intel_hdmi hdmi; > > }; > > -- > > 1.7.11.7 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ceeac97..3f2e6cf 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4519,6 +4519,7 @@ #define DDI_BUF_EMP_800MV_0DB_HSW (7<<24) /* Sel7 */ #define DDI_BUF_EMP_800MV_3_5DB_HSW (8<<24) /* Sel8 */ #define DDI_BUF_EMP_MASK (0xf<<24) +#define DDI_BUF_PORT_REVERSAL (1<<16) #define DDI_BUF_IS_IDLE (1<<7) #define DDI_A_4_LANES (1<<4) #define DDI_PORT_WIDTH_X1 (0<<1) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index cb2dcc6..59b778d 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -201,7 +201,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc) DP_TP_CTL_LINK_TRAIN_PAT1 | DP_TP_CTL_ENABLE); - /* Configure and enable DDI_BUF_CTL for DDI E with next voltage */ + /* Configure and enable DDI_BUF_CTL for DDI E with next voltage. + * DDI E does not support port reversal, the functionality is + * achieved on the PCH side in FDI_RX_CTL, so no need to set the + * port reversal bit */ I915_WRITE(DDI_BUF_CTL(PORT_E), DDI_BUF_CTL_ENABLE | ((intel_crtc->fdi_lanes - 1) << 1) | @@ -675,8 +678,11 @@ static void intel_ddi_mode_set(struct drm_encoder *encoder, if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + struct intel_digital_port *intel_dig_port = + enc_to_dig_port(encoder); - intel_dp->DP = DDI_BUF_CTL_ENABLE | DDI_BUF_EMP_400MV_0DB_HSW; + intel_dp->DP = intel_dig_port->port_reversal | + DDI_BUF_CTL_ENABLE | DDI_BUF_EMP_400MV_0DB_HSW; switch (intel_dp->lane_count) { case 1: intel_dp->DP |= DDI_PORT_WIDTH_X1; @@ -1289,11 +1295,15 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) int type = intel_encoder->type; if (type == INTEL_OUTPUT_HDMI) { + struct intel_digital_port *intel_dig_port = + enc_to_dig_port(encoder); + /* In HDMI/DVI mode, the port width, and swing/emphasis values * are ignored so nothing special needs to be done besides * enabling the port. */ - I915_WRITE(DDI_BUF_CTL(port), DDI_BUF_CTL_ENABLE); + I915_WRITE(DDI_BUF_CTL(port), + intel_dig_port->port_reversal | DDI_BUF_CTL_ENABLE); } else if (type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); @@ -1455,6 +1465,7 @@ static const struct drm_encoder_helper_funcs intel_ddi_helper_funcs = { void intel_ddi_init(struct drm_device *dev, enum port port) { + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_digital_port *intel_dig_port; struct intel_encoder *intel_encoder; struct drm_encoder *encoder; @@ -1495,6 +1506,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port) intel_encoder->get_hw_state = intel_ddi_get_hw_state; intel_dig_port->port = port; + intel_dig_port->port_reversal = I915_READ(DDI_BUF_CTL(port)) & + DDI_BUF_PORT_REVERSAL; if (hdmi_connector) intel_dig_port->hdmi.sdvox_reg = DDI_BUF_CTL(port); else diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8a1bd4a..afa45a9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -377,6 +377,7 @@ struct intel_dp { struct intel_digital_port { struct intel_encoder base; enum port port; + u32 port_reversal; struct intel_dp dp; struct intel_hdmi hdmi; };