[v6,3/8] drm/i915: Unify ilk and hsw .get_aux_clock_divider

Submitted by Mika Kahola on June 3, 2015, 12:45 p.m.

Details

Message ID 1433335514-4156-4-git-send-email-mika.kahola@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Mika Kahola June 3, 2015, 12:45 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

ilk_get_aux_clock_divider() is now a subset of
hsw_get_aux_clock_divider() so unify them.

v2: Rebased to the latest
v3: Rebased to the latest
v4: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9a6517d..959f115 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -704,23 +704,6 @@  static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (index)
-		return 0;
-
-	if (intel_dig_port->port == PORT_A) {
-		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
-
-	} else {
-		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
-	}
-}
-
-static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
-{
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	if (intel_dig_port->port == PORT_A) {
 		if (index)
 			return 0;
@@ -733,7 +716,9 @@  static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 		default: return 0;
 		}
 	} else  {
-		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
+		if (index)
+			return 0;
+		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
 	}
 }
 
@@ -5746,8 +5731,6 @@  intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
 	else if (IS_VALLEYVIEW(dev))
 		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
-	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
 	else if (HAS_PCH_SPLIT(dev))
 		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
 	else

Comments

On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> ilk_get_aux_clock_divider() is now a subset of
> hsw_get_aux_clock_divider() so unify them.

I do like the clarity of having these two separate, especially with the
early return in the ilk version and the w/a in the hsw/bdw version.

Moreover there's the subtle round up vs. closest difference, and a
history of aux bugs...

I'm dropping this one, doesn't seem to affect later patches.

BR,
Jani.


>
> v2: Rebased to the latest
> v3: Rebased to the latest
> v4: Fix for patch style problems
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>
> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9a6517d..959f115 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -704,23 +704,6 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (index)
> -		return 0;
> -
> -	if (intel_dig_port->port == PORT_A) {
> -		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> -
> -	} else {
> -		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> -	}
> -}
> -
> -static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> -{
> -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_device *dev = intel_dig_port->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
>  	if (intel_dig_port->port == PORT_A) {
>  		if (index)
>  			return 0;
> @@ -733,7 +716,9 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  		default: return 0;
>  		}
>  	} else  {
> -		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> +		if (index)
> +			return 0;
> +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>  	}
>  }
>  
> @@ -5746,8 +5731,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
>  	else if (IS_VALLEYVIEW(dev))
>  		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
> -	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
>  	else if (HAS_PCH_SPLIT(dev))
>  		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
>  	else
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 04, 2015 at 04:24:43PM +0300, Jani Nikula wrote:
> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > ilk_get_aux_clock_divider() is now a subset of
> > hsw_get_aux_clock_divider() so unify them.
> 
> I do like the clarity of having these two separate,

I see no clarity. Just pointless duplication which risks different
bugs in different subsets of the platforms.

> especially with the
> early return in the ilk version and the w/a in the hsw/bdw version
> Moreover there's the subtle round up vs. closest difference, and a
> history of aux bugs...

The up vs. closest makes no difference for ILK-IVB since the cdclk is
either 450 or 400 MHz. Anyway I suppose we should just change them all
to use DIV_ROUND_CLOSEST().

> 
> I'm dropping this one, doesn't seem to affect later patches.

No biggie. But I'm fairly sure I'll eventually send patches to store
the rawclk in dev_priv, at which point I'll propose killing all of
these (well, maybe settling for skl+ vs. the rest).

> 
> BR,
> Jani.
> 
> 
> >
> > v2: Rebased to the latest
> > v3: Rebased to the latest
> > v4: Fix for patch style problems
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >
> > Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
> >  1 file changed, 3 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 9a6517d..959f115 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -704,23 +704,6 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > -	if (index)
> > -		return 0;
> > -
> > -	if (intel_dig_port->port == PORT_A) {
> > -		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> > -
> > -	} else {
> > -		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> > -	}
> > -}
> > -
> > -static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> > -{
> > -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > -	struct drm_device *dev = intel_dig_port->base.base.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> >  	if (intel_dig_port->port == PORT_A) {
> >  		if (index)
> >  			return 0;
> > @@ -733,7 +716,9 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> >  		default: return 0;
> >  		}
> >  	} else  {
> > -		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> > +		if (index)
> > +			return 0;
> > +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> >  	}
> >  }
> >  
> > @@ -5746,8 +5731,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
> >  	else if (IS_VALLEYVIEW(dev))
> >  		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
> > -	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > -		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
> >  	else if (HAS_PCH_SPLIT(dev))
> >  		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
> >  	else
> > -- 
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 04, 2015 at 04:24:43PM +0300, Jani Nikula wrote:
>> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > ilk_get_aux_clock_divider() is now a subset of
>> > hsw_get_aux_clock_divider() so unify them.
>> 
>> I do like the clarity of having these two separate,
>
> I see no clarity. Just pointless duplication which risks different
> bugs in different subsets of the platforms.

I'm only replying because this is a much larger discussion than just
this patch.

The way I see it, it's currently more likely that we'll have subtle
platform specific bugs because of a bunch of if conditions in functions
combined for the sake of deduplicating 10 lines. When we add more
conditions to such combined functions, it's more likely that we
introduce bugs to platforms we didn't even intend to touch.

The ilk+ code returns early for index != 0. Hey, one subsecond glance
and you know there are no retries.

The ilk+ code does not check for a PCH ID which it doesn't ship
with. One fewer brief WTF pauses while reading the code.

When you read the code, it's most often not just these lowest level
functions. It's a bigger context, and each step here adds to a cognitive
burden you have to carry. If you have to consider more stuff, you'll
swap out something else, and it becomes harder and slower.

As a whole, this is hard stuff, and the readability has to be at a level
where you don't have to pause to wonder what's going on. It all adds up.

BR,
Jani.


>> especially with the
>> early return in the ilk version and the w/a in the hsw/bdw version
>> Moreover there's the subtle round up vs. closest difference, and a
>> history of aux bugs...
>
> The up vs. closest makes no difference for ILK-IVB since the cdclk is
> either 450 or 400 MHz. Anyway I suppose we should just change them all
> to use DIV_ROUND_CLOSEST().
>
>> 
>> I'm dropping this one, doesn't seem to affect later patches.
>
> No biggie. But I'm fairly sure I'll eventually send patches to store
> the rawclk in dev_priv, at which point I'll propose killing all of
> these (well, maybe settling for skl+ vs. the rest).
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > v2: Rebased to the latest
>> > v3: Rebased to the latest
>> > v4: Fix for patch style problems
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> >
>> > Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
>> >  1 file changed, 3 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 9a6517d..959f115 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -704,23 +704,6 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> >  
>> > -	if (index)
>> > -		return 0;
>> > -
>> > -	if (intel_dig_port->port == PORT_A) {
>> > -		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
>> > -
>> > -	} else {
>> > -		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>> > -	}
>> > -}
>> > -
>> > -static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>> > -{
>> > -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> > -	struct drm_device *dev = intel_dig_port->base.base.dev;
>> > -	struct drm_i915_private *dev_priv = dev->dev_private;
>> > -
>> >  	if (intel_dig_port->port == PORT_A) {
>> >  		if (index)
>> >  			return 0;
>> > @@ -733,7 +716,9 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>> >  		default: return 0;
>> >  		}
>> >  	} else  {
>> > -		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>> > +		if (index)
>> > +			return 0;
>> > +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>> >  	}
>> >  }
>> >  
>> > @@ -5746,8 +5731,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>> >  		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
>> >  	else if (IS_VALLEYVIEW(dev))
>> >  		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
>> > -	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>> > -		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
>> >  	else if (HAS_PCH_SPLIT(dev))
>> >  		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
>> >  	else
>> > -- 
>> > 1.9.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Ville Syrjälä
> Intel OTC