[Intel-gfx] [PATCH] drm/i915: Call encoder hot_plug hook only for hdmi

[Jindal, Sonika]

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Wednesday, September 9, 2015 8:48 PM
To: Jindal, Sonika
Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org; Sharma, Shashank
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Call encoder hot_plug hook only for hdmi

On Tue, Sep 08, 2015 at 05:08:02PM +0530, Jindal, Sonika wrote:
> 
> 
> On 9/8/2015 10:12 AM, Jindal, Sonika wrote:
> >
> >
> >On 9/7/2015 9:56 PM, Daniel Vetter wrote:
> >>On Mon, Sep 07, 2015 at 10:34:34AM +0530, Sonika Jindal wrote:
> >>>If the same port is enumerated as hdmi as well as DP, this will 
> >>>call hot_plug hook for DP as well which is not required here.
> >>>This splitting of edid read and detect is done only for HDMI with 
> >>>this series.
> >>>
> >>>v2: Avoid breaking DP hpd. Also corrected the commit message and 
> >>>description accordingly. (Daniel)
> >>>
> >>>Signed-off-by: Sonika Jindal <sonika.jindal at 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..ff4692a 100644
> >>>--- a/drivers/gpu/drm/i915/intel_hotplug.c
> >>>+++ b/drivers/gpu/drm/i915/intel_hotplug.c
> >>>@@ -331,7 +331,8 @@ static void i915_hotplug_work_func(struct 
> >>>work_struct *work)
> >>>          if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> >>>              DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug 
> >>>event.\n",
> >>>                        connector->name, intel_encoder->hpd_pin);
> >>>-            if (intel_encoder->hot_plug)
> >>>+            if (intel_encoder->hot_plug
> >>>+                && connector->connector_type ==
> >>>DRM_MODE_CONNECTOR_HDMIA)
> >>
> >>Please use something like grep to find all the other ->hot_plug 
> >>implementations and then please tell me why you don't break them all.
> >>-Daniel
> >>
> >Hmm, I only checked for hot_plug for DP/edp which is not there. 
> >Failed to notice that there is one in intel_sdvo.c.
> >My mistake. I will place it properly somewhere else.
> >
> >Regards,
> >Sonika
> 
> Is there any suggestion about how we can differentiate if it is actual 
> DP or HDMI hotplug at this point? intel_encoder's type gets updated 
> after detect call. So, not sure how to have this kind of check.
> 
> For now, I think we can abandon this patch from this series.

No, hpd is shared between hdmi and dp at the hw level so we can't differentiate. Long term my idea would be that we merge together all the hdmi _and_ dp hpd handling into one overall control-flow. Then we can make sure to not call anything twice and also have sensible high-level flow (like first checking for dp vs. hdmi and then taking relevant paths).

For dealing with ->hot_plug a quick fix might be to have a separate loop going over encoders (to make sure we only call it once per encoder if there's more than one connector for 1 encoder). That behaviour would also be ok for sdvo.

<Sonika> Hmm, so instead of relying on connector, we can check for the hpd_pin on encoder and remove the connector loop completely?

-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch