Re: [PATCH 3/5] drm/i915: retrieve current fb config into new plane_config structure at init v4

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 26 Nov 2013 18:43:53 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> > +	plane_config->fb->base.pitches[0] =
> > +		intel_framebuffer_pitch_for_width(dev_priv,
> > +						  plane_config->fb->base.width,
> > +						  plane_config->fb->base.bits_per_pixel,
> > +						  plane_config->tiled);
> 
> Shouldn't we read out the stride from the respective hw register, too?

Hm that's quite an idea.

> 
> > +
> > +	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> > +				   plane_config->fb->base.height, PAGE_SIZE);
> 
> If you bother with tiling I think you also need to bother with correct
> size alignement.

Yeah just fixed up the framebuffer size function per Ville's comments.
I should be able to just use that.

> > @@ -10562,6 +10672,7 @@ static void intel_init_display(struct drm_device *dev)
> >  		dev_priv->display.update_plane = ironlake_update_plane;
> 
> Sad Haswell is sad it seems.

Yeah for two reasons: I'm not testing HSW at all, and I'm still fairly
ignorant of HSW display.  Well that and I don't want to pile on more
work for this patchset which has already seen a bunch of churn...

> > +		if (dev_priv->display.get_plane_config)
> > +			dev_priv->display.get_plane_config(crtc,
> > +							   &crtc->plane_config);
> > +	}
> 
> If we go with Chris' suggestion to disable the crtc if we can't get at a
> sane framebuffer for it then this would make much more sense in the
> hardware state readout code. Especially since always having framebuffers
> around would allow us to ditch a bit of special-casing code all over the
> place.

I don't think so; reading out and allocating an fb on every plane
config readout would be overkill.  We'd need to poke around in the
struct for cross checking, then free it somewhere.  Plus it won't
always be preallocated, and creating a duplicate fb when the object
still exists would fail.

This is actually another argument for simply duplicating a few fields
from the fb struct into the plane config struct.  That makes cross
checking and readout simple, and allows us to allocate if possible in
the BIOS functions.

But damnit, then I'd have to drop back to an earlier version of the
patch and lose some changes... ugg

-- 
Jesse Barnes, Intel Open Source Technology Center