| Message ID | 20191213155907.16581-4-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AFBC support for Rockchip | expand |
Hi Andrzej, On Fri, Dec 13, 2019 at 04:58:34PM +0100, Andrzej Pietrasiewicz wrote: > Prepare tools for drivers which need to allocate a struct drm_framebuffer > (or a container of struct drm_framebuffer) explicitly, before calling > helpers. In such a case we need new helpers which omit allocating the > struct drm_framebuffer and this patch provides them. Consequently, they > are used also inside the helpers themselves. > > The interested drivers will likely need to be able to perform object > lookups and size checks in separate invocations and this patch provides > that as well. Helpers themselves are updated, too. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 184 ++++++++++++++----- > include/drm/drm_gem_framebuffer_helper.h | 17 ++ > 2 files changed, 153 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > index b9bcd310ca2d..787edb9a916b 100644 > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > @@ -54,6 +54,44 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > } > EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj); > > +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, > + unsigned int num_planes, > + const struct drm_framebuffer_funcs *funcs) > +{ > + int ret, i; > + > + drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd); > + > + for (i = 0; i < num_planes; i++) > + fb->obj[i] = obj[i]; > + > + ret = drm_framebuffer_init(dev, fb, funcs); > + if (ret) > + DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n", > + ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs); > + > +static const struct drm_framebuffer_funcs drm_gem_fb_funcs = { > + .destroy = drm_gem_fb_destroy, > + .create_handle = drm_gem_fb_create_handle, > +}; > + > +int drm_gem_fb_init(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, unsigned int num_planes) > +{ > + return drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, > + &drm_gem_fb_funcs); > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_init); If you export these two function they should better have documentation associated with them. > + > static struct drm_framebuffer * > drm_gem_fb_alloc(struct drm_device *dev, > const struct drm_mode_fb_cmd2 *mode_cmd, > @@ -61,21 +99,15 @@ drm_gem_fb_alloc(struct drm_device *dev, > const struct drm_framebuffer_funcs *funcs) > { > struct drm_framebuffer *fb; > - int ret, i; > + int ret; > > fb = kzalloc(sizeof(*fb), GFP_KERNEL); > if (!fb) > return ERR_PTR(-ENOMEM); > > - drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd); > - > - for (i = 0; i < num_planes; i++) > - fb->obj[i] = obj[i]; > - > - ret = drm_framebuffer_init(dev, fb, funcs); > + ret = drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, > + funcs); > if (ret) { > - DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n", > - ret); > kfree(fb); > return ERR_PTR(ret); > } > @@ -124,79 +156,135 @@ int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file, > EXPORT_SYMBOL(drm_gem_fb_create_handle); > > /** > - * drm_gem_fb_create_with_funcs() - Helper function for the > - * &drm_mode_config_funcs.fb_create > - * callback > + * drm_gem_fb_lookup() - Helper function for use in > + * &drm_mode_config_funcs.fb_create implementations > * @dev: DRM device > * @file: DRM file that holds the GEM handle(s) backing the framebuffer > * @mode_cmd: Metadata from the userspace framebuffer creation request > - * @funcs: vtable to be used for the new framebuffer object > * > - * This function can be used to set &drm_framebuffer_funcs for drivers that need > - * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to > - * change &drm_framebuffer_funcs. The function does buffer size validation. > + * This function can be used to look up the objects for all planes. > + * In case an error is returned all the objects are put by the > + * function before returning. > * > * Returns: > - * Pointer to a &drm_framebuffer on success or an error pointer on failure. > + * Number of planes on success or a negative error code on failure. > */ > -struct drm_framebuffer * > -drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, > - const struct drm_mode_fb_cmd2 *mode_cmd, > - const struct drm_framebuffer_funcs *funcs) > +int drm_gem_fb_lookup(struct drm_device *dev, > + struct drm_file *file, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs) > { > const struct drm_format_info *info; > - struct drm_gem_object *objs[4]; > - struct drm_framebuffer *fb; > int ret, i; > > info = drm_get_format_info(dev, mode_cmd); > if (!info) > - return ERR_PTR(-EINVAL); > + return -EINVAL; > > 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; > - > objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); > if (!objs[i]) { > DRM_DEBUG_KMS("Failed to lookup GEM object\n"); > ret = -ENOENT; > goto err_gem_object_put; > } > + } > + > + return i; > + > +err_gem_object_put: > + for (i--; i >= 0; i--) > + drm_gem_object_put_unlocked(objs[i]); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_lookup); I'm surprised git generated this mess of a diff. Given that you still have drm_gem_fb_create_with_funcs() further down, maybe you can re-order the functions so that Git creates a more sane diff? > + > +/** > + * drm_gem_fb_size_check() - Helper function for use in > + * &drm_mode_config_funcs.fb_create implementations > + * @dev: DRM device > + * @mode_cmd: Metadata from the userspace framebuffer creation request > + * > + * This function can be used to verify buffer sizes for all planes. > + * It is caller's responsibility to put the objects on failure. > + * > + * Returns: > + * Zero on success or a negative error code on failure. > + */ > +int drm_gem_fb_size_check(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs) > +{ > + const struct drm_format_info *info; > + int i; > + > + info = drm_get_format_info(dev, mode_cmd); > + if (!info) > + return -EINVAL; > + > + 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; > > min_size = (height - 1) * mode_cmd->pitches[i] > + drm_format_info_min_pitch(info, i, width) > + mode_cmd->offsets[i]; > > - if (objs[i]->size < min_size) { > - drm_gem_object_put_unlocked(objs[i]); > - ret = -EINVAL; > - goto err_gem_object_put; > - } > + if (objs[i]->size < min_size) > + return -EINVAL; > } > > - fb = drm_gem_fb_alloc(dev, mode_cmd, objs, i, funcs); > - if (IS_ERR(fb)) { > - ret = PTR_ERR(fb); > - goto err_gem_object_put; > - } > + return 0; > > - return fb; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_size_check); > > -err_gem_object_put: > - for (i--; i >= 0; i--) > - drm_gem_object_put_unlocked(objs[i]); > +/** > + * drm_gem_fb_create_with_funcs() - Helper function for the > + * &drm_mode_config_funcs.fb_create > + * callback > + * @dev: DRM device > + * @file: DRM file that holds the GEM handle(s) backing the framebuffer > + * @mode_cmd: Metadata from the userspace framebuffer creation request > + * @funcs: vtable to be used for the new framebuffer object > + * > + * This function can be used to set &drm_framebuffer_funcs for drivers that need > + * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to > + * change &drm_framebuffer_funcs. The function does buffer size validation. > + * > + * Returns: > + * Pointer to a &drm_framebuffer on success or an error pointer on failure. > + */ > +struct drm_framebuffer * > +drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + const struct drm_framebuffer_funcs *funcs) > +{ > + struct drm_gem_object *objs[4]; > + struct drm_framebuffer *fb; > + int ret, num_planes; > + > + ret = drm_gem_fb_lookup(dev, file, mode_cmd, objs); > + if (ret < 0) > + return ERR_PTR(ret); > + num_planes = ret; > + > + ret = drm_gem_fb_size_check(dev, mode_cmd, objs); if drm_gem_fb_size_check() returns an error, then ... > + if (ret) > + fb = ERR_PTR(ret); > + else > + fb = drm_gem_fb_alloc(dev, mode_cmd, objs, num_planes, funcs); .... the else part is not taken, but ... > > - return ERR_PTR(ret); > + if (IS_ERR(fb)) > + for (num_planes--; num_planes >= 0; num_planes--) > + drm_gem_object_put_unlocked(objs[num_planes]); ... here you'll attempt to dereference the objs. I don't think that is correct. > + > + return fb; > } > EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_funcs); > > -static const struct drm_framebuffer_funcs drm_gem_fb_funcs = { > - .destroy = drm_gem_fb_destroy, > - .create_handle = drm_gem_fb_create_handle, > -}; > - > /** > * drm_gem_fb_create() - Helper function for the > * &drm_mode_config_funcs.fb_create callback > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h > index d9f13fd25b0a..c85d4b152e91 100644 > --- a/include/drm/drm_gem_framebuffer_helper.h > +++ b/include/drm/drm_gem_framebuffer_helper.h > @@ -14,10 +14,27 @@ struct drm_simple_display_pipe; > > struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > unsigned int plane); > +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, > + unsigned int num_planes, > + const struct drm_framebuffer_funcs *funcs); > +int drm_gem_fb_init(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, unsigned int num_planes); > void drm_gem_fb_destroy(struct drm_framebuffer *fb); > int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file, > unsigned int *handle); > > +int drm_gem_fb_lookup(struct drm_device *dev, > + struct drm_file *file, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs); > +int drm_gem_fb_size_check(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs); > struct drm_framebuffer * > drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, > const struct drm_mode_fb_cmd2 *mode_cmd, > -- > 2.17.1 > Best regards, Liviu
Hi Liviu, My way of thinking is explained below. Do you still find it problematic? W dniu 16.12.2019 o 18:08, Liviu Dudau pisze: > Hi Andrzej, > <snip> >> +struct drm_framebuffer * >> +drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, >> + const struct drm_mode_fb_cmd2 *mode_cmd, >> + const struct drm_framebuffer_funcs *funcs) >> +{ >> + struct drm_gem_object *objs[4]; >> + struct drm_framebuffer *fb; >> + int ret, num_planes; >> + >> + ret = drm_gem_fb_lookup(dev, file, mode_cmd, objs); >> + if (ret < 0) >> + return ERR_PTR(ret); here objs is guaranteed to have been filled... >> + num_planes = ret; >> + >> + ret = drm_gem_fb_size_check(dev, mode_cmd, objs); > > if drm_gem_fb_size_check() returns an error, then ... > >> + if (ret) >> + fb = ERR_PTR(ret); >> + else >> + fb = drm_gem_fb_alloc(dev, mode_cmd, objs, num_planes, funcs); > > .... the else part is not taken, but ... ... nonetheless objs have already been looked up... > >> >> - return ERR_PTR(ret); >> + if (IS_ERR(fb)) >> + for (num_planes--; num_planes >= 0; num_planes--) >> + drm_gem_object_put_unlocked(objs[num_planes]); > > ... here you'll attempt to dereference the objs. I don't think that is correct. ... so it is safe to dereference objs here Regards, Andrzej
Hi Andrzej: Good work. It's a real useful patch, with it seems most vendor-specific fb_create can be simplified by these helper funcs. On Fri, Dec 13, 2019 at 04:58:34PM +0100, Andrzej Pietrasiewicz wrote: > Prepare tools for drivers which need to allocate a struct drm_framebuffer > (or a container of struct drm_framebuffer) explicitly, before calling > helpers. In such a case we need new helpers which omit allocating the > struct drm_framebuffer and this patch provides them. Consequently, they > are used also inside the helpers themselves. > > The interested drivers will likely need to be able to perform object > lookups and size checks in separate invocations and this patch provides > that as well. Helpers themselves are updated, too. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 184 ++++++++++++++----- > include/drm/drm_gem_framebuffer_helper.h | 17 ++ > 2 files changed, 153 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > index b9bcd310ca2d..787edb9a916b 100644 > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > @@ -54,6 +54,44 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > } > EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj); > > +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, > + unsigned int num_planes, > + const struct drm_framebuffer_funcs *funcs) > +{ > + int ret, i; > + > + drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd); > + > + for (i = 0; i < num_planes; i++) > + fb->obj[i] = obj[i]; > + > + ret = drm_framebuffer_init(dev, fb, funcs); > + if (ret) > + DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n", > + ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs); > + > +static const struct drm_framebuffer_funcs drm_gem_fb_funcs = { > + .destroy = drm_gem_fb_destroy, > + .create_handle = drm_gem_fb_create_handle, > +}; > + > +int drm_gem_fb_init(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, unsigned int num_planes) > +{ > + return drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, > + &drm_gem_fb_funcs); > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_init); > + > static struct drm_framebuffer * > drm_gem_fb_alloc(struct drm_device *dev, > const struct drm_mode_fb_cmd2 *mode_cmd, > @@ -61,21 +99,15 @@ drm_gem_fb_alloc(struct drm_device *dev, > const struct drm_framebuffer_funcs *funcs) > { > struct drm_framebuffer *fb; > - int ret, i; > + int ret; > > fb = kzalloc(sizeof(*fb), GFP_KERNEL); > if (!fb) > return ERR_PTR(-ENOMEM); > > - drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd); > - > - for (i = 0; i < num_planes; i++) > - fb->obj[i] = obj[i]; > - > - ret = drm_framebuffer_init(dev, fb, funcs); > + ret = drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, > + funcs); > if (ret) { > - DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n", > - ret); > kfree(fb); > return ERR_PTR(ret); > } > @@ -124,79 +156,135 @@ int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file, > EXPORT_SYMBOL(drm_gem_fb_create_handle); > > /** > - * drm_gem_fb_create_with_funcs() - Helper function for the > - * &drm_mode_config_funcs.fb_create > - * callback > + * drm_gem_fb_lookup() - Helper function for use in > + * &drm_mode_config_funcs.fb_create implementations > * @dev: DRM device > * @file: DRM file that holds the GEM handle(s) backing the framebuffer > * @mode_cmd: Metadata from the userspace framebuffer creation request > - * @funcs: vtable to be used for the new framebuffer object > * > - * This function can be used to set &drm_framebuffer_funcs for drivers that need > - * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to > - * change &drm_framebuffer_funcs. The function does buffer size validation. > + * This function can be used to look up the objects for all planes. > + * In case an error is returned all the objects are put by the > + * function before returning. > * > * Returns: > - * Pointer to a &drm_framebuffer on success or an error pointer on failure. > + * Number of planes on success or a negative error code on failure. > */ > -struct drm_framebuffer * > -drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, > - const struct drm_mode_fb_cmd2 *mode_cmd, > - const struct drm_framebuffer_funcs *funcs) > +int drm_gem_fb_lookup(struct drm_device *dev, [nit-pick] Maybe name it to drm_gem_fb_objs_lookup() > + struct drm_file *file, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs) > { > const struct drm_format_info *info; > - struct drm_gem_object *objs[4]; > - struct drm_framebuffer *fb; > int ret, i; > > info = drm_get_format_info(dev, mode_cmd); > if (!info) > - return ERR_PTR(-EINVAL); > + return -EINVAL; > > 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; > - > objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); > if (!objs[i]) { > DRM_DEBUG_KMS("Failed to lookup GEM object\n"); > ret = -ENOENT; > goto err_gem_object_put; > } > + } > + > + return i; > + > +err_gem_object_put: > + for (i--; i >= 0; i--) > + drm_gem_object_put_unlocked(objs[i]); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_lookup); > + > +/** > + * drm_gem_fb_size_check() - Helper function for use in > + * &drm_mode_config_funcs.fb_create implementations > + * @dev: DRM device > + * @mode_cmd: Metadata from the userspace framebuffer creation request > + * > + * This function can be used to verify buffer sizes for all planes. > + * It is caller's responsibility to put the objects on failure. > + * > + * Returns: > + * Zero on success or a negative error code on failure. > + */ > +int drm_gem_fb_size_check(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs) > +{ > + const struct drm_format_info *info; > + int i; > + > + info = drm_get_format_info(dev, mode_cmd); > + if (!info) > + return -EINVAL; > + > + 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; > > min_size = (height - 1) * mode_cmd->pitches[i] > + drm_format_info_min_pitch(info, i, width) > + mode_cmd->offsets[i]; > > - if (objs[i]->size < min_size) { > - drm_gem_object_put_unlocked(objs[i]); > - ret = -EINVAL; > - goto err_gem_object_put; > - } > + if (objs[i]->size < min_size) > + return -EINVAL; > } > > - fb = drm_gem_fb_alloc(dev, mode_cmd, objs, i, funcs); > - if (IS_ERR(fb)) { > - ret = PTR_ERR(fb); > - goto err_gem_object_put; > - } > + return 0; > > - return fb; > +} > +EXPORT_SYMBOL_GPL(drm_gem_fb_size_check); > > -err_gem_object_put: > - for (i--; i >= 0; i--) > - drm_gem_object_put_unlocked(objs[i]); > +/** > + * drm_gem_fb_create_with_funcs() - Helper function for the > + * &drm_mode_config_funcs.fb_create > + * callback > + * @dev: DRM device > + * @file: DRM file that holds the GEM handle(s) backing the framebuffer > + * @mode_cmd: Metadata from the userspace framebuffer creation request > + * @funcs: vtable to be used for the new framebuffer object > + * > + * This function can be used to set &drm_framebuffer_funcs for drivers that need > + * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to > + * change &drm_framebuffer_funcs. The function does buffer size validation. > + * > + * Returns: > + * Pointer to a &drm_framebuffer on success or an error pointer on failure. > + */ > +struct drm_framebuffer * > +drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + const struct drm_framebuffer_funcs *funcs) > +{ > + struct drm_gem_object *objs[4]; > + struct drm_framebuffer *fb; > + int ret, num_planes; > + > + ret = drm_gem_fb_lookup(dev, file, mode_cmd, objs); > + if (ret < 0) > + return ERR_PTR(ret); > + num_planes = ret; > + > + ret = drm_gem_fb_size_check(dev, mode_cmd, objs); > + if (ret) > + fb = ERR_PTR(ret); > + else > + fb = drm_gem_fb_alloc(dev, mode_cmd, objs, num_planes, funcs); > > - return ERR_PTR(ret); > + if (IS_ERR(fb)) > + for (num_planes--; num_planes >= 0; num_planes--) > + drm_gem_object_put_unlocked(objs[num_planes]); > + > + return fb; > } > EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_funcs); > > -static const struct drm_framebuffer_funcs drm_gem_fb_funcs = { > - .destroy = drm_gem_fb_destroy, > - .create_handle = drm_gem_fb_create_handle, > -}; > - > /** > * drm_gem_fb_create() - Helper function for the > * &drm_mode_config_funcs.fb_create callback > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h > index d9f13fd25b0a..c85d4b152e91 100644 > --- a/include/drm/drm_gem_framebuffer_helper.h > +++ b/include/drm/drm_gem_framebuffer_helper.h > @@ -14,10 +14,27 @@ struct drm_simple_display_pipe; > > struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > unsigned int plane); > +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, > + unsigned int num_planes, > + const struct drm_framebuffer_funcs *funcs); > +int drm_gem_fb_init(struct drm_framebuffer *fb, > + struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **obj, unsigned int num_planes); > void drm_gem_fb_destroy(struct drm_framebuffer *fb); > int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file, > unsigned int *handle); > > +int drm_gem_fb_lookup(struct drm_device *dev, > + struct drm_file *file, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs); > +int drm_gem_fb_size_check(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd, > + struct drm_gem_object **objs); > struct drm_framebuffer * > drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, > const struct drm_mode_fb_cmd2 *mode_cmd, Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com> James.
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index b9bcd310ca2d..787edb9a916b 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -54,6 +54,44 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, } EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj); +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb, + struct drm_device *dev, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **obj, + unsigned int num_planes, + const struct drm_framebuffer_funcs *funcs) +{ + int ret, i; + + drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd); + + for (i = 0; i < num_planes; i++) + fb->obj[i] = obj[i]; + + ret = drm_framebuffer_init(dev, fb, funcs); + if (ret) + DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n", + ret); + + return ret; +} +EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs); + +static const struct drm_framebuffer_funcs drm_gem_fb_funcs = { + .destroy = drm_gem_fb_destroy, + .create_handle = drm_gem_fb_create_handle, +}; + +int drm_gem_fb_init(struct drm_framebuffer *fb, + struct drm_device *dev, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **obj, unsigned int num_planes) +{ + return drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, + &drm_gem_fb_funcs); +} +EXPORT_SYMBOL_GPL(drm_gem_fb_init); + static struct drm_framebuffer * drm_gem_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd, @@ -61,21 +99,15 @@ drm_gem_fb_alloc(struct drm_device *dev, const struct drm_framebuffer_funcs *funcs) { struct drm_framebuffer *fb; - int ret, i; + int ret; fb = kzalloc(sizeof(*fb), GFP_KERNEL); if (!fb) return ERR_PTR(-ENOMEM); - drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd); - - for (i = 0; i < num_planes; i++) - fb->obj[i] = obj[i]; - - ret = drm_framebuffer_init(dev, fb, funcs); + ret = drm_gem_fb_init_with_funcs(fb, dev, mode_cmd, obj, num_planes, + funcs); if (ret) { - DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n", - ret); kfree(fb); return ERR_PTR(ret); } @@ -124,79 +156,135 @@ int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file, EXPORT_SYMBOL(drm_gem_fb_create_handle); /** - * drm_gem_fb_create_with_funcs() - Helper function for the - * &drm_mode_config_funcs.fb_create - * callback + * drm_gem_fb_lookup() - Helper function for use in + * &drm_mode_config_funcs.fb_create implementations * @dev: DRM device * @file: DRM file that holds the GEM handle(s) backing the framebuffer * @mode_cmd: Metadata from the userspace framebuffer creation request - * @funcs: vtable to be used for the new framebuffer object * - * This function can be used to set &drm_framebuffer_funcs for drivers that need - * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to - * change &drm_framebuffer_funcs. The function does buffer size validation. + * This function can be used to look up the objects for all planes. + * In case an error is returned all the objects are put by the + * function before returning. * * Returns: - * Pointer to a &drm_framebuffer on success or an error pointer on failure. + * Number of planes on success or a negative error code on failure. */ -struct drm_framebuffer * -drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, - const struct drm_mode_fb_cmd2 *mode_cmd, - const struct drm_framebuffer_funcs *funcs) +int drm_gem_fb_lookup(struct drm_device *dev, + struct drm_file *file, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **objs) { const struct drm_format_info *info; - struct drm_gem_object *objs[4]; - struct drm_framebuffer *fb; int ret, i; info = drm_get_format_info(dev, mode_cmd); if (!info) - return ERR_PTR(-EINVAL); + return -EINVAL; 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; - objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); if (!objs[i]) { DRM_DEBUG_KMS("Failed to lookup GEM object\n"); ret = -ENOENT; goto err_gem_object_put; } + } + + return i; + +err_gem_object_put: + for (i--; i >= 0; i--) + drm_gem_object_put_unlocked(objs[i]); + + return ret; +} +EXPORT_SYMBOL_GPL(drm_gem_fb_lookup); + +/** + * drm_gem_fb_size_check() - Helper function for use in + * &drm_mode_config_funcs.fb_create implementations + * @dev: DRM device + * @mode_cmd: Metadata from the userspace framebuffer creation request + * + * This function can be used to verify buffer sizes for all planes. + * It is caller's responsibility to put the objects on failure. + * + * Returns: + * Zero on success or a negative error code on failure. + */ +int drm_gem_fb_size_check(struct drm_device *dev, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **objs) +{ + const struct drm_format_info *info; + int i; + + info = drm_get_format_info(dev, mode_cmd); + if (!info) + return -EINVAL; + + 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; min_size = (height - 1) * mode_cmd->pitches[i] + drm_format_info_min_pitch(info, i, width) + mode_cmd->offsets[i]; - if (objs[i]->size < min_size) { - drm_gem_object_put_unlocked(objs[i]); - ret = -EINVAL; - goto err_gem_object_put; - } + if (objs[i]->size < min_size) + return -EINVAL; } - fb = drm_gem_fb_alloc(dev, mode_cmd, objs, i, funcs); - if (IS_ERR(fb)) { - ret = PTR_ERR(fb); - goto err_gem_object_put; - } + return 0; - return fb; +} +EXPORT_SYMBOL_GPL(drm_gem_fb_size_check); -err_gem_object_put: - for (i--; i >= 0; i--) - drm_gem_object_put_unlocked(objs[i]); +/** + * drm_gem_fb_create_with_funcs() - Helper function for the + * &drm_mode_config_funcs.fb_create + * callback + * @dev: DRM device + * @file: DRM file that holds the GEM handle(s) backing the framebuffer + * @mode_cmd: Metadata from the userspace framebuffer creation request + * @funcs: vtable to be used for the new framebuffer object + * + * This function can be used to set &drm_framebuffer_funcs for drivers that need + * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to + * change &drm_framebuffer_funcs. The function does buffer size validation. + * + * Returns: + * Pointer to a &drm_framebuffer on success or an error pointer on failure. + */ +struct drm_framebuffer * +drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, + const struct drm_mode_fb_cmd2 *mode_cmd, + const struct drm_framebuffer_funcs *funcs) +{ + struct drm_gem_object *objs[4]; + struct drm_framebuffer *fb; + int ret, num_planes; + + ret = drm_gem_fb_lookup(dev, file, mode_cmd, objs); + if (ret < 0) + return ERR_PTR(ret); + num_planes = ret; + + ret = drm_gem_fb_size_check(dev, mode_cmd, objs); + if (ret) + fb = ERR_PTR(ret); + else + fb = drm_gem_fb_alloc(dev, mode_cmd, objs, num_planes, funcs); - return ERR_PTR(ret); + if (IS_ERR(fb)) + for (num_planes--; num_planes >= 0; num_planes--) + drm_gem_object_put_unlocked(objs[num_planes]); + + return fb; } EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_funcs); -static const struct drm_framebuffer_funcs drm_gem_fb_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, -}; - /** * drm_gem_fb_create() - Helper function for the * &drm_mode_config_funcs.fb_create callback diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h index d9f13fd25b0a..c85d4b152e91 100644 --- a/include/drm/drm_gem_framebuffer_helper.h +++ b/include/drm/drm_gem_framebuffer_helper.h @@ -14,10 +14,27 @@ struct drm_simple_display_pipe; struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, unsigned int plane); +int drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb, + struct drm_device *dev, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **obj, + unsigned int num_planes, + const struct drm_framebuffer_funcs *funcs); +int drm_gem_fb_init(struct drm_framebuffer *fb, + struct drm_device *dev, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **obj, unsigned int num_planes); void drm_gem_fb_destroy(struct drm_framebuffer *fb); int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file, unsigned int *handle); +int drm_gem_fb_lookup(struct drm_device *dev, + struct drm_file *file, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **objs); +int drm_gem_fb_size_check(struct drm_device *dev, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object **objs); struct drm_framebuffer * drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd,
Prepare tools for drivers which need to allocate a struct drm_framebuffer (or a container of struct drm_framebuffer) explicitly, before calling helpers. In such a case we need new helpers which omit allocating the struct drm_framebuffer and this patch provides them. Consequently, they are used also inside the helpers themselves. The interested drivers will likely need to be able to perform object lookups and size checks in separate invocations and this patch provides that as well. Helpers themselves are updated, too. Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> --- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 184 ++++++++++++++----- include/drm/drm_gem_framebuffer_helper.h | 17 ++ 2 files changed, 153 insertions(+), 48 deletions(-)