[Intel-gfx] [PATCH 2/5] drm/i915/irq: abstract irq storm hotplug disabling
On 22 June 2015 at 23:02, Daniel Vetter wrote:
> On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote:
>> Continue abstracting hotplug storm related functions to clarify the
>> code. This time, abstract hotplug irq storm related hotplug
>> disabling. While at it, clean up the loop iterating over connectors for
>> readability.
>>
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 77
>> ++---
>> 1 file changed, 50 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index d64d6895a2e5..bf4c15d0ea2b 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct
>> *work)
>> /*
>> * Handle hotplug events outside the interrupt handler proper.
>> */
>> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
>> +static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
>>
>> static void i915_hotplug_work_func(struct work_struct *work)
>> {
>> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct
>> *work)
>> struct intel_connector *intel_connector;
>> struct intel_encoder *intel_encoder;
>> struct drm_connector *connector;
>> - bool hpd_disabled = false;
>> bool changed = false;
>> u32 hpd_event_bits;
>>
>> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct
>> *work)
>>
>> hpd_event_bits = dev_priv->hotplug.event_bits;
>> dev_priv->hotplug.event_bits = 0;
>> - list_for_each_entry(connector, _config->connector_list, head) {
>
> Random comment: We have piles of connector_list walking in probe codde,
> and DP MST adds/removes them without much thought really users of these.
> Dave? Do we need a connector_list spinlock?
>
> Just grabbing one of the modeset locks won't cut it I think since it'll
> serialize way too much.
In my tree currently this code grabs mode_config->mutex, so it should
be fine wrt
the connector list disappearing, as removing connectors is protected
my mode_config->mutex
as well.
if we do add a spinlock, we need to be careful about dropping it and restarting
the loops etc, as I would guess there are a fair few paths we don't
want to descend
holding the spin lock, due to possible sleeping/scheduling etc.
I think the intel_hpd_irq_reenable_work is the one that stands out to
me, the short
hotplug paths don't traverse connector lists deliberately to avoid the problem.
Dave.
[Intel-gfx] [PATCH 2/5] drm/i915/irq: abstract irq storm hotplug disabling
On Tue, Jun 23, 2015 at 10:29:27AM +1000, Dave Airlie wrote:
> On 22 June 2015 at 23:02, Daniel Vetter wrote:
> > On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote:
> >> Continue abstracting hotplug storm related functions to clarify the
> >> code. This time, abstract hotplug irq storm related hotplug
> >> disabling. While at it, clean up the loop iterating over connectors for
> >> readability.
> >>
> >> Signed-off-by: Jani Nikula
> >> ---
> >> drivers/gpu/drm/i915/i915_irq.c | 77
> >> ++---
> >> 1 file changed, 50 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >> b/drivers/gpu/drm/i915/i915_irq.c
> >> index d64d6895a2e5..bf4c15d0ea2b 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct
> >> *work)
> >> /*
> >> * Handle hotplug events outside the interrupt handler proper.
> >> */
> >> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
> >> +static void intel_hpd_irq_storm_disable(struct drm_i915_private
> >> *dev_priv);
> >>
> >> static void i915_hotplug_work_func(struct work_struct *work)
> >> {
> >> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct
> >> *work)
> >> struct intel_connector *intel_connector;
> >> struct intel_encoder *intel_encoder;
> >> struct drm_connector *connector;
> >> - bool hpd_disabled = false;
> >> bool changed = false;
> >> u32 hpd_event_bits;
> >>
> >> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct
> >> *work)
> >>
> >> hpd_event_bits = dev_priv->hotplug.event_bits;
> >> dev_priv->hotplug.event_bits = 0;
> >> - list_for_each_entry(connector, _config->connector_list, head) {
> >
> > Random comment: We have piles of connector_list walking in probe codde,
> > and DP MST adds/removes them without much thought really users of these.
> > Dave? Do we need a connector_list spinlock?
> >
> > Just grabbing one of the modeset locks won't cut it I think since it'll
> > serialize way too much.
>
> In my tree currently this code grabs mode_config->mutex, so it should
> be fine wrt
> the connector list disappearing, as removing connectors is protected
> my mode_config->mutex
> as well.
Hm right I mixed things up and didn't realize this is protected by the
mode_config->mutex.
> if we do add a spinlock, we need to be careful about dropping it and
> restarting
> the loops etc, as I would guess there are a fair few paths we don't
> want to descend
> holding the spin lock, due to possible sleeping/scheduling etc.
>
> I think the intel_hpd_irq_reenable_work is the one that stands out to
> me, the short
> hotplug paths don't traverse connector lists deliberately to avoid the
> problem.
I think grabbing the mode_config->mutex in there should be fine. Looks
like we can stretch just grabbing mode_config->mutex for a bit longer ;-)
Jani, can you perhaps throw this in with your next round of hpd cleanup?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Intel-gfx] [PATCH 2/5] drm/i915/irq: abstract irq storm hotplug disabling
On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote:
> Continue abstracting hotplug storm related functions to clarify the
> code. This time, abstract hotplug irq storm related hotplug
> disabling. While at it, clean up the loop iterating over connectors for
> readability.
>
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/i915_irq.c | 77
> ++---
> 1 file changed, 50 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d64d6895a2e5..bf4c15d0ea2b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct
> *work)
> /*
> * Handle hotplug events outside the interrupt handler proper.
> */
> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
> +static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
>
> static void i915_hotplug_work_func(struct work_struct *work)
> {
> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct
> *work)
> struct intel_connector *intel_connector;
> struct intel_encoder *intel_encoder;
> struct drm_connector *connector;
> - bool hpd_disabled = false;
> bool changed = false;
> u32 hpd_event_bits;
>
> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct
> *work)
>
> hpd_event_bits = dev_priv->hotplug.event_bits;
> dev_priv->hotplug.event_bits = 0;
> - list_for_each_entry(connector, _config->connector_list, head) {
Random comment: We have piles of connector_list walking in probe codde,
and DP MST adds/removes them without much thought really users of these.
Dave? Do we need a connector_list spinlock?
Just grabbing one of the modeset locks won't cut it I think since it'll
serialize way too much.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
