From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Vivi Subject: Re: [PATCH 5/6] drm/i915: drm/i915: Process hpd only for hdmi inside hotplug_work_func Date: Wed, 09 Sep 2015 19:20:34 +0000 Message-ID: References: <1441373176-22302-1-git-send-email-sonika.jindal@intel.com> <1441373176-22302-6-git-send-email-sonika.jindal@intel.com> <20150904144702.GA26318@phenom.ffwll.local> <55EBC1B7.8010609@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0433583618==" Return-path: Received: from mail-io0-f179.google.com (mail-io0-f179.google.com [209.85.223.179]) by gabe.freedesktop.org (Postfix) with ESMTPS id 715846E28B for ; Wed, 9 Sep 2015 12:20:44 -0700 (PDT) Received: by ioiz6 with SMTP id z6so34284866ioi.2 for ; Wed, 09 Sep 2015 12:20:43 -0700 (PDT) In-Reply-To: <55EBC1B7.8010609@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Jindal, Sonika" , Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0433583618== Content-Type: multipart/alternative; boundary=001a113ee5de01d959051f556147 --001a113ee5de01d959051f556147 Content-Type: text/plain; charset=UTF-8 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 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 > >> --- > >> 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 > --001a113ee5de01d959051f556147 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
I was also going to say that I believe this breaks DP hotp= lug, but this was agreed already...

But I also don't= understand how hot_plug is called twice since it calls encoder->hot_plu= g...
and dp has no ->hot_plug...=C2=A0
And also if t= his is happening how this code that checks for connector would help... ther= e is something else strange happening...=C2=A0


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 ge= t
>> 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>
>> ---
>>=C2=A0 =C2=A0drivers/gpu/drm/i915/intel_hotplug.c |=C2=A0 =C2=A0 3 = ++-
>>=C2=A0 =C2=A01 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/dr= m/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)
>>
>>=C2=A0 =C2=A0 =C2=A0 list_for_each_entry(connector, &mode_confi= g->connector_list, head) {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 intel_connector = =3D to_intel_connector(connector);
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!intel_connector-&g= t;encoder)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!intel_connector-&g= t;encoder
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 && connector->connector_type !=3D DRM_MODE_CONNECTOR_HDMI= A)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 continue;
>
> Pretty sure this breaks hotplug detection for everything but HDMI (sin= ce
> now nothing but hdmi gets called it's ->detect function, and on= ly 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.

>
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 intel_encoder =3D = intel_connector->encoder;
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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
--001a113ee5de01d959051f556147-- --===============0433583618== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============0433583618==--