| Message ID | 1441373176-22302-6-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 04, 2015 at 06:56:15PM +0530, Sonika Jindal wrote: > If the same port is enumerated as hdmi as well as DP, this will get > called for DP connector as well which is not required because > i915_hotplug_work_func is solely to handle hdmi HPD. > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/i915/intel_hotplug.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c > index 53c0173..8e1c43e 100644 > --- a/drivers/gpu/drm/i915/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > @@ -325,7 +325,8 @@ static void i915_hotplug_work_func(struct work_struct *work) > > list_for_each_entry(connector, &mode_config->connector_list, head) { > intel_connector = to_intel_connector(connector); > - if (!intel_connector->encoder) > + if (!intel_connector->encoder > + && connector->connector_type != DRM_MODE_CONNECTOR_HDMIA) > continue; Pretty sure this breaks hotplug detection for everything but HDMI (since now nothing but hdmi gets called it's ->detect function, and only if that signals a status change will we send out an uevent to userspace). Does this really not break DP hotplug? Yes we probe both hdmi and DP always, and probably we could be more intelligent with the fallback between the too. But this here doesn't seem to be the right solution. -Daniel > intel_encoder = intel_connector->encoder; > if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 9/4/2015 8:17 PM, Daniel Vetter wrote: > On Fri, Sep 04, 2015 at 06:56:15PM +0530, Sonika Jindal wrote: >> If the same port is enumerated as hdmi as well as DP, this will get >> called for DP connector as well which is not required because >> i915_hotplug_work_func is solely to handle hdmi HPD. >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> drivers/gpu/drm/i915/intel_hotplug.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c >> index 53c0173..8e1c43e 100644 >> --- a/drivers/gpu/drm/i915/intel_hotplug.c >> +++ b/drivers/gpu/drm/i915/intel_hotplug.c >> @@ -325,7 +325,8 @@ static void i915_hotplug_work_func(struct work_struct *work) >> >> list_for_each_entry(connector, &mode_config->connector_list, head) { >> intel_connector = to_intel_connector(connector); >> - if (!intel_connector->encoder) >> + if (!intel_connector->encoder >> + && connector->connector_type != DRM_MODE_CONNECTOR_HDMIA) >> continue; > > Pretty sure this breaks hotplug detection for everything but HDMI (since > now nothing but hdmi gets called it's ->detect function, and only if that > signals a status change will we send out an uevent to userspace). Does > this really not break DP hotplug? > > Yes we probe both hdmi and DP always, and probably we could be more > intelligent with the fallback between the too. But this here doesn't seem > to be the right solution. > -Daniel Hmm :( This doesn't allow detect to be called for dp. I missed the part that hpd_pulse only handles mst. From the name and comments it looks like it should handle hpd for dp completely. I think this this code needs some refactoring. For now, I think I better move this check to intel_encoder->hot_plug function because for the connectors with dp and hdmi both, this hot_plug hook gets called twice. > >> intel_encoder = intel_connector->encoder; >> if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
I was also going to say that I believe this breaks DP hotplug, but this was agreed already... But I also don't understand how hot_plug is called twice since it calls encoder->hot_plug... and dp has no ->hot_plug... And also if this is happening how this code that checks for connector would help... there is something else strange happening... On Sat, Sep 5, 2015 at 9:31 PM Jindal, Sonika <sonika.jindal@intel.com> wrote: > > > On 9/4/2015 8:17 PM, Daniel Vetter wrote: > > On Fri, Sep 04, 2015 at 06:56:15PM +0530, Sonika Jindal wrote: > >> If the same port is enumerated as hdmi as well as DP, this will get > >> called for DP connector as well which is not required because > >> i915_hotplug_work_func is solely to handle hdmi HPD. > >> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_hotplug.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c > b/drivers/gpu/drm/i915/intel_hotplug.c > >> index 53c0173..8e1c43e 100644 > >> --- a/drivers/gpu/drm/i915/intel_hotplug.c > >> +++ b/drivers/gpu/drm/i915/intel_hotplug.c > >> @@ -325,7 +325,8 @@ static void i915_hotplug_work_func(struct > work_struct *work) > >> > >> list_for_each_entry(connector, &mode_config->connector_list, head) > { > >> intel_connector = to_intel_connector(connector); > >> - if (!intel_connector->encoder) > >> + if (!intel_connector->encoder > >> + && connector->connector_type != > DRM_MODE_CONNECTOR_HDMIA) > >> continue; > > > > Pretty sure this breaks hotplug detection for everything but HDMI (since > > now nothing but hdmi gets called it's ->detect function, and only if that > > signals a status change will we send out an uevent to userspace). Does > > this really not break DP hotplug? > > > > Yes we probe both hdmi and DP always, and probably we could be more > > intelligent with the fallback between the too. But this here doesn't seem > > to be the right solution. > > -Daniel > > Hmm :( > This doesn't allow detect to be called for dp. I missed the part that > hpd_pulse only handles mst. From the name and comments it looks like it > should handle hpd for dp completely. I think this this code needs some > refactoring. > > For now, I think I better move this check to intel_encoder->hot_plug > function because for the connectors with dp and hdmi both, this hot_plug > hook gets called twice. > > > > >> intel_encoder = intel_connector->encoder; > >> if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { > >> -- > >> 1.7.10.4 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Since, the port is enumerated as DP as well as hdmi, the init connector is called for both and the same intel encoder gets attached to both the connectors. Now the intel_encoder’s type could be a differentiator. But since, hdmi’s detect gets called, the type of the encoder remains hdmi. When hpd occurs, it checks from the list of connectors whose hpd pin is same as the one asserted. Since both dp and hdmi are INITed, both the connectors have that pin and since DP’s connector is first in the list, it calls the ->hot_plug once when it finds DP connector matching and then again when HDMI connector is found matching as per the pin. This code check will avoid calling in the case when we have found the DP connector because we are sure that there is no ->hot_plug hook for DP. Anyways, that was not a generic implementation. Regards, Sonika From: Rodrigo Vivi [mailto:rodrigo.vivi@gmail.com] Sent: Thursday, September 10, 2015 12:51 AM To: Jindal, Sonika; Daniel Vetter Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 5/6] drm/i915: drm/i915: Process hpd only for hdmi inside hotplug_work_func I was also going to say that I believe this breaks DP hotplug, but this was agreed already... But I also don't understand how hot_plug is called twice since it calls encoder->hot_plug... and dp has no ->hot_plug... And also if this is happening how this code that checks for connector would help... there is something else strange happening... On Sat, Sep 5, 2015 at 9:31 PM Jindal, Sonika <sonika.jindal@intel.com<mailto:sonika.jindal@intel.com>> wrote: On 9/4/2015 8:17 PM, Daniel Vetter wrote: > On Fri, Sep 04, 2015 at 06:56:15PM +0530, Sonika Jindal wrote: >> If the same port is enumerated as hdmi as well as DP, this will get >> called for DP connector as well which is not required because >> i915_hotplug_work_func is solely to handle hdmi HPD. >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com<mailto:sonika.jindal@intel.com>> >> --- >> drivers/gpu/drm/i915/intel_hotplug.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c >> index 53c0173..8e1c43e 100644 >> --- a/drivers/gpu/drm/i915/intel_hotplug.c >> +++ b/drivers/gpu/drm/i915/intel_hotplug.c >> @@ -325,7 +325,8 @@ static void i915_hotplug_work_func(struct work_struct *work) >> >> list_for_each_entry(connector, &mode_config->connector_list, head) { >> intel_connector = to_intel_connector(connector); >> - if (!intel_connector->encoder) >> + if (!intel_connector->encoder >> + && connector->connector_type != DRM_MODE_CONNECTOR_HDMIA) >> continue; > > Pretty sure this breaks hotplug detection for everything but HDMI (since > now nothing but hdmi gets called it's ->detect function, and only if that > signals a status change will we send out an uevent to userspace). Does > this really not break DP hotplug? > > Yes we probe both hdmi and DP always, and probably we could be more > intelligent with the fallback between the too. But this here doesn't seem > to be the right solution. > -Daniel Hmm :( This doesn't allow detect to be called for dp. I missed the part that hpd_pulse only handles mst. From the name and comments it looks like it should handle hpd for dp completely. I think this this code needs some refactoring. For now, I think I better move this check to intel_encoder->hot_plug function because for the connectors with dp and hdmi both, this hot_plug hook gets called twice. > >> intel_encoder = intel_connector->encoder; >> if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org<mailto:Intel-gfx@lists.freedesktop.org> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org<mailto:Intel-gfx@lists.freedesktop.org> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c index 53c0173..8e1c43e 100644 --- a/drivers/gpu/drm/i915/intel_hotplug.c +++ b/drivers/gpu/drm/i915/intel_hotplug.c @@ -325,7 +325,8 @@ static void i915_hotplug_work_func(struct work_struct *work) list_for_each_entry(connector, &mode_config->connector_list, head) { intel_connector = to_intel_connector(connector); - if (!intel_connector->encoder) + if (!intel_connector->encoder + && connector->connector_type != DRM_MODE_CONNECTOR_HDMIA) continue; intel_encoder = intel_connector->encoder; if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
If the same port is enumerated as hdmi as well as DP, this will get called for DP connector as well which is not required because i915_hotplug_work_func is solely to handle hdmi HPD. Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/intel_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)