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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 20 Nov 2013 15:10:39 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > +	case DISPPLANE_8BPP:
> > +		return DRM_FORMAT_C8;
> > +	case DISPPLANE_BGRX555:
> > +		return DRM_FORMAT_ARGB1555;
>                                   ^
> X

Oops fixed, thanks.  These formats are a lot of typing.

> > +	case DISPPLANE_RGBA888:
> > +	case DISPPLANE_RGBX101010:
> > +	case DISPPLANE_RGBA101010:
> > +	case DISPPLANE_BGRX101010:
> > +		plane_config->bpp = 32;
> > +		break;
> > +	}
> 
> Maybe just intel_format_to_fourcc()+drm_format_plane_cpp() or something.
> Just to avoid duplicating essentially the same code.

Ah yeah, good idea, fixed.

> > +	val = I915_READ(HTOTAL(pipe));
> > +	plane_config->fb_width = (val & 0xffff) + 1;
> > +	val = I915_READ(VTOTAL(pipe));
> > +	plane_config->fb_height = (val & 0xffff) + 1;
> 
> Why make the fb the size of htotal/vtotal? pipe src size should be all
> we need.

Ok fixed.  Are there no cases where they'll be different?  Maybe
centered modes would have a pipesrc that differs from the
htotal/vtotal?  But even in that case we just want the pipesrc.

> > +struct intel_plane_config {
> > +	int pixel_format; /* DRM fourcc code */
> 
> The comment doesn't match how you do things in the code.

Yeah, Bob mentioned that too, fixed.

Does this look useful to you otherwise?

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