diff mbox

[v3,05/33] drm: Pass the drm_dp_aux->hw_mutex to i2c for its locking

Message ID 1464964636-3877-6-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson June 3, 2016, 2:36 p.m. UTC 
Rather than have both drm_dp_aux lock within its transfer, and i2c to
lock around the transfer, use the same lock by filling in the locking
callbacks that i2c wants to use. We require our own hw_mutex as we
bypass i2c_transfer for drm_dp_dpcd_access().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä June 3, 2016, 3:11 p.m. UTC | #1 
On Fri, Jun 03, 2016 at 03:36:48PM +0100, Chris Wilson wrote:
> Rather than have both drm_dp_aux lock within its transfer, and i2c to
> lock around the transfer, use the same lock by filling in the locking
> callbacks that i2c wants to use. We require our own hw_mutex as we
> bypass i2c_transfer for drm_dp_dpcd_access().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eeaf5a7c3aa7..4b088afa21b2 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -708,8 +708,6 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  
>  	memset(&msg, 0, sizeof(msg));
>  
> -	mutex_lock(&aux->hw_mutex);
> -
>  	for (i = 0; i < num; i++) {
>  		msg.address = msgs[i].addr;
>  		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> @@ -764,8 +762,6 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  	msg.size = 0;
>  	(void)drm_dp_i2c_do_msg(aux, &msg);
>  
> -	mutex_unlock(&aux->hw_mutex);
> -
>  	return err;
>  }

AFAICS the only functional difference will be that we'll hold the lock
across the retries performed by the core. Looks like we set the retry
count to 3 for whatever reason, but I'm not seeing that we'd ever
actually return -EAGAIN so I guess the core will neever retry.
Oh, actually if we ever end up in the trylock path
(in_atomic||irqs_disabled) then the core would retry. But I don't think
we should ever do that.

Patch seems OK to me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
> @@ -774,6 +770,26 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
>  	.master_xfer = drm_dp_i2c_xfer,
>  };
>  
> +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c)
> +{
> +	return container_of(i2c, struct drm_dp_aux, ddc);
> +}
> +
> +static void lock_bus(struct i2c_adapter *i2c, unsigned int flags)
> +{
> +	mutex_lock(&i2c_to_aux(i2c)->hw_mutex);
> +}
> +
> +static int trylock_bus(struct i2c_adapter *i2c, unsigned int flags)
> +{
> +	return mutex_trylock(&i2c_to_aux(i2c)->hw_mutex);
> +}
> +
> +static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
> +{
> +	mutex_unlock(&i2c_to_aux(i2c)->hw_mutex);
> +}
> +
>  /**
>   * drm_dp_aux_register() - initialise and register aux channel
>   * @aux: DisplayPort AUX channel
> @@ -790,6 +806,10 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  	aux->ddc.algo_data = aux;
>  	aux->ddc.retries = 3;
>  
> +	aux->ddc.lock_bus = lock_bus;
> +	aux->ddc.trylock_bus = trylock_bus;
> +	aux->ddc.unlock_bus = unlock_bus;
> +
>  	aux->ddc.class = I2C_CLASS_DDC;
>  	aux->ddc.owner = THIS_MODULE;
>  	aux->ddc.dev.parent = aux->dev;
> -- 
> 2.8.1
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index eeaf5a7c3aa7..4b088afa21b2 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -708,8 +708,6 @@  static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 
 	memset(&msg, 0, sizeof(msg));
 
-	mutex_lock(&aux->hw_mutex);
-
 	for (i = 0; i < num; i++) {
 		msg.address = msgs[i].addr;
 		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
@@ -764,8 +762,6 @@  static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 	msg.size = 0;
 	(void)drm_dp_i2c_do_msg(aux, &msg);
 
-	mutex_unlock(&aux->hw_mutex);
-
 	return err;
 }
 
@@ -774,6 +770,26 @@  static const struct i2c_algorithm drm_dp_i2c_algo = {
 	.master_xfer = drm_dp_i2c_xfer,
 };
 
+static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c)
+{
+	return container_of(i2c, struct drm_dp_aux, ddc);
+}
+
+static void lock_bus(struct i2c_adapter *i2c, unsigned int flags)
+{
+	mutex_lock(&i2c_to_aux(i2c)->hw_mutex);
+}
+
+static int trylock_bus(struct i2c_adapter *i2c, unsigned int flags)
+{
+	return mutex_trylock(&i2c_to_aux(i2c)->hw_mutex);
+}
+
+static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
+{
+	mutex_unlock(&i2c_to_aux(i2c)->hw_mutex);
+}
+
 /**
  * drm_dp_aux_register() - initialise and register aux channel
  * @aux: DisplayPort AUX channel
@@ -790,6 +806,10 @@  int drm_dp_aux_register(struct drm_dp_aux *aux)
 	aux->ddc.algo_data = aux;
 	aux->ddc.retries = 3;
 
+	aux->ddc.lock_bus = lock_bus;
+	aux->ddc.trylock_bus = trylock_bus;
+	aux->ddc.unlock_bus = unlock_bus;
+
 	aux->ddc.class = I2C_CLASS_DDC;
 	aux->ddc.owner = THIS_MODULE;
 	aux->ddc.dev.parent = aux->dev;