[Intel-gfx] [PATCH] drm/i915: Rework sdvo proxy i2c locking

[Chris Wilson]

Quoting Daniel Vetter (2017-07-26 07:02:44)
> lockdep complaints about a locking recursion for the i2c bus lock
> because both the sdvo ddc proxy bus and the gmbus nested within use
> the same locking class. It's not really a deadlock since we never nest
> the other way round, but it's annoying.
> 
> Fix it by pulling the gmbus locking into the i2c lock_ops for both
> i2c_adapater and making sure that the ddc_proxy_xfer function is
> entirely lockless.
> 
> Re-layouting the extracted function resulted in some whitespace
> cleanups, I figured we might as well keep them.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 0403e30dfabc..392d3d2d4bbf 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -451,23 +451,24 @@ static const char * const cmd_status_names[] = {
>         "Scaling not supported"
>  };
>  
> -static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> -                                const void *args, int args_len)
> +static bool __intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> +                                  const void *args, int args_len,
> +                                  bool locked)
>  {
>         u8 *buf, status;
>         struct i2c_msg *msgs;
>         int i, ret = true;
>  
> -        /* Would be simpler to allocate both in one go ? */        
> +       /* Would be simpler to allocate both in one go ? */
>         buf = kzalloc(args_len * 2 + 2, GFP_KERNEL);
>         if (!buf)
>                 return false;
>  
>         msgs = kcalloc(args_len + 3, sizeof(*msgs), GFP_KERNEL);
>         if (!msgs) {
> -               kfree(buf);
> +               kfree(buf);
>                 return false;
> -        }
> +       }
>  
>         intel_sdvo_debug_write(intel_sdvo, cmd, args, args_len);
>  
> @@ -498,7 +499,10 @@ static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
>         msgs[i+2].len = 1;
>         msgs[i+2].buf = &status;
>  
> -       ret = i2c_transfer(intel_sdvo->i2c, msgs, i+3);
> +       if (locked)

locked=true is called from the unlocked context, very confusing.

> +               ret = i2c_transfer(intel_sdvo->i2c, msgs, i+3);
> +       else
> +               ret = __i2c_transfer(intel_sdvo->i2c, msgs, i+3);
>         if (ret < 0) {
>                 DRM_DEBUG_KMS("I2c transfer returned %d\n", ret);
>                 ret = false;
> @@ -516,6 +520,12 @@ static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
>         return ret;
>  }
>  
> +static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd,
> +                                const void *args, int args_len)
> +{
> +       return __intel_sdvo_write_cmd(intel_sdvo, cmd, args, args_len, true);
> +}
> +
>  static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>                                      void *response, int response_len)
>  {
> @@ -602,13 +612,13 @@ static int intel_sdvo_get_pixel_multiplier(const struct drm_display_mode *adjust
>                 return 4;
>  }
>  
> -static bool intel_sdvo_set_control_bus_switch(struct intel_sdvo *intel_sdvo,
> -                                             u8 ddc_bus)
> +static bool __intel_sdvo_set_control_bus_switch(struct intel_sdvo *intel_sdvo,
> +                                               u8 ddc_bus)
>  {
>         /* This must be the immediately preceding write before the i2c xfer */
> -       return intel_sdvo_write_cmd(intel_sdvo,
> -                                   SDVO_CMD_SET_CONTROL_BUS_SWITCH,
> -                                   &ddc_bus, 1);
> +       return __intel_sdvo_write_cmd(intel_sdvo,
> +                                     SDVO_CMD_SET_CONTROL_BUS_SWITCH,
> +                                     &ddc_bus, 1, false);
>  }
>  
>  static bool intel_sdvo_set_value(struct intel_sdvo *intel_sdvo, u8 cmd, const void *data, int len)
> @@ -2934,7 +2944,7 @@ static int intel_sdvo_ddc_proxy_xfer(struct i2c_adapter *adapter,
>  {
>         struct intel_sdvo *sdvo = adapter->algo_data;
>  
> -       if (!intel_sdvo_set_control_bus_switch(sdvo, sdvo->ddc_bus))
> +       if (!__intel_sdvo_set_control_bus_switch(sdvo, sdvo->ddc_bus))
>                 return -EIO;
>  
>         return sdvo->i2c->algo->master_xfer(sdvo->i2c, msgs, num);
> @@ -2951,6 +2961,39 @@ static const struct i2c_algorithm intel_sdvo_ddc_proxy = {
>         .functionality  = intel_sdvo_ddc_proxy_func
>  };
>  
> +static void proxy_lock_bus(struct i2c_adapter *adapter,
> +                          unsigned int flags)
> +{
> +       struct intel_sdvo *sdvo = adapter->algo_data;
> +       struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
> +
> +       mutex_lock(&dev_priv->gmbus_mutex);

sdvo->i2c->lock_ops->lock_bus(sdvo->i2c, flags);


> +}
> +
> +static int proxy_trylock_bus(struct i2c_adapter *adapter,
> +                            unsigned int flags)
> +{
> +       struct intel_sdvo *sdvo = adapter->algo_data;
> +       struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
> +
> +       return mutex_trylock(&dev_priv->gmbus_mutex);

sdvo->i2c->lock_ops->trylock_bus(sdvo->i2c, flags);

> +}
> +
> +static void proxy_unlock_bus(struct i2c_adapter *adapter,
> +                            unsigned int flags)
> +{
> +       struct intel_sdvo *sdvo = adapter->algo_data;
> +       struct drm_i915_private *dev_priv = to_i915(sdvo->base.base.dev);
> +
> +       mutex_unlock(&dev_priv->gmbus_mutex);

sdvo->i2c->lock_ops->unlock_bus(sdvo->i2c, flags);

On reflection, you only need to hook up gmbus.lock_ops since gmbus is a
different lockclass it will be allowed to nest.
-Chris