| Message ID | 1437121022-28678-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2015-07-17 at 13:47 +0530, Sonika Jindal wrote: > As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic > and interrupts to check the external panel connection and DDIC HPD > logic for edp panel. > > v2: For DP, irq_port is used to determine the encoder instead of > hpd_pin and removing the edp HPD logic because port A HPD is not > present(Imre) > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 10 +++++++++- > drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index e2c6f73..777e3a3 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3225,7 +3225,15 @@ void intel_ddi_init(struct drm_device *dev, enum port port) > goto err; > > intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; > - dev_priv->hotplug.irq_port[port] = intel_dig_port; > + /* > + * On BXT A0/A1, sw needs to activate DDIA HPD logic and > + * interrupts to check the external panel connection. > + */ > + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0) > + && port == PORT_B) > + dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; This happens to work but is confusing. irq_port[PORT_A] will be set here already and the above will simply overwrite it without explanation. I would also handle the port == PORT_A case and not set irq_port for it. The same swapping for hpd_pin is missing from intel_dp_init_connector(). > + else > + dev_priv->hotplug.irq_port[port] = intel_dig_port; > } > > /* In theory we don't need the encoder->type check, but leave it just in > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 70bad5b..94fa716 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1973,7 +1973,14 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, > intel_hdmi->ddc_bus = GMBUS_PIN_1_BXT; > else > intel_hdmi->ddc_bus = GMBUS_PIN_DPB; > - intel_encoder->hpd_pin = HPD_PORT_B; > + /* > + * On BXT A0/A1, sw needs to activate DDIA HPD logic and > + * interrupts to check the external panel connection. > + */ > + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) > + intel_encoder->hpd_pin = HPD_PORT_A; > + else > + intel_encoder->hpd_pin = HPD_PORT_B; > break; > case PORT_C: > if (IS_BROXTON(dev_priv)) As I earlier pointed out with the above approach, you need to add support for HPD events on the HPD_PORT_A pin. If you look at the for_each_hpd_pin() macro and intel_hpd_irq_handler()/is_dig_port you'll notice that any interrupt event on the HPD_PORT_A pin will be ignored now. --Imre
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6822
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 315/315 315/315
IVB 342/342 342/342
BYT -2 285/285 283/285
HSW 378/378 378/378
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads-display PASS(1) FAIL(1)
*BYT igt@gem_partial_pwrite_pread@reads-uncached PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
On 7/18/2015 5:17 AM, Imre Deak wrote: > On Fri, 2015-07-17 at 13:47 +0530, Sonika Jindal wrote: >> As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic >> and interrupts to check the external panel connection and DDIC HPD >> logic for edp panel. >> >> v2: For DP, irq_port is used to determine the encoder instead of >> hpd_pin and removing the edp HPD logic because port A HPD is not >> present(Imre) >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 10 +++++++++- >> drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- >> 2 files changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index e2c6f73..777e3a3 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -3225,7 +3225,15 @@ void intel_ddi_init(struct drm_device *dev, enum port port) >> goto err; >> >> intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; >> - dev_priv->hotplug.irq_port[port] = intel_dig_port; >> + /* >> + * On BXT A0/A1, sw needs to activate DDIA HPD logic and >> + * interrupts to check the external panel connection. >> + */ >> + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0) >> + && port == PORT_B) >> + dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; > > This happens to work but is confusing. irq_port[PORT_A] will be set here > already and the above will simply overwrite it without explanation. I > would also handle the port == PORT_A case and not set irq_port for it. > > The same swapping for hpd_pin is missing from intel_dp_init_connector(). > >> + else >> + dev_priv->hotplug.irq_port[port] = intel_dig_port; >> } >> >> /* In theory we don't need the encoder->type check, but leave it just in >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index 70bad5b..94fa716 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1973,7 +1973,14 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, >> intel_hdmi->ddc_bus = GMBUS_PIN_1_BXT; >> else >> intel_hdmi->ddc_bus = GMBUS_PIN_DPB; >> - intel_encoder->hpd_pin = HPD_PORT_B; >> + /* >> + * On BXT A0/A1, sw needs to activate DDIA HPD logic and >> + * interrupts to check the external panel connection. >> + */ >> + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) >> + intel_encoder->hpd_pin = HPD_PORT_A; >> + else >> + intel_encoder->hpd_pin = HPD_PORT_B; >> break; >> case PORT_C: >> if (IS_BROXTON(dev_priv)) > > As I earlier pointed out with the above approach, you need to add > support for HPD events on the HPD_PORT_A pin. If you look at the > for_each_hpd_pin() macro and intel_hpd_irq_handler()/is_dig_port you'll > notice that any interrupt event on the HPD_PORT_A pin will be ignored > now. Hmm :(. For now, we can fix for_each_hpd_pin and add a check in intel_hpd_pin_to_port to return PORT_B for pin 0 if A0/A1. Then we can skip the check in intel_ddi_init. But this again will look very confusing. So two options: 1. We move back to the older approach where we just use another hpd ports array in i915_irq.c 2. Go with current approach. I feel option 1 is more clean and less prone to further issues. What do you suggest? Regards, Sonika > > --Imre >
On Mon, 2015-07-20 at 11:36 +0530, Jindal, Sonika wrote: > > On 7/18/2015 5:17 AM, Imre Deak wrote: > > On Fri, 2015-07-17 at 13:47 +0530, Sonika Jindal wrote: > >> As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic > >> and interrupts to check the external panel connection and DDIC HPD > >> logic for edp panel. > >> > >> v2: For DP, irq_port is used to determine the encoder instead of > >> hpd_pin and removing the edp HPD logic because port A HPD is not > >> present(Imre) > >> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_ddi.c | 10 +++++++++- > >> drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- > >> 2 files changed, 17 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> index e2c6f73..777e3a3 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -3225,7 +3225,15 @@ void intel_ddi_init(struct drm_device *dev, enum port port) > >> goto err; > >> > >> intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; > >> - dev_priv->hotplug.irq_port[port] = intel_dig_port; > >> + /* > >> + * On BXT A0/A1, sw needs to activate DDIA HPD logic and > >> + * interrupts to check the external panel connection. > >> + */ > >> + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0) > >> + && port == PORT_B) > >> + dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; > > > > This happens to work but is confusing. irq_port[PORT_A] will be set here > > already and the above will simply overwrite it without explanation. I > > would also handle the port == PORT_A case and not set irq_port for it. > > > > The same swapping for hpd_pin is missing from intel_dp_init_connector(). > > > >> + else > >> + dev_priv->hotplug.irq_port[port] = intel_dig_port; > >> } > >> > >> /* In theory we don't need the encoder->type check, but leave it just in > >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >> index 70bad5b..94fa716 100644 > >> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >> @@ -1973,7 +1973,14 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, > >> intel_hdmi->ddc_bus = GMBUS_PIN_1_BXT; > >> else > >> intel_hdmi->ddc_bus = GMBUS_PIN_DPB; > >> - intel_encoder->hpd_pin = HPD_PORT_B; > >> + /* > >> + * On BXT A0/A1, sw needs to activate DDIA HPD logic and > >> + * interrupts to check the external panel connection. > >> + */ > >> + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) > >> + intel_encoder->hpd_pin = HPD_PORT_A; > >> + else > >> + intel_encoder->hpd_pin = HPD_PORT_B; > >> break; > >> case PORT_C: > >> if (IS_BROXTON(dev_priv)) > > > > As I earlier pointed out with the above approach, you need to add > > support for HPD events on the HPD_PORT_A pin. If you look at the > > for_each_hpd_pin() macro and intel_hpd_irq_handler()/is_dig_port you'll > > notice that any interrupt event on the HPD_PORT_A pin will be ignored > > now. > Hmm :(. For now, we can fix for_each_hpd_pin and add a check in > intel_hpd_pin_to_port to return PORT_B for pin 0 if A0/A1. Then we can > skip the check in intel_ddi_init. But this again will look very confusing. > > So two options: > 1. We move back to the older approach where we just use another hpd > ports array in i915_irq.c > 2. Go with current approach. > > I feel option 1 is more clean and less prone to further issues. > What do you suggest? I also think that adding support for HPD on the port A pin is the better way and this would be needed in any case for eDP short pulse detection support in the future regardless of this workaround. I'll send a patchset that does this, I think on top of that we could apply the current version of your 2 workaround patches (with the comments addressed). --Imre
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index e2c6f73..777e3a3 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3225,7 +3225,15 @@ void intel_ddi_init(struct drm_device *dev, enum port port) goto err; intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; - dev_priv->hotplug.irq_port[port] = intel_dig_port; + /* + * On BXT A0/A1, sw needs to activate DDIA HPD logic and + * interrupts to check the external panel connection. + */ + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0) + && port == PORT_B) + dev_priv->hotplug.irq_port[PORT_A] = intel_dig_port; + else + dev_priv->hotplug.irq_port[port] = intel_dig_port; } /* In theory we don't need the encoder->type check, but leave it just in diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 70bad5b..94fa716 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1973,7 +1973,14 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, intel_hdmi->ddc_bus = GMBUS_PIN_1_BXT; else intel_hdmi->ddc_bus = GMBUS_PIN_DPB; - intel_encoder->hpd_pin = HPD_PORT_B; + /* + * On BXT A0/A1, sw needs to activate DDIA HPD logic and + * interrupts to check the external panel connection. + */ + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) + intel_encoder->hpd_pin = HPD_PORT_A; + else + intel_encoder->hpd_pin = HPD_PORT_B; break; case PORT_C: if (IS_BROXTON(dev_priv))
As per bspec, on BXT A0/A1, sw needs to activate DDIA HPD logic and interrupts to check the external panel connection and DDIC HPD logic for edp panel. v2: For DP, irq_port is used to determine the encoder instead of hpd_pin and removing the edp HPD logic because port A HPD is not present(Imre) Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 10 +++++++++- drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- 2 files changed, 17 insertions(+), 2 deletions(-)