Re: [PATCH 1/5] drm/i915: make pitch_for_width take a tiled arg v2

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 26 Nov 2013 19:28:27 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Nov 25, 2013 at 03:51:15PM -0800, Jesse Barnes wrote:
> > And move it up in the file for earlier usage.
> > 
> > v2: add pre-gen4 support as well (Chris)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++++++----------
> >  1 file changed, 38 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e85d838..321d751 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5452,6 +5452,28 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >  	pipe_config->port_clock = clock.dot / 5;
> >  }
> >  
> > +static u32
> > +intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
> > +				  int bpp, bool tiled)
> > +{
> > +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> > +	int align;
> > +
> > +	if (tiled) {
> > +		if (INTEL_INFO(dev_priv->dev)->gen < 4) {
> > +			/* Pre-965 needs power of two tile width */
> > +			for (align = 512; align < pitch; align <<= 1)
> > +				;
> > +		} else {
> > +			align = 512;
> > +		}
> 
> Gen2 tiles are 128 bytes wide, not 512 bytes.
> 
> So maybe something like this:
>  if (IS_GEN2()) {
>    return roundup_power_of_two(max(pitch, 128));
>  else if (IS_GEN3())
>    return roundup_power_of_two(max(pitch, 512));
>  else
>    return ALIGN(pitch, 512);
> 
> > +	} else {
> > +		align = 64;

Ah good old gen2.  Pretty sure we've just treated it as 512 wide in the
past?  But I'm not going to go digging around the DDX to see... :)

I was looking for that POT helper but didn't see it, that makes things
a little nicer.

> Also I just noticed in the docs that gen2/3 only seem to require 32byte
> alignment for linear buffers. But relaxing that would require a change
> to intel_framebuffer_init() as well, so it looks like material for
> another patch. Also would need to be tested on real hardware.

Yeah separate patch.  It'll save us gobs of memory. :)

> > -intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
> > +intel_framebuffer_size_for_mode(struct drm_i915_private *dev_priv,
> > +				struct drm_display_mode *mode, int bpp)
> >  {
> > -	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> > +	u32 pitch = intel_framebuffer_pitch_for_width(dev_priv,
> > +						      mode->hdisplay, bpp,
> > +						      false);
> >  	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);
> 
> This should also align the fb height to tile height, otherwise
> intel_framebuffer_init() might reject it.

Hm another existing bug.  Separate patch.

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