Re: [PATCH 6/7] drm: add RGB formats with separate alpha plane

From: Daniel Vetter <daniel@ffwll.ch>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: kernel@pengutronix.de, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 6/7] drm: add RGB formats with separate alpha plane
Date: Wed, 20 May 2015 17:09:36 +0200	[thread overview]
Message-ID: <20150520150936.GG15256@phenom.ffwll.local> (raw)
In-Reply-To: <1432133641.4466.62.camel@pengutronix.de>

On Wed, May 20, 2015 at 04:54:01PM +0200, Philipp Zabel wrote:
> Am Mittwoch, den 20.05.2015, 17:07 +0300 schrieb Ville Syrjälä:
> > On Wed, May 20, 2015 at 02:58:53PM +0200, Daniel Vetter wrote:
> > > On Wed, May 20, 2015 at 12:50:35PM +0200, Philipp Zabel wrote:
> > > > Hi Daniel,
> > > > 
> > > > thank you for the comments.
> > > > 
> > > > Am Dienstag, den 19.05.2015, 18:52 +0200 schrieb Daniel Vetter:
> > > > [...]
> > > > > > @@ -5198,11 +5206,15 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> > > > > >  		break;
> > > > > >  	case DRM_FORMAT_RGB565:
> > > > > >  	case DRM_FORMAT_BGR565:
> > > > > > +	case DRM_FORMAT_RGB565_A8:
> > > > > > +	case DRM_FORMAT_BGR565_A8:
> > > > > >  		*depth = 16;
> > > > > >  		*bpp = 16;
> > > > > >  		break;
> > > > > >  	case DRM_FORMAT_RGB888:
> > > > > >  	case DRM_FORMAT_BGR888:
> > > > > > +	case DRM_FORMAT_RGB888_A8:
> > > > > > +	case DRM_FORMAT_BGR888_A8:
> > > > > >  		*depth = 24;
> > > > > >  		*bpp = 24;
> > > > > >  		break;
> > > > > > @@ -5210,6 +5222,10 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> > > > > >  	case DRM_FORMAT_XBGR8888:
> > > > > >  	case DRM_FORMAT_RGBX8888:
> > > > > >  	case DRM_FORMAT_BGRX8888:
> > > > > > +	case DRM_FORMAT_XRGB8888_A8:
> > > > > > +	case DRM_FORMAT_XBGR8888_A8:
> > > > > > +	case DRM_FORMAT_RGBX8888_A8:
> > > > > > +	case DRM_FORMAT_BGRX8888_A8:
> > > > > >  		*depth = 24;
> > > > > >  		*bpp = 32;
> > > > > >  		break;
> > > > > 
> > > > > Please drop the above two hunks, these functions are only for backwards
> > > > > compat with drivers from the addfb1 days. Modern drivers should only use
> > > > > the format tags directly. Extending the plane_cpp function like you do
> > > > > below is enough.
> > > > 
> > > > I'll leave drm_fb_get_bpp_depth untouched.
> > > > 
> > > > > Maybe we should add a WARN_ON(num_planes(format) != 0) to the top of this
> > > > > and a comment that this is for legacy stuff only.
> > > > 
> > > > Do you mean:
> > > > 
> > > > -----8<-----
> > > > --- a/drivers/gpu/drm/drm_crtc.c
> > > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > @@ -5186,6 +5186,12 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
> > > >  void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> > > >  			  int *bpp)
> > > >  {
> > > > +	/*
> > > > +	 * This function is to be used for legacy drivers only, no new formats
> > > > +	 * should be added here.
> > > > +	 */
> > > > +	WARN_ON(drm_format_num_planes(format) != 1);
> > > > +
> > > >  	switch (format) {
> > > >  	case DRM_FORMAT_C8:
> > > >  	case DRM_FORMAT_RGB332:
> > > > ----->8-----
> > > 
> > > Yeah that looks perfect, please wrap with commit message&sob and I'll
> > > apply.
> > 
> > I think that's going to trigger a lot. IIRC we call this thing for
> > any format when constructing FBs.
> 
> Yes I just noticed that too, for example via:
> 
> drm_fb_cma_create -> drm_fb_cma_alloc -> drm_helper_mode_fill_fb_struct
> -> drm_fb_get_bpp_depth

Hm right, so perhaps just a comment then?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel