[Intel-gfx] [PATCH 10/12] drm/i915/skl+: use linetime latency if ddb size is not available
Mahesh Kumar
mahesh1.kumar at intel.com
Tue May 16 09:46:48 UTC 2017
Hi,
On Tuesday 16 May 2017 04:08 AM, Matt Roper wrote:
> On Mon, May 15, 2017 at 02:04:35PM +0530, Mahesh Kumar wrote:
>> From: "Kumar, Mahesh" <mahesh1.kumar at intel.com>
>>
>> This patch make changes to use linetime latency if allocated
>> DDB size during plane watermark calculation is not available,
>> This is required to implement new DDB allocation algorithm.
>>
>> In New Algorithm DDB is allocated based on WM values, because of which
>> number of DDB blocks will not be available during WM calculation,
>> So this "linetime latency" is suggested by SV/HW team to be used during
>> switch-case for WM blocks selection.
> (Just realized I never actually sent my review of this patch on the
> previous series review; sorry for the delay.)
>
> I'm not sure if the term "linetime" is completely self-explanatory. You
> might want to explicitly clarify that it's the time it takes to fill a
> single row at a given clock rate. So "linetime latency" here is defined
> as "line time in microseconds = pipe horizontal total number of pixels /
> pixel rate in MHz."
Will update commit message.
>
>> Changes since v1:
>> - Rebase on top of Paulo's patch series
>> Changes since v2:
>> - Fix if-else condition (pointed by Maarten)
>> Changes since v3:
>> - Use common function for timetime_us calculation (Paulo)
>> - rebase on drm-tip
>> Changes since v4:
>> - Use consistent name for fixed_point operation
>>
>> Signed-off-by: "Mahesh Kumar" <mahesh1.kumar at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
>> drivers/gpu/drm/i915/intel_pm.c | 42 ++++++++++++++++++++++++++++++++---------
>> 2 files changed, 40 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a1858c3eb33a..84052990e695 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -115,6 +115,13 @@ typedef struct {
>> fp; \
>> })
>>
>> +static inline bool is_fixed16_zero(uint_fixed_16_16_t val)
>> +{
>> + if (val.val == 0)
>> + return true;
>> + return false;
>> +}
>> +
>> static inline uint_fixed_16_16_t u32_to_fixed_16_16(uint32_t val)
>> {
>> uint_fixed_16_16_t fp;
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 0f1d9f672e83..d73369c2c2d9 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4197,6 +4197,27 @@ static uint_fixed_16_16_t skl_wm_method2(uint32_t pixel_rate,
>> return ret;
>> }
>>
>> +static uint_fixed_16_16_t
>> +skl_get_linetime_us(struct intel_crtc_state *cstate)
> We use the same definition of linetime for all platforms that use the
> concept of 'linetime' (hsw+). So maybe the skl_ prefix here isn't
> really appropriate?
will make it intel_get_linetime_us.
> For consistency, should we also call this new function when calculating
> the HSW/BDW watermarks in hsw_compute_linetime_wm()? Or consolidate
> skl_compute_linetime_wm() with hsw_compute_linetime_wm()? Potentially
> something we could do as a follow-up patch if we don't want to deal with
> it while getting this series in.
sounds good :), will create a new patch to use intel_get_linetime_us in
hsw_compute_linetime_wm.
hsw & skl require bunch of other calculations/WA in linetime_wm, so will
keep them separate function only.
>
>> +{
>> + uint32_t pixel_rate;
>> + uint32_t crtc_htotal;
>> + uint_fixed_16_16_t linetime_us;
>> +
>> + if (!cstate->base.active)
>> + return u32_to_fixed_16_16(0);
>> +
>> + pixel_rate = cstate->pixel_rate;
>> +
>> + if (WARN_ON(pixel_rate == 0))
>> + return u32_to_fixed_16_16(0);
>> +
>> + crtc_htotal = cstate->base.adjusted_mode.crtc_htotal;
>> + linetime_us = fixed_16_16_div_u64(crtc_htotal * 1000, pixel_rate);
>> +
>> + return linetime_us;
>> +}
>> +
>> static uint32_t
>> skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cstate,
>> const struct intel_plane_state *pstate)
>> @@ -4331,12 +4352,18 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>> if (y_tiled) {
>> selected_result = max_fixed_16_16(method2, y_tile_minimum);
>> } else {
>> + uint32_t linetime_us;
>> +
>> + linetime_us = fixed_16_16_to_u32_round_up(
>> + skl_get_linetime_us(cstate));
>> if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
>> (plane_bytes_per_line / 512 < 1))
>> selected_result = method2;
>> - else if ((ddb_allocation /
>> + else if ((ddb_allocation && ddb_allocation /
>> fixed_16_16_to_u32_round_up(plane_blocks_per_line)) >= 1)
>> selected_result = min_fixed_16_16(method1, method2);
>> + else if (latency >= linetime_us)
>> + selected_result = method2;
> Based on the commit message, I'm a little bit confused. The plan is to
> switch to the new algorithm where we calculate watermarks first and then
> partition DDB second. So at the end, ddb_allocation should never be
> available here, right? In that case, can we just remove it as a
> parameter to this function and drop your first 'else if' branch here?
> That would make us take the new decision logic immediately and give us
> an earlier idea whether it really behaves the way we expect it to,
> rather than waiting until we've made a bunch of other changes too.
right, We will eventually remove first "else if" check in next patch
itself :)
I kept it in this patch due to earlier comment by Paulo not to remove
(ddb_allocation / plane blocks per line)
check until patch which removes the ddb_allocation is merged/submitted.
https://patchwork.freedesktop.org/patch/115537/
>
> It's also not clear to me why with the old algorithm, the DDB allocation
> test being true meant "use the lower of the two watermark methods"
> whereas the linetime test means "aways use method2." Is there any more
> explanation available for how/why the linetime latency plays into the
> watermark method decision? The new decision isn't reflected in the
> bspec yet (that I can see), so it's kind of hard to review that we're
> definitely doing the right thing.
Thanks alot for catching this, I revisited the BSpec & it says to use
minimum of method1, method2.
BTW these changes are already included in internal Bspec, After new
version release it should be part of open BSpec.
I don't know about release process & timeline though :P
-Mahesh
>
>
> Matt
>
>> else
>> selected_result = method1;
>> }
>> @@ -4424,19 +4451,16 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
>> {
>> struct drm_atomic_state *state = cstate->base.state;
>> struct drm_i915_private *dev_priv = to_i915(state->dev);
>> - uint32_t pixel_rate;
>> + uint_fixed_16_16_t linetime_us;
>> uint32_t linetime_wm;
>>
>> - if (!cstate->base.active)
>> - return 0;
>> + linetime_us = skl_get_linetime_us(cstate);
>>
>> - pixel_rate = cstate->pixel_rate;
>> -
>> - if (WARN_ON(pixel_rate == 0))
>> + if (is_fixed16_zero(linetime_us))
>> return 0;
>>
>> - linetime_wm = DIV_ROUND_UP(8 * cstate->base.adjusted_mode.crtc_htotal *
>> - 1000, pixel_rate);
>> + linetime_wm = fixed_16_16_to_u32_round_up(mul_u32_fixed_16_16(8,
>> + linetime_us));
>>
>> /* Display WA #1135: bxt. */
>> if (IS_BROXTON(dev_priv) && dev_priv->ipc_enabled)
>> --
>> 2.11.0
>>
More information about the Intel-gfx
mailing list