diff mbox

[v4,03/14] drm: Use drm_format_info() in DRM core code

Message ID 1473345868-25453-4-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart Sept. 8, 2016, 2:44 p.m. UTC 
Replace calls to the drm_format_*() helper functions with direct use of
the drm_format_info structure. This improves efficiency by removing
duplicate lookups.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_fb_cma_helper.c |  23 ++++----
 drivers/gpu/drm/drm_framebuffer.c   | 102 +++++-------------------------------
 2 files changed, 25 insertions(+), 100 deletions(-)

Comments

Tomi Valkeinen Sept. 14, 2016, 1:23 p.m. UTC | #1 
On 08/09/16 17:44, Laurent Pinchart wrote:
> Replace calls to the drm_format_*() helper functions with direct use of
> the drm_format_info structure. This improves efficiency by removing
> duplicate lookups.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c |  23 ++++----
>  drivers/gpu/drm/drm_framebuffer.c   | 102 +++++-------------------------------
>  2 files changed, 25 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 1fd6eac1400c..fac4f06f8485 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -176,20 +176,20 @@ struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
>  	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
>  	const struct drm_framebuffer_funcs *funcs)
>  {
> +	const struct drm_format_info *info;
>  	struct drm_fb_cma *fb_cma;
>  	struct drm_gem_cma_object *objs[4];
>  	struct drm_gem_object *obj;
> -	unsigned int hsub;
> -	unsigned int vsub;
>  	int ret;
>  	int i;
>  
> -	hsub = drm_format_horz_chroma_subsampling(mode_cmd->pixel_format);
> -	vsub = drm_format_vert_chroma_subsampling(mode_cmd->pixel_format);
> +	info = drm_format_info(mode_cmd->pixel_format);
> +	if (!info)
> +		return ERR_PTR(-EINVAL);
>  
> -	for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
> -		unsigned int width = mode_cmd->width / (i ? hsub : 1);
> -		unsigned int height = mode_cmd->height / (i ? vsub : 1);
> +	for (i = 0; i < info->num_planes; i++) {
> +		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> +		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
>  		unsigned int min_size;
>  
>  		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
> @@ -200,7 +200,7 @@ struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
>  		}
>  
>  		min_size = (height - 1) * mode_cmd->pitches[i]
> -			 + width * drm_format_plane_cpp(mode_cmd->pixel_format, i)
> +			 + width * info->cpp[i]
>  			 + mode_cmd->offsets[i];
>  
>  		if (obj->size < min_size) {
> @@ -269,12 +269,15 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
>  static void drm_fb_cma_describe(struct drm_framebuffer *fb, struct seq_file *m)
>  {
>  	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> -	int i, n = drm_format_num_planes(fb->pixel_format);
> +	const struct drm_format_info *info;
> +	int i;
>  
>  	seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
>  			(char *)&fb->pixel_format);
>  
> -	for (i = 0; i < n; i++) {
> +	info = drm_format_info(fb->pixel_format);
> +
> +	for (i = 0; i < info->num_planes; i++) {
>  		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
>  				i, fb->offsets[i], fb->pitches[i]);
>  		drm_gem_cma_describe(fb_cma->obj[i], m);

This change doesn't seem to improve the function. Afaics, only the num
planes is retrieved and used.

 Tomi
Laurent Pinchart Sept. 14, 2016, 10:31 p.m. UTC | #2 
Hi Tomi,

Thank you for the review.

On Wednesday 14 Sep 2016 16:23:09 Tomi Valkeinen wrote:
> On 08/09/16 17:44, Laurent Pinchart wrote:
> > Replace calls to the drm_format_*() helper functions with direct use of
> > the drm_format_info structure. This improves efficiency by removing
> > duplicate lookups.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_fb_cma_helper.c |  23 ++++----
> >  drivers/gpu/drm/drm_framebuffer.c   | 102 ++++---------------------------
> >  2 files changed, 25 insertions(+), 100 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac1400c..fac4f06f8485
> > 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c

[snip]

> > @@ -269,12 +269,15 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
> > 
> >  static void drm_fb_cma_describe(struct drm_framebuffer *fb, struct
> >  seq_file *m) {
> >  	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> > -	int i, n = drm_format_num_planes(fb->pixel_format);
> > +	const struct drm_format_info *info;
> > +	int i;
> > 
> >  	seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
> >  			(char *)&fb->pixel_format);
> > 
> > -	for (i = 0; i < n; i++) {
> > +	info = drm_format_info(fb->pixel_format);
> > +
> > +	for (i = 0; i < info->num_planes; i++) {
> >  		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
> >  				i, fb->offsets[i], fb->pitches[i]);
> >  		drm_gem_cma_describe(fb_cma->obj[i], m);
> 
> This change doesn't seem to improve the function. Afaics, only the num
> planes is retrieved and used.

You're right. I was actually trying to remove usage of all the wrappers from 
the DRM core code. If that's not desired I can drop this hunk.
Daniel Vetter Sept. 21, 2016, 7:26 a.m. UTC | #3 
On Thu, Sep 15, 2016 at 01:31:23AM +0300, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the review.
> 
> On Wednesday 14 Sep 2016 16:23:09 Tomi Valkeinen wrote:
> > On 08/09/16 17:44, Laurent Pinchart wrote:
> > > Replace calls to the drm_format_*() helper functions with direct use of
> > > the drm_format_info structure. This improves efficiency by removing
> > > duplicate lookups.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_fb_cma_helper.c |  23 ++++----
> > >  drivers/gpu/drm/drm_framebuffer.c   | 102 ++++---------------------------
> > >  2 files changed, 25 insertions(+), 100 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac1400c..fac4f06f8485
> > > 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> 
> [snip]
> 
> > > @@ -269,12 +269,15 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
> > > 
> > >  static void drm_fb_cma_describe(struct drm_framebuffer *fb, struct
> > >  seq_file *m) {
> > >  	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> > > -	int i, n = drm_format_num_planes(fb->pixel_format);
> > > +	const struct drm_format_info *info;
> > > +	int i;
> > > 
> > >  	seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
> > >  			(char *)&fb->pixel_format);
> > > 
> > > -	for (i = 0; i < n; i++) {
> > > +	info = drm_format_info(fb->pixel_format);
> > > +
> > > +	for (i = 0; i < info->num_planes; i++) {
> > >  		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
> > >  				i, fb->offsets[i], fb->pitches[i]);
> > >  		drm_gem_cma_describe(fb_cma->obj[i], m);
> > 
> > This change doesn't seem to improve the function. Afaics, only the num
> > planes is retrieved and used.
> 
> You're right. I was actually trying to remove usage of all the wrappers from 
> the DRM core code. If that's not desired I can drop this hunk.

No preference from my side, either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 1fd6eac1400c..fac4f06f8485 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -176,20 +176,20 @@  struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
 	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
 	const struct drm_framebuffer_funcs *funcs)
 {
+	const struct drm_format_info *info;
 	struct drm_fb_cma *fb_cma;
 	struct drm_gem_cma_object *objs[4];
 	struct drm_gem_object *obj;
-	unsigned int hsub;
-	unsigned int vsub;
 	int ret;
 	int i;
 
-	hsub = drm_format_horz_chroma_subsampling(mode_cmd->pixel_format);
-	vsub = drm_format_vert_chroma_subsampling(mode_cmd->pixel_format);
+	info = drm_format_info(mode_cmd->pixel_format);
+	if (!info)
+		return ERR_PTR(-EINVAL);
 
-	for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
-		unsigned int width = mode_cmd->width / (i ? hsub : 1);
-		unsigned int height = mode_cmd->height / (i ? vsub : 1);
+	for (i = 0; i < info->num_planes; i++) {
+		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
+		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
 		unsigned int min_size;
 
 		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
@@ -200,7 +200,7 @@  struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
 		}
 
 		min_size = (height - 1) * mode_cmd->pitches[i]
-			 + width * drm_format_plane_cpp(mode_cmd->pixel_format, i)
+			 + width * info->cpp[i]
 			 + mode_cmd->offsets[i];
 
 		if (obj->size < min_size) {
@@ -269,12 +269,15 @@  EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
 static void drm_fb_cma_describe(struct drm_framebuffer *fb, struct seq_file *m)
 {
 	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-	int i, n = drm_format_num_planes(fb->pixel_format);
+	const struct drm_format_info *info;
+	int i;
 
 	seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
 			(char *)&fb->pixel_format);
 
-	for (i = 0; i < n; i++) {
+	info = drm_format_info(fb->pixel_format);
+
+	for (i = 0; i < info->num_planes; i++) {
 		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
 				i, fb->offsets[i], fb->pitches[i]);
 		drm_gem_cma_describe(fb_cma->obj[i], m);
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 30dc01e6eb5d..ce8045228642 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -100,111 +100,33 @@  int drm_mode_addfb(struct drm_device *dev,
 	return 0;
 }
 
-static int format_check(const struct drm_mode_fb_cmd2 *r)
-{
-	uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
-	char *format_name;
-
-	switch (format) {
-	case DRM_FORMAT_C8:
-	case DRM_FORMAT_RGB332:
-	case DRM_FORMAT_BGR233:
-	case DRM_FORMAT_XRGB4444:
-	case DRM_FORMAT_XBGR4444:
-	case DRM_FORMAT_RGBX4444:
-	case DRM_FORMAT_BGRX4444:
-	case DRM_FORMAT_ARGB4444:
-	case DRM_FORMAT_ABGR4444:
-	case DRM_FORMAT_RGBA4444:
-	case DRM_FORMAT_BGRA4444:
-	case DRM_FORMAT_XRGB1555:
-	case DRM_FORMAT_XBGR1555:
-	case DRM_FORMAT_RGBX5551:
-	case DRM_FORMAT_BGRX5551:
-	case DRM_FORMAT_ARGB1555:
-	case DRM_FORMAT_ABGR1555:
-	case DRM_FORMAT_RGBA5551:
-	case DRM_FORMAT_BGRA5551:
-	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_BGR565:
-	case DRM_FORMAT_RGB888:
-	case DRM_FORMAT_BGR888:
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_RGBX8888:
-	case DRM_FORMAT_BGRX8888:
-	case DRM_FORMAT_ARGB8888:
-	case DRM_FORMAT_ABGR8888:
-	case DRM_FORMAT_RGBA8888:
-	case DRM_FORMAT_BGRA8888:
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-	case DRM_FORMAT_RGBX1010102:
-	case DRM_FORMAT_BGRX1010102:
-	case DRM_FORMAT_ARGB2101010:
-	case DRM_FORMAT_ABGR2101010:
-	case DRM_FORMAT_RGBA1010102:
-	case DRM_FORMAT_BGRA1010102:
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_YVYU:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_VYUY:
-	case DRM_FORMAT_AYUV:
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
-	case DRM_FORMAT_NV16:
-	case DRM_FORMAT_NV61:
-	case DRM_FORMAT_NV24:
-	case DRM_FORMAT_NV42:
-	case DRM_FORMAT_YUV410:
-	case DRM_FORMAT_YVU410:
-	case DRM_FORMAT_YUV411:
-	case DRM_FORMAT_YVU411:
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-	case DRM_FORMAT_YUV422:
-	case DRM_FORMAT_YVU422:
-	case DRM_FORMAT_YUV444:
-	case DRM_FORMAT_YVU444:
-		return 0;
-	default:
-		format_name = drm_get_format_name(r->pixel_format);
-		DRM_DEBUG_KMS("invalid pixel format %s\n", format_name);
-		kfree(format_name);
-		return -EINVAL;
-	}
-}
-
 static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 {
-	int ret, hsub, vsub, num_planes, i;
+	const struct drm_format_info *info;
+	int i;
 
-	ret = format_check(r);
-	if (ret) {
+	info = drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
+	if (!info) {
 		char *format_name = drm_get_format_name(r->pixel_format);
 		DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
 		kfree(format_name);
-		return ret;
+		return -EINVAL;
 	}
 
-	hsub = drm_format_horz_chroma_subsampling(r->pixel_format);
-	vsub = drm_format_vert_chroma_subsampling(r->pixel_format);
-	num_planes = drm_format_num_planes(r->pixel_format);
-
-	if (r->width == 0 || r->width % hsub) {
+	if (r->width == 0 || r->width % info->hsub) {
 		DRM_DEBUG_KMS("bad framebuffer width %u\n", r->width);
 		return -EINVAL;
 	}
 
-	if (r->height == 0 || r->height % vsub) {
+	if (r->height == 0 || r->height % info->vsub) {
 		DRM_DEBUG_KMS("bad framebuffer height %u\n", r->height);
 		return -EINVAL;
 	}
 
-	for (i = 0; i < num_planes; i++) {
-		unsigned int width = r->width / (i != 0 ? hsub : 1);
-		unsigned int height = r->height / (i != 0 ? vsub : 1);
-		unsigned int cpp = drm_format_plane_cpp(r->pixel_format, i);
+	for (i = 0; i < info->num_planes; i++) {
+		unsigned int width = r->width / (i != 0 ? info->hsub : 1);
+		unsigned int height = r->height / (i != 0 ? info->vsub : 1);
+		unsigned int cpp = info->cpp[i];
 
 		if (!r->handles[i]) {
 			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
@@ -247,7 +169,7 @@  static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 		}
 	}
 
-	for (i = num_planes; i < 4; i++) {
+	for (i = info->num_planes; i < 4; i++) {
 		if (r->modifier[i]) {
 			DRM_DEBUG_KMS("non-zero modifier for unused plane %d\n", i);
 			return -EINVAL;