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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 01/14] drm: Centralize format information
Date: Fri, 16 Sep 2016 12:59:27 +0300	[thread overview]
Message-ID: <4784824.hFjVRIpE8i@avalon> (raw)
In-Reply-To: <f70c944e-92fc-782b-23ef-af4cea247b21@ti.com>

Hi Tomi,

On Friday 16 Sep 2016 12:44:31 Tomi Valkeinen wrote:
> On 16/09/16 02:30, Laurent Pinchart wrote:
> > 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 agree, I don't want this series to be held up. But this depth is
> clearly broken, in some way or another. Even more reason to move it to
> fb code =).

And to then drop fbdev ;-)

> >>> 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.
> 
> Ok.
> 
> Btw, are you sure alpha is not counted into depth? With a quick glance,
> also drm_mode_legacy_fb_format() seems to expect depth to include alpha.
> 
> I'm just thinking here that if "depth" is not clearly defined anywhere,
> and the most common formats, 24bit RGB formats, are defined with depth
> including alpha, well, maybe then that's how depth should be defined.
> 
> Then again, I had a quick glance at the fbdev code, and
> fb_get_color_depth() suggests that alpha is not counted in...

I think depth is just ill-defined and shouldn't be used in drivers anymore, at 
least certainly not by new code.

-- 
Regards,

Laurent Pinchart

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