Re: [PATCH v4 01/14] drm: Centralize format information

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Eric Engestrom <eric.engestrom@imgtec.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 01/14] drm: Centralize format information
Date: Fri, 16 Sep 2016 02:30:52 +0300	[thread overview]
Message-ID: <6266324.K2me61iYYT@avalon> (raw)
In-Reply-To: <20160915161212.GU3075@imgtec.com>

Hello,

On Thursday 15 Sep 2016 17:12:12 Eric Engestrom wrote:
> On Thu, Sep 15, 2016 at 09:22:54AM +0300, Tomi Valkeinen wrote:
> > On 15/09/16 01:22, Laurent Pinchart wrote:
> >> No, the depth value is the number of colour bits, excluding the alpha
> >> bits. This is used to implement the fbdev compatibility code, as fbdev
> >> (unlike kms) makes use of that information.
> >> 
> >> The total number of bits per pixel is not stored in the table as it can
> >> be computed by cpp[0]*8 + (cpp[1]+cpp[2])*8/hsub/vsub. For RGB formats,
> >> which are the only formats supported by the existing
> >> drm_fb_get_bpp_depth() function, this simplifies to just cpp[0] * 8.
> > 
> > Ok. Then the ARGB8888 & co. formats have depth wrong. I presumed those
> > were right as they're the "normal" ones =).
> 
> Good catch, these should be 24 not 32.
> I must admit I kinda skipped over that table the first time, and only
> checked a few random values.
> I just checked the whole table, and all the C and RGB formats are all
> good (with the 4 /[ARGB]{4}8888/ formats set to .depth=24), and all the
> YUV formats I know (~3/4) are good, but I don't know them all :)

I've checked the existing code that this patch series is replacing, and the 
[ARGB]{4}8888 formats are currently reported as having a depth of 32. I'm not 
sure why that's the case, but I'd rather not touch it in this patch. If this 
is a bug we should fix it in a separate patch.

> > I'm not sure if it's worth the hassle, but if the depth is only for
> > fbdev compat code, maybe a separate format->depth table in fbdev code
> > (for the formats fbdev supports) would make this cleaner and avoid any
> > future mistakes with new drm drivers.
> 
> I agree actually, having it here will encourage anyone to use it. If you
> don't want to split it out, at least add a comment along the lines of
> your reply:
>
> >> This is used to implement the fbdev compatibility code, as fbdev (unlike
> >> kms) makes use of that information.

I've double-checked the existing usage of the depth value, and it turns out 
that quite a few drivers still use it for various purpose, through struct 
drm_framebuffer.depth. I thus wonder whether splitting the depth value from 
the format information table would really help, as drivers would have a way to 
access it anyway.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel