Re: [PATCH 1/2] drm/i915: Enable scanline read for gen9 dsi

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 19, 2017 at 08:49:21AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Maarten Lankhorst
> >Sent: Tuesday, September 19, 2017 12:56 PM
> >To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Enable scanline read for gen9 dsi
> >
> >Op 18-09-17 om 16:24 schreef Ville Syrjälä:
> >> On Mon, Sep 18, 2017 at 03:57:38PM +0200, Maarten Lankhorst wrote:
> >>> Op 18-09-17 om 15:32 schreef Vidya Srinivas:
> >>>> From: Uma Shankar <uma.shankar@intel.com>
<snip>
> >>>> +	WARN_ON(!crtc_vtotal);
> >>>> +	if (!crtc_vtotal)
> >>>> +		return scanline;
> >>>> +
> >>>> +	/* To avoid the race condition where we might cross into the
> >>>> +	 * next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR
> >>>> +	 * reads. We make sure we read PIPE_FRMTMSTMP and
> >TIMESTAMP_CTR
> >>>> +	 * during the same frame.
> >>>> +	 */
> >>>> +	do {
> >>>> +		/*
> >>>> +		 * This field provides read back of the display
> >>>> +		 * pipe frame time stamp. The time stamp value
> >>>> +		 * is sampled at every start of vertical blank.
> >>>> +		 */
> >>>> +		scan_prev_time = I915_READ_FW(PIPE_FRMTMSTMP(crtc-
> >>pipe));
> >>>> +
> >>>> +		/*
> >>>> +		 * The TIMESTAMP_CTR register has the current
> >>>> +		 * time stamp value.
> >>>> +		 */
> >>>> +		scan_curr_time = I915_READ_FW(GEN7_TIMESTAMP_CTR);
> >>>> +
> >>>> +		scan_post_time = I915_READ_FW(PIPE_FRMTMSTMP(crtc-
> >>pipe));
> >>>> +	} while (scan_post_time != scan_prev_time);
> >>>> +
> >>>> +	/*
> >>>> +	 * Since the register is 32 bit and the values
> >>>> +	 * can overflow and wrap around, making sure
> >>>> +	 * current time accounts for the register
> >>>> +	 * wrap
> >>>> +	 */
> >>>> +	if (scan_curr_time < scan_prev_time)
> >>>> +		scan_curr_time += 0x100000000;
> >>>> +
> >>>> +	scanline = div_u64(mul_u64_u32_shr((scan_curr_time -
> >>>> +scan_prev_time),
> >>> Isn't mul_u64_u32_div exactly what you want here?
> >> I think we'd actually want a mul_u32_u32_div(). But that doesn't seem
> >> to exist.
> >Yeah, couldn't find it either. :(
> >
> >Why do we even use the macros, can't we simply do the multiplications and
> >divide without?
> >i915 is only on x86 anyway. :)
> 
> mul_u32_u32 handles it differently using x86 assembly instructions (arch/x86/include/asm/div64.h).
> I guess it's more precise than normal u32*u32 multiplication.  

It's a workaround for modern gcc which (for whatever reason) can no
longer figure out that on a 32bit machine we can do this witha a 
normal 32x32->64 multiplication rather than a full blown 64 bit
multiplication, which would be more expensive.

Earlier gcc versions did work this out correctly without handholding,
but somehow they broke it at some point.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx