Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, May 04, 2015 at 09:57:17AM +0200, Daniel Vetter wrote:
> On Mon, Apr 27, 2015 at 08:33:38PM +0000, Konduru, Chandra wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Monday, April 27, 2015 8:28 AM
> > > On Fri, Feb 20, 2015 at 04:11:33PM -0800, Chandra Konduru wrote:
> > > > This patch adds support for Skylake display pipe background color.
> > > > +	<td valign="top" >Background color in 16bpc BGR (B-MSB, R-LSB)</td>
> > > 
> > > I'd prefer to settle on ARGB msb->lsb layout, unless there's already precedent
> > > for something else. I think it's the most common component order around, and
> > > so should result in least amount of code to stuff the bits into the hardware
> > > registers.
> > 
> > To give some background on this, initially using ARGB msb->lsb layout to stuff
> > bit into hw regs, but changed to BGR msb->lsb per feedback. Before reversing/
> > respunning, would like to have an agreement. Daniel/Damien, are you ok with 
> > reversing the order to align with hw? Or just keep the way current patch doing?
> 
> Imo if we want to really end up with a real standard we should have a
> macro to encode ARGB values and 4 macros to extract the individual parts
> in some uapi header and use that instead of open-coding shifts. That would
> need to be a drm core header ofc.

Sounds like a decent idea.

> The exact layout is probably irrelevant
> anyway.

I think going with (A)RGB makes it much easier to parse when you're
looking at the hex string. That's the order those things usually come
in.

Which brings another point to mind. Should we define a new property type
for color data? Would make it easier to pretty-print in userland.
Using decimal for color data isn't very useful after all.

> -Daniel
> 
> > 
> > > 
> > > > +static int intel_crtc_set_property(struct drm_crtc *crtc,
> > > > +	struct drm_property *property, uint64_t val) {
> > > > +	struct drm_device *dev = crtc->dev;
> > > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +	int i;
> > > > +
> > > > +	if (INTEL_INFO(dev)->gen >= 9) {
> > > > +		if (property == dev->mode_config.background_color_property)
> > > {
> > > > +			uint64_t bottom = 0;
> > > > +
> > > > +			/* BGR 16bpc ==> RGB 10bpc */
> > > > +			for (i = 0; i < 3; i++)
> > > > +				bottom |= (((val >> (i * 16 + 6)) & 0x3FF) << ((2 -
> > > i) * 10));
> > > > +
> > > > +			intel_crtc->background_color = (uint32_t) bottom;
> > > > +
> > > > +			skl_crtc_set_background_color(intel_crtc);
> > > > +		}
> > > > +		return 0;
> > > > +	}
> > > > +	return -EINVAL;
> > > > +}
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx