Re: [PATCH 05/24] drm/rockchip: rockchip_drm_fb -> drm_framebuffer

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3604 bytes --]

On Fri, Mar 30, 2018 at 03:11:19PM +0100, Daniel Stone wrote:
> Now that rockchip_drm_fb is just a wrapper around drm_framebuffer, we
> can remove it.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: Heiko Stübner <heiko@sntech.de>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  | 54 ++++++++++-------------------
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.h  |  3 --
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  4 +--
>  3 files changed, 21 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index d7083c07c294..a4d0a00abcd9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -25,21 +25,6 @@
>  #include "rockchip_drm_gem.h"
>  #include "rockchip_drm_psr.h"
>  
> -#define to_rockchip_fb(x) container_of(x, struct rockchip_drm_fb, fb)
> -
> -struct rockchip_drm_fb {
> -	struct drm_framebuffer fb;
> -};
> -
> -struct drm_gem_object *rockchip_fb_get_gem_obj(struct drm_framebuffer *fb,
> -					       unsigned int plane)
> -{
> -	if (plane >= ROCKCHIP_MAX_FB_BUFFER)
> -		return NULL;
> -
> -	return fb->obj[plane];
> -}
> -
>  static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
>  				 struct drm_file *file,
>  				 unsigned int flags, unsigned int color,
> @@ -56,41 +41,40 @@ static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>  	.dirty	       = rockchip_drm_fb_dirty,
>  };
>  
> -static struct rockchip_drm_fb *
> +static struct drm_framebuffer *
>  rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd,
>  		  struct drm_gem_object **obj, unsigned int num_planes)
>  {
> -	struct rockchip_drm_fb *rockchip_fb;
> +	struct drm_framebuffer *fb;
>  	int ret;
>  	int i;
>  
> -	rockchip_fb = kzalloc(sizeof(*rockchip_fb), GFP_KERNEL);
> -	if (!rockchip_fb)
> +	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> +	if (!fb)
>  		return ERR_PTR(-ENOMEM);
>  
> -	drm_helper_mode_fill_fb_struct(dev, &rockchip_fb->fb, mode_cmd);
> +	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>  
>  	for (i = 0; i < num_planes; i++)
> -		rockchip_fb->fb.obj[i] = obj[i];
> +		fb->obj[i] = obj[i];
>  
> -	ret = drm_framebuffer_init(dev, &rockchip_fb->fb,
> -				   &rockchip_drm_fb_funcs);
> +	ret = drm_framebuffer_init(dev, fb, &rockchip_drm_fb_funcs);
>  	if (ret) {
>  		DRM_DEV_ERROR(dev->dev,
>  			      "Failed to initialize framebuffer: %d\n",
>  			      ret);
> -		kfree(rockchip_fb);
> +		kfree(fb);
>  		return ERR_PTR(ret);
>  	}
>  
> -	return rockchip_fb;
> +	return fb;
>  }
>  
>  static struct drm_framebuffer *
>  rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>  			const struct drm_mode_fb_cmd2 *mode_cmd)
>  {
> -	struct rockchip_drm_fb *rockchip_fb;
> +	struct drm_framebuffer *fb;
>  	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];

The use of the ROCKCHIP_MAX_FB_BUFFER seems somewhat misguided here.
What if a given pixel format requires 4 planes? This function will just
silently ignore the last plane. Not sure that's a good idea. Currently I
don't know of any formats that require 4 planes, so this is probably not
an issue, but there's also no reason to limit this to anything less than
the 4 planes that's baked into the KMS ABI everywhere.

Anyway, just a random comment and it doesn't have anything to do with
this series, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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