| Message ID | 1436852930-20830-2-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 14, 2015 at 11:18:50AM +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. > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> Yeah I think this is much clearer. Will pull in as soon as there's an r-b on these two. Thanks, Daniel > --- > drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++-- > drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 7b54b9d..c32f3d3 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > /* Set up the hotplug pin. */ > switch (port) { > case PORT_A: > - intel_encoder->hpd_pin = HPD_PORT_A; > + /* > + * On BXT A0/A1, sw needs to activate DDIC HPD logic and > + * interrupts to check the eDP panel connection. > + */ > + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) > + intel_encoder->hpd_pin = HPD_PORT_C; > + else > + intel_encoder->hpd_pin = HPD_PORT_A; > break; > case PORT_B: > - 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: > intel_encoder->hpd_pin = HPD_PORT_C; > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 44e7160..121e626 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -2011,7 +2011,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)) > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On ti, 2015-07-14 at 11:18 +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. > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++-- > drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 7b54b9d..c32f3d3 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > /* Set up the hotplug pin. */ > switch (port) { > case PORT_A: > - intel_encoder->hpd_pin = HPD_PORT_A; > + /* > + * On BXT A0/A1, sw needs to activate DDIC HPD logic and > + * interrupts to check the eDP panel connection. > + */ > + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) > + intel_encoder->hpd_pin = HPD_PORT_C; > + else > + intel_encoder->hpd_pin = HPD_PORT_A; > break; > case PORT_B: > - 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: > intel_encoder->hpd_pin = HPD_PORT_C; This won't work for a couple of reasons: atm i915_digport_work_func() assumes a fixed pin->port mapping, for example it'll call the HPD handler for the port A encoder for a short/long pulse event triggered via the HPD_PORT_A pin, whereas after the above patch you want to select port B's encoder on BXT A0/1. This could be fixed by setting up hotplug.irq_port in intel_ddi_init() according to the above change, or not using irq_port at all, but instead looking up the encoder the same way i915_hotplug_work_func() does using intel_encoder->hpd_pin. The HPD_PORT_A HPD handling is missing in general, see for_each_hpd_pin() and intel_hpd_irq_handler()/is_dig_port. So if going with the above way, these issues need to be addressed first. --Imre
On 7/14/2015 7:52 PM, Imre Deak wrote: > On ti, 2015-07-14 at 11:18 +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. >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++-- >> drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- >> 2 files changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 7b54b9d..c32f3d3 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >> /* Set up the hotplug pin. */ >> switch (port) { >> case PORT_A: >> - intel_encoder->hpd_pin = HPD_PORT_A; >> + /* >> + * On BXT A0/A1, sw needs to activate DDIC HPD logic and >> + * interrupts to check the eDP panel connection. >> + */ >> + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) >> + intel_encoder->hpd_pin = HPD_PORT_C; >> + else >> + intel_encoder->hpd_pin = HPD_PORT_A; >> break; >> case PORT_B: >> - 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: >> intel_encoder->hpd_pin = HPD_PORT_C; > > This won't work for a couple of reasons: atm i915_digport_work_func() > assumes a fixed pin->port mapping, for example it'll call the HPD > handler for the port A encoder for a short/long pulse event triggered > via the HPD_PORT_A pin, whereas after the above patch you want to select > port B's encoder on BXT A0/1. This could be fixed by setting up > hotplug.irq_port in intel_ddi_init() according to the above change, or > not using irq_port at all, but instead looking up the encoder the same > way i915_hotplug_work_func() does using intel_encoder->hpd_pin. > Hmm, i didnt realize that. Moving this check to intel_ddi_init, will again make the checks spread all over which we wanted to avoid since hpd_pin was in place. But looks like hpd_pin is only for i915_hotplug_work_func. I feel we can move back to the older approach itself What do you suggest? Daniel, any comments? > The HPD_PORT_A HPD handling is missing in general, see > for_each_hpd_pin() and intel_hpd_irq_handler()/is_dig_port. > > So if going with the above way, these issues need to be addressed first. > > --Imre >
On 7/15/2015 12:05 PM, Jindal, Sonika wrote: > > > On 7/14/2015 7:52 PM, Imre Deak wrote: >> On ti, 2015-07-14 at 11:18 +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. >>> >>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++-- >>> drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- >>> 2 files changed, 24 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>> index 7b54b9d..c32f3d3 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >>> /* Set up the hotplug pin. */ >>> switch (port) { >>> case PORT_A: >>> - intel_encoder->hpd_pin = HPD_PORT_A; >>> + /* >>> + * On BXT A0/A1, sw needs to activate DDIC HPD logic and >>> + * interrupts to check the eDP panel connection. >>> + */ >>> + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) >>> + intel_encoder->hpd_pin = HPD_PORT_C; >>> + else >>> + intel_encoder->hpd_pin = HPD_PORT_A; >>> break; >>> case PORT_B: >>> - 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: >>> intel_encoder->hpd_pin = HPD_PORT_C; >> >> This won't work for a couple of reasons: atm i915_digport_work_func() >> assumes a fixed pin->port mapping, for example it'll call the HPD >> handler for the port A encoder for a short/long pulse event triggered >> via the HPD_PORT_A pin, whereas after the above patch you want to select >> port B's encoder on BXT A0/1. This could be fixed by setting up >> hotplug.irq_port in intel_ddi_init() according to the above change, or >> not using irq_port at all, but instead looking up the encoder the same >> way i915_hotplug_work_func() does using intel_encoder->hpd_pin. >> > Hmm, i didnt realize that. > Moving this check to intel_ddi_init, will again make the checks spread > all over which we wanted to avoid since hpd_pin was in place. > But looks like hpd_pin is only for i915_hotplug_work_func. > I feel we can move back to the older approach itself > What do you suggest? > But then we can remove these checks from here, and have it only in intel_ddi_init, right? So it should look fine. For HPD_PORT_A, I think we can skip that part as of now. Please suggest.. > Daniel, any comments? > >> The HPD_PORT_A HPD handling is missing in general, see >> for_each_hpd_pin() and intel_hpd_irq_handler()/is_dig_port. >> > >> So if going with the above way, these issues need to be addressed first. >> >> --Imre >> > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Wed, Jul 15, 2015 at 01:34:29PM +0530, Jindal, Sonika wrote: > > > On 7/15/2015 12:05 PM, Jindal, Sonika wrote: > > > > > >On 7/14/2015 7:52 PM, Imre Deak wrote: > >>On ti, 2015-07-14 at 11:18 +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. > >>> > >>>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >>>--- > >>> drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++-- > >>> drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- > >>> 2 files changed, 24 insertions(+), 3 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >>>index 7b54b9d..c32f3d3 100644 > >>>--- a/drivers/gpu/drm/i915/intel_dp.c > >>>+++ b/drivers/gpu/drm/i915/intel_dp.c > >>>@@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > >>> /* Set up the hotplug pin. */ > >>> switch (port) { > >>> case PORT_A: > >>>- intel_encoder->hpd_pin = HPD_PORT_A; > >>>+ /* > >>>+ * On BXT A0/A1, sw needs to activate DDIC HPD logic and > >>>+ * interrupts to check the eDP panel connection. > >>>+ */ > >>>+ if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) > >>>+ intel_encoder->hpd_pin = HPD_PORT_C; > >>>+ else > >>>+ intel_encoder->hpd_pin = HPD_PORT_A; > >>> break; > >>> case PORT_B: > >>>- 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: > >>> intel_encoder->hpd_pin = HPD_PORT_C; > >> > >>This won't work for a couple of reasons: atm i915_digport_work_func() > >>assumes a fixed pin->port mapping, for example it'll call the HPD > >>handler for the port A encoder for a short/long pulse event triggered > >>via the HPD_PORT_A pin, whereas after the above patch you want to select > >>port B's encoder on BXT A0/1. This could be fixed by setting up > >>hotplug.irq_port in intel_ddi_init() according to the above change, or > >>not using irq_port at all, but instead looking up the encoder the same > >>way i915_hotplug_work_func() does using intel_encoder->hpd_pin. Yeah that's kinda a bug in digport_work_func, but that part is also a mess. Simplest would be to rename hotplug.irq_port to irq_pin and change the assignment for that too. > >Hmm, i didnt realize that. > >Moving this check to intel_ddi_init, will again make the checks spread > >all over which we wanted to avoid since hpd_pin was in place. > >But looks like hpd_pin is only for i915_hotplug_work_func. > >I feel we can move back to the older approach itself > >What do you suggest? > > > But then we can remove these checks from here, and have it only in > intel_ddi_init, right? So it should look fine. > > For HPD_PORT_A, I think we can skip that part as of now. > > Please suggest.. I still think that fake-handling hpd A in the low-level irq handler is massively confusing. And we need to change all the same parts anyway (i.e. you'd have to change the digport_work_func too with the old approach). -Daniel
On 7/15/2015 2:37 PM, Daniel Vetter wrote: > On Wed, Jul 15, 2015 at 01:34:29PM +0530, Jindal, Sonika wrote: >> >> >> On 7/15/2015 12:05 PM, Jindal, Sonika wrote: >>> >>> >>> On 7/14/2015 7:52 PM, Imre Deak wrote: >>>> On ti, 2015-07-14 at 11:18 +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. >>>>> >>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++-- >>>>> drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- >>>>> 2 files changed, 24 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>>> index 7b54b9d..c32f3d3 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>>> @@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >>>>> /* Set up the hotplug pin. */ >>>>> switch (port) { >>>>> case PORT_A: >>>>> - intel_encoder->hpd_pin = HPD_PORT_A; >>>>> + /* >>>>> + * On BXT A0/A1, sw needs to activate DDIC HPD logic and >>>>> + * interrupts to check the eDP panel connection. >>>>> + */ >>>>> + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) >>>>> + intel_encoder->hpd_pin = HPD_PORT_C; >>>>> + else >>>>> + intel_encoder->hpd_pin = HPD_PORT_A; >>>>> break; >>>>> case PORT_B: >>>>> - 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: >>>>> intel_encoder->hpd_pin = HPD_PORT_C; >>>> >>>> This won't work for a couple of reasons: atm i915_digport_work_func() >>>> assumes a fixed pin->port mapping, for example it'll call the HPD >>>> handler for the port A encoder for a short/long pulse event triggered >>>> via the HPD_PORT_A pin, whereas after the above patch you want to select >>>> port B's encoder on BXT A0/1. This could be fixed by setting up >>>> hotplug.irq_port in intel_ddi_init() according to the above change, or >>>> not using irq_port at all, but instead looking up the encoder the same >>>> way i915_hotplug_work_func() does using intel_encoder->hpd_pin. > > Yeah that's kinda a bug in digport_work_func, but that part is also a > mess. Simplest would be to rename hotplug.irq_port to irq_pin and change > the assignment for that too. > >>> Hmm, i didnt realize that. >>> Moving this check to intel_ddi_init, will again make the checks spread >>> all over which we wanted to avoid since hpd_pin was in place. >>> But looks like hpd_pin is only for i915_hotplug_work_func. >>> I feel we can move back to the older approach itself >>> What do you suggest? >>> >> But then we can remove these checks from here, and have it only in >> intel_ddi_init, right? So it should look fine. >> >> For HPD_PORT_A, I think we can skip that part as of now. >> >> Please suggest.. > > I still think that fake-handling hpd A in the low-level irq handler is > massively confusing. And we need to change all the same parts anyway (i.e. > you'd have to change the digport_work_func too with the old approach). > -Daniel So, for now, I will just correct it in intel_ddi_init and leave the handling of HPD for port A untouched. I will only change the irq_port for external DP (port B). And the other part with hpd_pin to handle hdmi hotplug will remain as is. Please let me know if you see any issue in this. Regards, Sonika >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7b54b9d..c32f3d3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5869,10 +5869,24 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, /* Set up the hotplug pin. */ switch (port) { case PORT_A: - intel_encoder->hpd_pin = HPD_PORT_A; + /* + * On BXT A0/A1, sw needs to activate DDIC HPD logic and + * interrupts to check the eDP panel connection. + */ + if (IS_BROXTON(dev_priv) && (INTEL_REVID(dev) < BXT_REVID_B0)) + intel_encoder->hpd_pin = HPD_PORT_C; + else + intel_encoder->hpd_pin = HPD_PORT_A; break; case PORT_B: - 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: intel_encoder->hpd_pin = HPD_PORT_C; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 44e7160..121e626 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2011,7 +2011,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. Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++-- drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++++- 2 files changed, 24 insertions(+), 3 deletions(-)