All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v2 2/3] drm/i915: Nuke aux regs from intel_dp
Date: Thu, 22 Feb 2018 07:16:07 +0000	[thread overview]
Message-ID: <1519285174.2358.8.camel@dk-H97M-D3H> (raw)
In-Reply-To: <20180220190053.2895-1-ville.syrjala@linux.intel.com>


On Tue, 2018-02-20 at 21:00 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Just store function pointers that give us the correct register offsets
> instead of storing the register offsets themselves. Slightly less
> efficient perhaps but saves a few bytes and better matches how we do
> things elsewhere.
> 
> v2: Keep a local array of data registers (Chris)
> 
Intriguing, why bother storing register offsets in one go if it's okay
to make these additional function calls for every aux transaction.

What am I missing here?



> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 85 ++++++++++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_drv.h |  5 ++-
>  2 files changed, 45 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eeb8a026fd08..b0c273b5b2a9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -936,7 +936,7 @@ static uint32_t
>  intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>  	uint32_t status;
>  	bool done;
>  
> @@ -1089,7 +1089,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv =
>  			to_i915(intel_dig_port->base.base.dev);
> -	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> +	i915_reg_t ch_ctl, ch_data[5];
>  	uint32_t aux_clock_divider;
>  	int i, ret, recv_bytes;
>  	uint32_t status;
> @@ -1097,6 +1097,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
>  	bool vdd;
>  
> +	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> +	for (i = 0; i < ARRAY_SIZE(ch_data); i++)
> +		     ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
> +
>  	pps_lock(intel_dp);
>  
>  	/*
> @@ -1154,7 +1158,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		for (try = 0; try < 5; try++) {
>  			/* Load the send data into the aux channel data registers */
>  			for (i = 0; i < send_bytes; i += 4)
> -				I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> +				I915_WRITE(ch_data[i >> 2],
>  					   intel_dp_pack_aux(send + i,
>  							     send_bytes - i));
>  
> @@ -1239,7 +1243,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		recv_bytes = recv_size;
>  
>  	for (i = 0; i < recv_bytes; i += 4)
> -		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >> 2]),
> +		intel_dp_unpack_aux(I915_READ(ch_data[i >> 2]),
>  				    recv + i, recv_bytes - i);
>  
>  	ret = recv_bytes;
> @@ -1396,9 +1400,11 @@ intel_aux_power_domain(struct intel_dp *intel_dp)
>  	}
>  }
>  
> -static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				  enum aux_ch aux_ch)
> +static i915_reg_t g4x_aux_ctl_reg(struct intel_dp *intel_dp)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_B:
>  	case AUX_CH_C:
> @@ -1410,9 +1416,11 @@ static i915_reg_t g4x_aux_ctl_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
> -				   enum aux_ch aux_ch, int index)
> +static i915_reg_t g4x_aux_data_reg(struct intel_dp *intel_dp, int index)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_B:
>  	case AUX_CH_C:
> @@ -1424,9 +1432,11 @@ static i915_reg_t g4x_aux_data_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				  enum aux_ch aux_ch)
> +static i915_reg_t ilk_aux_ctl_reg(struct intel_dp *intel_dp)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  		return DP_AUX_CH_CTL(aux_ch);
> @@ -1440,9 +1450,11 @@ static i915_reg_t ilk_aux_ctl_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
> -				   enum aux_ch aux_ch, int index)
> +static i915_reg_t ilk_aux_data_reg(struct intel_dp *intel_dp, int index)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  		return DP_AUX_CH_DATA(aux_ch, index);
> @@ -1456,9 +1468,11 @@ static i915_reg_t ilk_aux_data_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				  enum aux_ch aux_ch)
> +static i915_reg_t skl_aux_ctl_reg(struct intel_dp *intel_dp)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  	case AUX_CH_B:
> @@ -1472,9 +1486,11 @@ static i915_reg_t skl_aux_ctl_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
> -				   enum aux_ch aux_ch, int index)
> +static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
>  	switch (aux_ch) {
>  	case AUX_CH_A:
>  	case AUX_CH_B:
> @@ -1488,37 +1504,20 @@ static i915_reg_t skl_aux_data_reg(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static i915_reg_t intel_aux_ctl_reg(struct drm_i915_private *dev_priv,
> -				    enum aux_ch aux_ch)
> -{
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		return skl_aux_ctl_reg(dev_priv, aux_ch);
> -	else if (HAS_PCH_SPLIT(dev_priv))
> -		return ilk_aux_ctl_reg(dev_priv, aux_ch);
> -	else
> -		return g4x_aux_ctl_reg(dev_priv, aux_ch);
> -}
> -
> -static i915_reg_t intel_aux_data_reg(struct drm_i915_private *dev_priv,
> -				     enum aux_ch aux_ch, int index)
> -{
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		return skl_aux_data_reg(dev_priv, aux_ch, index);
> -	else if (HAS_PCH_SPLIT(dev_priv))
> -		return ilk_aux_data_reg(dev_priv, aux_ch, index);
> -	else
> -		return g4x_aux_data_reg(dev_priv, aux_ch, index);
> -}
> -
>  static void intel_aux_reg_init(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> -	enum aux_ch aux_ch = intel_dp->aux_ch;
> -	int i;
>  
> -	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, aux_ch);
> -	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> -		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, aux_ch, i);
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
> +		intel_dp->aux_ch_data_reg = skl_aux_data_reg;
> +	} else if (HAS_PCH_SPLIT(dev_priv)) {
> +		intel_dp->aux_ch_ctl_reg = ilk_aux_ctl_reg;
> +		intel_dp->aux_ch_data_reg = ilk_aux_data_reg;
> +	} else {
> +		intel_dp->aux_ch_ctl_reg = g4x_aux_ctl_reg;
> +		intel_dp->aux_ch_data_reg = g4x_aux_data_reg;
> +	}
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7f6a7f592fe6..a72820fe5262 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1038,8 +1038,6 @@ struct intel_dp_compliance {
>  
>  struct intel_dp {
>  	i915_reg_t output_reg;
> -	i915_reg_t aux_ch_ctl_reg;
> -	i915_reg_t aux_ch_data_reg[5];
>  	uint32_t DP;
>  	int link_rate;
>  	uint8_t lane_count;
> @@ -1124,6 +1122,9 @@ struct intel_dp {
>  				     int send_bytes,
>  				     uint32_t aux_clock_divider);
>  
> +	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> +	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
> +
>  	/* This is called before a link training is starterd */
>  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2018-02-22  7:16 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-20 17:05 [PATCH 1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Ville Syrjala
2018-02-20 17:05 ` [PATCH 2/3] drm/i915: Nuke aux regs from intel_dp Ville Syrjala
2018-02-20 17:30   ` Chris Wilson
2018-02-20 17:49     ` Ville Syrjälä
2018-02-20 17:57       ` Chris Wilson
2018-02-20 19:00   ` [PATCH v2 " Ville Syrjala
2018-02-22  7:16     ` Pandiyan, Dhinakaran [this message]
2018-02-22 12:43       ` Ville Syrjälä
2018-02-22 20:38         ` Pandiyan, Dhinakaran
2018-02-20 17:05 ` [PATCH 3/3] drm/i915: Collect aux ch vfunc setup into intel_dp_aux_init() Ville Syrjala
2018-02-20 17:25   ` Chris Wilson
2018-02-20 19:35   ` Rodrigo Vivi
2018-02-20 17:28 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Add enum aux_ch and clean up the aux init to use it Patchwork
2018-02-20 17:43 ` ✓ Fi.CI.BAT: success " Patchwork
2018-02-20 17:47 ` [PATCH 1/3] " Chris Wilson
2018-02-20 19:31 ` Rodrigo Vivi
2018-02-22  7:25   ` Pandiyan, Dhinakaran
2018-02-22 12:48     ` Ville Syrjälä
2018-02-22 16:29   ` Ville Syrjälä
2018-02-22  6:12 ` Pandiyan, Dhinakaran

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1519285174.2358.8.camel@dk-H97M-D3H \
    --to=dhinakaran.pandiyan@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.