| Message ID | 1441602274-2439-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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 > intel_encoder->hot_plug(intel_encoder); > if (intel_hpd_irq_event(dev, connector)) > changed = true; > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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 >> intel_encoder->hot_plug(intel_encoder); >> if (intel_hpd_irq_event(dev, connector)) >> changed = true; >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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@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. Regards, Sonika >>> intel_encoder->hot_plug(intel_encoder); >>> if (intel_hpd_irq_event(dev, connector)) >>> changed = true; >>> -- >>> 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
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@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. -Daniel
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, September 9, 2015 8:48 PM To: Jindal, Sonika Cc: Daniel Vetter; intel-gfx@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@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
On Thu, Sep 10, 2015 at 01:07:18AM +0000, Jindal, Sonika wrote: > > > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Wednesday, September 9, 2015 8:48 PM > To: Jindal, Sonika > Cc: Daniel Vetter; intel-gfx@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@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? That would be my suggestion, but I didn't check the details. -Daniel
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) intel_encoder->hot_plug(intel_encoder); if (intel_hpd_irq_event(dev, connector)) changed = true;
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@intel.com> --- drivers/gpu/drm/i915/intel_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)