| Message ID | 20191003150653.15881-7-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DP Phy compliace auto test. | expand |
On Thu, Oct 03, 2019 at 08:36:53PM +0530, Animesh Manna wrote: > This patch process phy compliance request by programming requested > vswing, pre-emphasis and test pattern. So the design of this whole patch will need to be changed to work with the whole kms infrastructure Basically what you are doing here is handling the HPD test request, getting the test patterns. populating the intel_dp_compliance phytest struct and also writing the I915 regs right then and there. This model does not fit the atomic KMS infrastructure, IMO this is how it should look like: 1. Handle the test request 2. Prepare the PHY patterns where you erad the test patterns from DPCD and populate the intel_dp_compliance phytest struct 3. set test_active = true 4. Now in atomic_commit_tail somewhere just before enabling pipe, is where based on if intel_dp_compliance.phytest not NULL you will need to call ddi_disable, enable, set_signal_levels, phy_update on the other hand all teh functions that write data to the I915 registers will need to happen here. Clint, Jani N any thoughts here? Regards Manasi > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 62 +++++++++++++++++++++++++ > 1 file changed, 62 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 93b1ce80c174..dd4c3a81c11d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -4817,14 +4817,76 @@ static inline void intel_dp_phy_pattern_update(struct intel_dp *intel_dp) > } > } > > +static void > +intel_dp_autotest_phy_ddi_disable(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > + enum port port = intel_dig_port->base.port; > + u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; > + > + ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); > + dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); > + trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); > + > + ddi_buf_ctl_value &= ~(DDI_BUF_CTL_ENABLE | DDI_PORT_WIDTH_MASK); > + dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE; > + trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE | > + DDI_PORT_WIDTH_MASK); > + > + I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); > + I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); > + I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); > +} > + > +static void > +intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > + enum port port = intel_dig_port->base.port; > + u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; > + > + ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); > + dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); > + trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); > + > + ddi_buf_ctl_value |= DDI_BUF_CTL_ENABLE | > + DDI_PORT_WIDTH(lane_cnt); > + dp_tp_ctl_value |= DP_TP_CTL_ENABLE; > + trans_ddi_func_ctl_value |= TRANS_DDI_FUNC_ENABLE | > + DDI_PORT_WIDTH(lane_cnt); > + > + I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); > + I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); > + I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); > +} > + > static u8 intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp) > { > u8 test_result = DP_TEST_NAK; > + struct drm_dp_phy_test_params *data = > + &intel_dp->compliance.test_data.phytest; > > test_result = intel_dp_prepare_phytest(intel_dp); > if (test_result != DP_TEST_ACK) > DRM_ERROR("Phy test preparation failed\n"); > > + intel_dp_autotest_phy_ddi_disable(intel_dp); > + > + intel_dp_set_signal_levels(intel_dp); > + > + intel_dp_phy_pattern_update(intel_dp); > + > + intel_dp_autotest_phy_ddi_enable(intel_dp, data->link.num_lanes); > + > + drm_dp_set_phy_test_pattern(&intel_dp->aux, data); > + > + /* Set test active flag here so userspace doesn't interrupt things */ > + intel_dp->compliance.test_active = 1; > + > return test_result; > } > > -- > 2.22.0 >
On 10/22/2019 5:17 AM, Manasi Navare wrote: > On Thu, Oct 03, 2019 at 08:36:53PM +0530, Animesh Manna wrote: >> This patch process phy compliance request by programming requested >> vswing, pre-emphasis and test pattern. > So the design of this whole patch will need to be changed to work with the whole kms infrastructure > Basically what you are doing here is handling the HPD test request, getting the test patterns. > populating the intel_dp_compliance phytest struct and also writing the I915 regs right then and there. > > This model does not fit the atomic KMS infrastructure, IMO this is how it should look like: > > 1. Handle the test request > 2. Prepare the PHY patterns where you erad the test patterns from DPCD and populate > the intel_dp_compliance phytest struct > 3. set test_active = true > 4. Now in atomic_commit_tail somewhere just before enabling pipe, is where > based on if intel_dp_compliance.phytest not NULL you will need to call ddi_disable, enable, > set_signal_levels, phy_update on the other hand all teh functions that write data to the > I915 registers will need to happen here. Got the high level view, more brainstorming will help here to finalize the design. Hope initial patches will not much change irrespective of the design Do we need any drm-property here? The process of auto phy compliance testing is general for all driver so a thought came. Have an option to put the preparation for phy compliance request in atomic_check. Will wait for feedback from Jani, Clint and others. Regards, Animesh > > Clint, Jani N any thoughts here? > > Regards > Manasi > >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dp.c | 62 +++++++++++++++++++++++++ >> 1 file changed, 62 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index 93b1ce80c174..dd4c3a81c11d 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -4817,14 +4817,76 @@ static inline void intel_dp_phy_pattern_update(struct intel_dp *intel_dp) >> } >> } >> >> +static void >> +intel_dp_autotest_phy_ddi_disable(struct intel_dp *intel_dp) >> +{ >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> + struct drm_i915_private *dev_priv = to_i915(dev); >> + enum port port = intel_dig_port->base.port; >> + u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; >> + >> + ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); >> + dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); >> + trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); >> + >> + ddi_buf_ctl_value &= ~(DDI_BUF_CTL_ENABLE | DDI_PORT_WIDTH_MASK); >> + dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE; >> + trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE | >> + DDI_PORT_WIDTH_MASK); >> + >> + I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); >> + I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); >> + I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); >> +} >> + >> +static void >> +intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt) >> +{ >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> + struct drm_i915_private *dev_priv = to_i915(dev); >> + enum port port = intel_dig_port->base.port; >> + u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; >> + >> + ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); >> + dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); >> + trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); >> + >> + ddi_buf_ctl_value |= DDI_BUF_CTL_ENABLE | >> + DDI_PORT_WIDTH(lane_cnt); >> + dp_tp_ctl_value |= DP_TP_CTL_ENABLE; >> + trans_ddi_func_ctl_value |= TRANS_DDI_FUNC_ENABLE | >> + DDI_PORT_WIDTH(lane_cnt); >> + >> + I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); >> + I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); >> + I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); >> +} >> + >> static u8 intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp) >> { >> u8 test_result = DP_TEST_NAK; >> + struct drm_dp_phy_test_params *data = >> + &intel_dp->compliance.test_data.phytest; >> >> test_result = intel_dp_prepare_phytest(intel_dp); >> if (test_result != DP_TEST_ACK) >> DRM_ERROR("Phy test preparation failed\n"); >> >> + intel_dp_autotest_phy_ddi_disable(intel_dp); >> + >> + intel_dp_set_signal_levels(intel_dp); >> + >> + intel_dp_phy_pattern_update(intel_dp); >> + >> + intel_dp_autotest_phy_ddi_enable(intel_dp, data->link.num_lanes); >> + >> + drm_dp_set_phy_test_pattern(&intel_dp->aux, data); >> + >> + /* Set test active flag here so userspace doesn't interrupt things */ >> + intel_dp->compliance.test_active = 1; >> + >> return test_result; >> } >> >> -- >> 2.22.0 >>
On Tue, Oct 22, 2019 at 08:37:58PM +0530, Animesh Manna wrote: > > On 10/22/2019 5:17 AM, Manasi Navare wrote: > >On Thu, Oct 03, 2019 at 08:36:53PM +0530, Animesh Manna wrote: > >>This patch process phy compliance request by programming requested > >>vswing, pre-emphasis and test pattern. > >So the design of this whole patch will need to be changed to work with the whole kms infrastructure > >Basically what you are doing here is handling the HPD test request, getting the test patterns. > >populating the intel_dp_compliance phytest struct and also writing the I915 regs right then and there. > > > >This model does not fit the atomic KMS infrastructure, IMO this is how it should look like: > > > >1. Handle the test request > >2. Prepare the PHY patterns where you erad the test patterns from DPCD and populate > >the intel_dp_compliance phytest struct > >3. set test_active = true > >4. Now in atomic_commit_tail somewhere just before enabling pipe, is where > >based on if intel_dp_compliance.phytest not NULL you will need to call ddi_disable, enable, > >set_signal_levels, phy_update on the other hand all teh functions that write data to the > >I915 registers will need to happen here. > > Got the high level view, more brainstorming will help here to finalize the > design. Hope initial patches will not much change irrespective of the design > > Do we need any drm-property here? The process of auto phy compliance testing > is general for all driver so a thought came. > No need to have any property since the control to run phy test stays within the kernel. So we can just make use of whether the intel_dp_compliance.phytest struct is set or not to determine calling phy test related functions in atomic_commit_tail before enabling the pipe Manasi > Have an option to put the preparation for phy compliance request in > atomic_check. > > Will wait for feedback from Jani, Clint and others. > > Regards, > Animesh > > > > >Clint, Jani N any thoughts here? > > > >Regards > >Manasi > > > >>Signed-off-by: Animesh Manna <animesh.manna@intel.com> > >>--- > >> drivers/gpu/drm/i915/display/intel_dp.c | 62 +++++++++++++++++++++++++ > >> 1 file changed, 62 insertions(+) > >> > >>diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > >>index 93b1ce80c174..dd4c3a81c11d 100644 > >>--- a/drivers/gpu/drm/i915/display/intel_dp.c > >>+++ b/drivers/gpu/drm/i915/display/intel_dp.c > >>@@ -4817,14 +4817,76 @@ static inline void intel_dp_phy_pattern_update(struct intel_dp *intel_dp) > >> } > >> } > >>+static void > >>+intel_dp_autotest_phy_ddi_disable(struct intel_dp *intel_dp) > >>+{ > >>+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >>+ struct drm_device *dev = intel_dig_port->base.base.dev; > >>+ struct drm_i915_private *dev_priv = to_i915(dev); > >>+ enum port port = intel_dig_port->base.port; > >>+ u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; > >>+ > >>+ ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); > >>+ dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); > >>+ trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); > >>+ > >>+ ddi_buf_ctl_value &= ~(DDI_BUF_CTL_ENABLE | DDI_PORT_WIDTH_MASK); > >>+ dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE; > >>+ trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE | > >>+ DDI_PORT_WIDTH_MASK); > >>+ > >>+ I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); > >>+ I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); > >>+ I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); > >>+} > >>+ > >>+static void > >>+intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt) > >>+{ > >>+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >>+ struct drm_device *dev = intel_dig_port->base.base.dev; > >>+ struct drm_i915_private *dev_priv = to_i915(dev); > >>+ enum port port = intel_dig_port->base.port; > >>+ u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; > >>+ > >>+ ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); > >>+ dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); > >>+ trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); > >>+ > >>+ ddi_buf_ctl_value |= DDI_BUF_CTL_ENABLE | > >>+ DDI_PORT_WIDTH(lane_cnt); > >>+ dp_tp_ctl_value |= DP_TP_CTL_ENABLE; > >>+ trans_ddi_func_ctl_value |= TRANS_DDI_FUNC_ENABLE | > >>+ DDI_PORT_WIDTH(lane_cnt); > >>+ > >>+ I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); > >>+ I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); > >>+ I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); > >>+} > >>+ > >> static u8 intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp) > >> { > >> u8 test_result = DP_TEST_NAK; > >>+ struct drm_dp_phy_test_params *data = > >>+ &intel_dp->compliance.test_data.phytest; > >> test_result = intel_dp_prepare_phytest(intel_dp); > >> if (test_result != DP_TEST_ACK) > >> DRM_ERROR("Phy test preparation failed\n"); > >>+ intel_dp_autotest_phy_ddi_disable(intel_dp); > >>+ > >>+ intel_dp_set_signal_levels(intel_dp); > >>+ > >>+ intel_dp_phy_pattern_update(intel_dp); > >>+ > >>+ intel_dp_autotest_phy_ddi_enable(intel_dp, data->link.num_lanes); > >>+ > >>+ drm_dp_set_phy_test_pattern(&intel_dp->aux, data); > >>+ > >>+ /* Set test active flag here so userspace doesn't interrupt things */ > >>+ intel_dp->compliance.test_active = 1; > >>+ > >> return test_result; > >> } > >>-- > >>2.22.0 > >> >
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 93b1ce80c174..dd4c3a81c11d 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4817,14 +4817,76 @@ static inline void intel_dp_phy_pattern_update(struct intel_dp *intel_dp) } } +static void +intel_dp_autotest_phy_ddi_disable(struct intel_dp *intel_dp) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = to_i915(dev); + enum port port = intel_dig_port->base.port; + u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; + + ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); + dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); + trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); + + ddi_buf_ctl_value &= ~(DDI_BUF_CTL_ENABLE | DDI_PORT_WIDTH_MASK); + dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE; + trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE | + DDI_PORT_WIDTH_MASK); + + I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); + I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); + I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); +} + +static void +intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = to_i915(dev); + enum port port = intel_dig_port->base.port; + u32 ddi_buf_ctl_value, dp_tp_ctl_value, trans_ddi_func_ctl_value; + + ddi_buf_ctl_value = I915_READ(DDI_BUF_CTL(port)); + dp_tp_ctl_value = I915_READ(DP_TP_CTL(port)); + trans_ddi_func_ctl_value = I915_READ(TRANS_DDI_FUNC_CTL(port)); + + ddi_buf_ctl_value |= DDI_BUF_CTL_ENABLE | + DDI_PORT_WIDTH(lane_cnt); + dp_tp_ctl_value |= DP_TP_CTL_ENABLE; + trans_ddi_func_ctl_value |= TRANS_DDI_FUNC_ENABLE | + DDI_PORT_WIDTH(lane_cnt); + + I915_WRITE(TRANS_DDI_FUNC_CTL(port), trans_ddi_func_ctl_value); + I915_WRITE(DP_TP_CTL(port), dp_tp_ctl_value); + I915_WRITE(DDI_BUF_CTL(port), ddi_buf_ctl_value); +} + static u8 intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp) { u8 test_result = DP_TEST_NAK; + struct drm_dp_phy_test_params *data = + &intel_dp->compliance.test_data.phytest; test_result = intel_dp_prepare_phytest(intel_dp); if (test_result != DP_TEST_ACK) DRM_ERROR("Phy test preparation failed\n"); + intel_dp_autotest_phy_ddi_disable(intel_dp); + + intel_dp_set_signal_levels(intel_dp); + + intel_dp_phy_pattern_update(intel_dp); + + intel_dp_autotest_phy_ddi_enable(intel_dp, data->link.num_lanes); + + drm_dp_set_phy_test_pattern(&intel_dp->aux, data); + + /* Set test active flag here so userspace doesn't interrupt things */ + intel_dp->compliance.test_active = 1; + return test_result; }
This patch process phy compliance request by programming requested vswing, pre-emphasis and test pattern. Signed-off-by: Animesh Manna <animesh.manna@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 62 +++++++++++++++++++++++++ 1 file changed, 62 insertions(+)