[Intel-gfx] [PATCH v4 1/8] drm/i915/skl+: use linetime latency instead of ddb size

[Paulo Zanoni]

Em Qua, 2016-11-09 às 20:28 +0530, Mahesh Kumar escreveu:
> Hi,
> 
> 
> On Monday 31 October 2016 11:33 PM, Paulo Zanoni wrote:
> > 
> > Em Qui, 2016-10-13 às 16:28 +0530, Kumar, Mahesh escreveu:
> > > 
> > > This patch make changes to use linetime latency instead of
> > > allocated
> > > DDB size during plane watermark calculation in switch case, 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 use
> > > during
> > > switch-case for WM blocks selection.
> > > 
> > > Changes since v1:
> > >   - Rebase on top of Paulo's patch series
> > > Changes since v2:
> > >   - Fix if-else condition (pointed by Maarten)
> > > 
> > > Signed-off-by: "Kumar, Mahesh" <mahesh1.kumar at intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_pm.c | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 7f1748a..098336d 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3616,10 +3616,14 @@ static int skl_compute_plane_wm(const
> > > struct
> > > drm_i915_private *dev_priv,
> > >   	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> > >   		selected_result = max(method2, y_tile_minimum);
> > >   	} else {
> > > +		uint32_t linetime_us = 0;
> > > +
> > > +		linetime_us = DIV_ROUND_UP(width * 1000,
> > > +				skl_pipe_pixel_rate(cstate));
> > Can't we just call skl_compute_linetime_wm() here? I don't like
> > having
> > two pieces of the code computing the same thing. My last round of
> > bug
> > fixes included a fix for duplicated code that got out of sync after
> > spec changes.
> These two have different values in skl_compute_linetime_wm we
> multiply 
> by 8, not here.

You can move the *8 multiplication to the caller that needs it.


> > 
> > 
> > > 
> > >   		if ((cpp * cstate-
> > > >base.adjusted_mode.crtc_htotal /
> > > 512 < 1) &&
> > >   		    (plane_bytes_per_line / 512 < 1))
> > >   			selected_result = method2;
> > > -		else if ((ddb_allocation /
> > > plane_blocks_per_line) >=
> > > 1)
> > > +		else if (latency >= linetime_us)
> > Still doesn't match the spec. The "ddb_allocation /
> > planes_block_per_line" is still necessary.
> After New algorithm patch, we will not have access to ddb_allocation,
> as 
> allocation will happen later.
> So we can't use ddb_allocation in our calculation, This check in
> Bspec 
> is kept for the environment/OS where we don't allocate DDB
> dynamically.
> hence those OS will have access to ddb_allocation during WM
> calculation 
> phase.
> thanks,

So the patch that implements the DDB allocation can remove the check
when/if it gets merged. The code with only this patch does not use the
new algorithm, so let's make everything works if we only have it
applied. Bisectability is really important.

> 
> Regards,
> -Mahesh
> > 
> > 
> > > 
> > >   			selected_result = min(method1,
> > > method2);
> > >   		else
> > >   			selected_result = method1;
>