[4/8] drm/rockchip: Use drm_gem_fb_create_with_dirty
diff mbox series

Message ID 20191115092120.4445-5-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • Untitled series #203473
Related show

Commit Message

Daniel Vetter Nov. 15, 2019, 9:21 a.m. UTC
If rockchip would switch over to the generic fbdev setup we could
grabage collect even more of all this code (all of the remaining fb
handling code really).

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

Comments

Andrzej Pietrasiewicz Nov. 27, 2019, 11:45 a.m. UTC | #1
Hi Daniel,

W dniu 15.11.2019 o 10:21, Daniel Vetter pisze:
> If rockchip would switch over to the generic fbdev setup we could
> grabage collect even more of all this code (all of the remaining fb
> handling code really).
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org

I carried out limited testing with modetest on a rockpi4,
using this command:

for i in `./modetest -c | grep ^[[:space:]]*[1-9][0-9]*x[1-9][0-9]* | cut -f3 
-d" " | grep -v i$ | uniq`; do ./modetest -s41:$i -C; done

All modes (excluding those whose names end with an "i", e.g. 1920x1080i)
produced sensible output which seems no different to what is produced
when the patch in question is not applied.

If such a test scope is acceptable, you can add my

Tested-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
>   1 file changed, 1 insertion(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..081dbdaa0b07 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>   	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)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev,
> -								 mode_cmd);
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> -	struct drm_gem_object *obj;
> -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> -	int ret;
> -	int i;
> -
> -	for (i = 0; i < 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]);
> -		if (!obj) {
> -			DRM_DEV_ERROR(dev->dev,
> -				      "Failed to lookup GEM object\n");
> -			ret = -ENXIO;
> -			goto err_gem_object_unreference;
> -		}
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i] +
> -			mode_cmd->offsets[i] +
> -			width * info->cpp[i];
> -
> -		if (obj->size < min_size) {
> -			drm_gem_object_put_unlocked(obj);
> -			ret = -EINVAL;
> -			goto err_gem_object_unreference;
> -		}
> -		objs[i] = obj;
> -	}
> -
> -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto err_gem_object_unreference;
> -	}
> -
> -	return fb;
> -
> -err_gem_object_unreference:
> -	for (i--; i >= 0; i--)
> -		drm_gem_object_put_unlocked(objs[i]);
> -	return ERR_PTR(ret);
> -}
> -
>   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
>   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>   };
>   
>   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> -	.fb_create = rockchip_user_fb_create,
> +	.fb_create = drm_gem_fb_create,
>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
>
Andrzej Pietrasiewicz Nov. 27, 2019, 5:33 p.m. UTC | #2
Hi Daniel,

After applying this patch there are some slight differences
in the effective behavior of the code.

I can't tell if they are important, please see below.

Andrzej

W dniu 15.11.2019 o 10:21, Daniel Vetter pisze:
> If rockchip would switch over to the generic fbdev setup we could
> grabage collect even more of all this code (all of the remaining fb
> handling code really).
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
>   1 file changed, 1 insertion(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..081dbdaa0b07 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>   	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)
> -{
> -	const struct drm_format_info *info = drm_get_format_info(dev,
> -								 mode_cmd);
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> -	struct drm_gem_object *obj;
> -	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> -	int ret;
> -	int i;
> -
> -	for (i = 0; i < num_planes; i++) {

drm_gem_fb_create_with_funcs(), if no error happens,
iterates exactly info->num_planes times,
but the function being removed here iterates
min_t(int, info->num_planes, 3) times.

Is it ensured earlier elsewhere that info->num_planes does not exceed 3?

> -		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]);
> -		if (!obj) {
> -			DRM_DEV_ERROR(dev->dev,
> -				      "Failed to lookup GEM object\n");
> -			ret = -ENXIO;
> -			goto err_gem_object_unreference;
> -		}
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i] +
> -			mode_cmd->offsets[i] +
> -			width * info->cpp[i];

This computation in drm_gem_fb_create_with_funcs looks like this:

		min_size = (height - 1) * mode_cmd->pitches[i]
			 + drm_format_info_min_pitch(info, i, width)
			 + mode_cmd->offsets[i];

Perhaps that's actually the same thing?

> -
> -		if (obj->size < min_size) {
> -			drm_gem_object_put_unlocked(obj);
> -			ret = -EINVAL;
> -			goto err_gem_object_unreference;
> -		}
> -		objs[i] = obj;
> -	}
> -
> -	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto err_gem_object_unreference;
> -	}
> -
> -	return fb;
> -
> -err_gem_object_unreference:
> -	for (i--; i >= 0; i--)
> -		drm_gem_object_put_unlocked(objs[i]);
> -	return ERR_PTR(ret);
> -}
> -
>   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
>   	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>   };
>   
>   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> -	.fb_create = rockchip_user_fb_create,
> +	.fb_create = drm_gem_fb_create,

That way you leave out the ->dirty() callback from
static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs

I'd say instead:

struct drm_framebuffer *
rockchip_gem_fb_create(struct drm_device *dev, struct drm_file *file,
		       const struct drm_mode_fb_cmd2 *mode_cmd)
{
	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
					    &rockchip_drm_fb_funcs);
}

and then

+	.fb_create = rockchip_gem_fb_create,

>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
>   	.atomic_check = drm_atomic_helper_check,
>   	.atomic_commit = drm_atomic_helper_commit,
>
Daniel Vetter Nov. 27, 2019, 5:54 p.m. UTC | #3
On Wed, Nov 27, 2019 at 6:33 PM Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> Hi Daniel,
>
> After applying this patch there are some slight differences
> in the effective behavior of the code.
>
> I can't tell if they are important, please see below.
>
> Andrzej
>
> W dniu 15.11.2019 o 10:21, Daniel Vetter pisze:
> > If rockchip would switch over to the generic fbdev setup we could
> > grabage collect even more of all this code (all of the remaining fb
> > handling code really).
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Sandy Huang <hjc@rock-chips.com>
> > Cc: "Heiko Stübner" <heiko@sntech.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-rockchip@lists.infradead.org
> > ---
> >   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 54 +---------------------
> >   1 file changed, 1 insertion(+), 53 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index ca01234c037c..081dbdaa0b07 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -53,64 +53,12 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
> >       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)
> > -{
> > -     const struct drm_format_info *info = drm_get_format_info(dev,
> > -                                                              mode_cmd);
> > -     struct drm_framebuffer *fb;
> > -     struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
> > -     struct drm_gem_object *obj;
> > -     int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
> > -     int ret;
> > -     int i;
> > -
> > -     for (i = 0; i < num_planes; i++) {
>
> drm_gem_fb_create_with_funcs(), if no error happens,
> iterates exactly info->num_planes times,
> but the function being removed here iterates
> min_t(int, info->num_planes, 3) times.
>
> Is it ensured earlier elsewhere that info->num_planes does not exceed 3?

rockchip only supports fourcc codes with at most 3 planes. Now we're
not checking for that here, it'll only be caught later on in the
ATOMIC ioctl. So would be nice to fix, but it's a preexisting bug (the
todo.rst patch has an idea how to fix this for good). So really
doesn't matter whether we fill out the 4th plane or not. Also iirc we
don't have any fourcc code with 4 planes right now.

> > -             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]);
> > -             if (!obj) {
> > -                     DRM_DEV_ERROR(dev->dev,
> > -                                   "Failed to lookup GEM object\n");
> > -                     ret = -ENXIO;
> > -                     goto err_gem_object_unreference;
> > -             }
> > -
> > -             min_size = (height - 1) * mode_cmd->pitches[i] +
> > -                     mode_cmd->offsets[i] +
> > -                     width * info->cpp[i];
>
> This computation in drm_gem_fb_create_with_funcs looks like this:
>
>                 min_size = (height - 1) * mode_cmd->pitches[i]
>                          + drm_format_info_min_pitch(info, i, width)
>                          + mode_cmd->offsets[i];
>
> Perhaps that's actually the same thing?
>
> > -
> > -             if (obj->size < min_size) {
> > -                     drm_gem_object_put_unlocked(obj);
> > -                     ret = -EINVAL;
> > -                     goto err_gem_object_unreference;
> > -             }
> > -             objs[i] = obj;
> > -     }
> > -
> > -     fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
> > -     if (IS_ERR(fb)) {
> > -             ret = PTR_ERR(fb);
> > -             goto err_gem_object_unreference;
> > -     }
> > -
> > -     return fb;
> > -
> > -err_gem_object_unreference:
> > -     for (i--; i >= 0; i--)
> > -             drm_gem_object_put_unlocked(objs[i]);
> > -     return ERR_PTR(ret);
> > -}
> > -
> >   static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
> >       .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> >   };
> >
> >   static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> > -     .fb_create = rockchip_user_fb_create,
> > +     .fb_create = drm_gem_fb_create,
>
> That way you leave out the ->dirty() callback from
> static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs

Oops indeed. Subject of this patch is right, but the actual patch
isn't, this should be drm_gem_fb_create_with_dirty, then it's a 100%
matching replacement. Thanks for catching this, I'll send out an
updated patch.
-Daniel

>
> I'd say instead:
>
> struct drm_framebuffer *
> rockchip_gem_fb_create(struct drm_device *dev, struct drm_file *file,
>                        const struct drm_mode_fb_cmd2 *mode_cmd)
> {
>         return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
>                                             &rockchip_drm_fb_funcs);
> }
>
> and then
>
> +       .fb_create = rockchip_gem_fb_create,
>
> >       .output_poll_changed = drm_fb_helper_output_poll_changed,
> >       .atomic_check = drm_atomic_helper_check,
> >       .atomic_commit = drm_atomic_helper_commit,
> >
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index ca01234c037c..081dbdaa0b07 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -53,64 +53,12 @@  rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
 	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)
-{
-	const struct drm_format_info *info = drm_get_format_info(dev,
-								 mode_cmd);
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *objs[ROCKCHIP_MAX_FB_BUFFER];
-	struct drm_gem_object *obj;
-	int num_planes = min_t(int, info->num_planes, ROCKCHIP_MAX_FB_BUFFER);
-	int ret;
-	int i;
-
-	for (i = 0; i < 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]);
-		if (!obj) {
-			DRM_DEV_ERROR(dev->dev,
-				      "Failed to lookup GEM object\n");
-			ret = -ENXIO;
-			goto err_gem_object_unreference;
-		}
-
-		min_size = (height - 1) * mode_cmd->pitches[i] +
-			mode_cmd->offsets[i] +
-			width * info->cpp[i];
-
-		if (obj->size < min_size) {
-			drm_gem_object_put_unlocked(obj);
-			ret = -EINVAL;
-			goto err_gem_object_unreference;
-		}
-		objs[i] = obj;
-	}
-
-	fb = rockchip_fb_alloc(dev, mode_cmd, objs, i);
-	if (IS_ERR(fb)) {
-		ret = PTR_ERR(fb);
-		goto err_gem_object_unreference;
-	}
-
-	return fb;
-
-err_gem_object_unreference:
-	for (i--; i >= 0; i--)
-		drm_gem_object_put_unlocked(objs[i]);
-	return ERR_PTR(ret);
-}
-
 static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
 static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
-	.fb_create = rockchip_user_fb_create,
+	.fb_create = drm_gem_fb_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,