| Message ID | 1432119035.4466.46.camel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -Daniel
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.
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 regards Philipp
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
--- 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: