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

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx