Re: [PATCH 5/6] drm/i915: drm/i915: Process hpd only for hdmi inside hotplug_work_func

From: "Jindal, Sonika" <sonika.jindal@intel.com>
To: Rodrigo Vivi <rodrigo.vivi@gmail.com>, Daniel Vetter <daniel@ffwll.ch>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 5/6] drm/i915: drm/i915: Process hpd only for hdmi inside hotplug_work_func
Date: Wed, 9 Sep 2015 23:37:23 +0000	[thread overview]
Message-ID: <000C66961D35964B9714611E548C10AD0C2C891B@BGSMSX104.gar.corp.intel.com> (raw)
In-Reply-To: <CABVU7+u710rGCEfn4wqUUweOSgxUPmKs8uxuo+bH9MfzWhHUpw@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 4250 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 9468 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx