[Linaro-mm-sig] Re: [PATCH 7/9] drm/verisilicon: Add drm plane funcs

Am 02.06.23 um 09:40 schrieb Keith Zhao:

Implement plane functions for the DRM driver.

Signed-off-by: Keith Zhao keith.zhao@starfivetech.com

drivers/gpu/drm/verisilicon/Makefile | 3 +- drivers/gpu/drm/verisilicon/vs_plane.c | 440 +++++++++++++++++++++++++ drivers/gpu/drm/verisilicon/vs_plane.h | 74 +++++ 3 files changed, 516 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h

diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile index bae5fbab9bbb..d96ad9399fc7 100644 --- a/drivers/gpu/drm/verisilicon/Makefile +++ b/drivers/gpu/drm/verisilicon/Makefile @@ -3,7 +3,8 @@ vs_drm-objs := vs_drv.o \ vs_crtc.o \ vs_fb.o \

• vs_gem.o
  

• vs_gem.o \
  

• vs_plane.o
  

obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c new file mode 100644 index 000000000000..7b0dcef232ae --- /dev/null +++ b/drivers/gpu/drm/verisilicon/vs_plane.c @@ -0,0 +1,440 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
• */

+#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_blend.h> +#include <drm/drm_gem_dma_helper.h> +#include <drm/drm_fb_dma_helper.h> +#include <drm/drm_framebuffer.h> +#include <drm/drm_plane_helper.h>

+#include <drm/vs_drm.h>

+#include "vs_crtc.h" +#include "vs_fb.h" +#include "vs_gem.h" +#include "vs_plane.h" +#include "vs_type.h"

+void vs_plane_destory(struct drm_plane *plane)

'_destroy'

+{

• struct vs_plane *vs_plane = to_vs_plane(plane);
• drm_plane_cleanup(plane);
• kfree(vs_plane);

+}

I think we have helpers to do the cleanup.

As I mentioned in another review, you should switch to managed code instead of using your own memory management. If your modesetting pipeline is static, you can use drmm_mode_config_init() and it will release all pipeline structs for you. The related memory can allowed as part of the device instance. If the pipeline is dynamic, you can use drmm_ helpers to allocate conenctors, crtcs, planes, etc. IIRC, the vc4 driver does something like that.

+static void vs_plane_reset(struct drm_plane *plane) +{

• struct vs_plane_state *state;
• struct vs_plane *vs_plane = to_vs_plane(plane);
• if (plane->state) {

• __drm_atomic_helper_plane_destroy_state(plane->state);
  

• state = to_vs_plane_state(plane->state);
  

• kfree(state);
  

• plane->state = NULL;
  

• }
• state = kzalloc(sizeof(*state), GFP_KERNEL);
• if (!state)

• return;
  

• __drm_atomic_helper_plane_reset(plane, &state->base);
• state->degamma = VS_DEGAMMA_DISABLE;
• state->degamma_changed = false;
• state->base.zpos = vs_plane->id;
• memset(&state->status, 0, sizeof(state->status));

Should already be 0 from kzalloc().

+}

+static void _vs_plane_duplicate_blob(struct vs_plane_state *state,

• 		     struct vs_plane_state *ori_state)
  

+{

• state->watermark = ori_state->watermark;
• state->color_mgmt = ori_state->color_mgmt;
• state->roi = ori_state->roi;
• if (state->watermark)

• drm_property_blob_get(state->watermark);
  

• if (state->color_mgmt)

• drm_property_blob_get(state->color_mgmt);
  

• if (state->roi)

• drm_property_blob_get(state->roi);
  

+}

+static int +_vs_plane_set_property_blob_from_id(struct drm_device *dev,

• 		    struct drm_property_blob **blob,
  

• 		    u64 blob_id,
  

• 		    size_t expected_size)
  

+{

• struct drm_property_blob *new_blob = NULL;
• if (blob_id) {

• new_blob = drm_property_lookup_blob(dev, blob_id);
  

• if (!new_blob)
  

• 	return -EINVAL;
  

• if (new_blob->length != expected_size) {
  

• 	drm_property_blob_put(new_blob);
  

• 	return -EINVAL;
  

• }
  

• }
• drm_property_replace_blob(blob, new_blob);
• drm_property_blob_put(new_blob);
• return 0;

+}

+static struct drm_plane_state * +vs_plane_atomic_duplicate_state(struct drm_plane *plane) +{

• struct vs_plane_state *ori_state;
• struct vs_plane_state *state;
• if (WARN_ON(!plane->state))

• return NULL;
  

• ori_state = to_vs_plane_state(plane->state);
• state = kzalloc(sizeof(*state), GFP_KERNEL);
• if (!state)

• return NULL;
  

• __drm_atomic_helper_plane_duplicate_state(plane, &state->base);
• state->degamma = ori_state->degamma;
• state->degamma_changed = ori_state->degamma_changed;
• _vs_plane_duplicate_blob(state, ori_state);
• memcpy(&state->status, &ori_state->status, sizeof(ori_state->status));
• return &state->base;

+}

+static void vs_plane_atomic_destroy_state(struct drm_plane *plane,

• 			  struct drm_plane_state *state)
  

+{

• struct vs_plane_state *vs_plane_state = to_vs_plane_state(state);
• __drm_atomic_helper_plane_destroy_state(state);
• drm_property_blob_put(vs_plane_state->watermark);
• drm_property_blob_put(vs_plane_state->color_mgmt);
• drm_property_blob_put(vs_plane_state->roi);
• kfree(vs_plane_state);

+}

+static int vs_plane_atomic_set_property(struct drm_plane *plane,

• 			struct drm_plane_state *state,
  

• 			struct drm_property *property,
  

• 			uint64_t val)
  

+{

• struct drm_device *dev = plane->dev;
• struct vs_plane *vs_plane = to_vs_plane(plane);
• struct vs_plane_state *vs_plane_state = to_vs_plane_state(state);
• int ret = 0;
• if (property == vs_plane->degamma_mode) {

• if (vs_plane_state->degamma != val) {
  

• 	vs_plane_state->degamma = val;
  

• 	vs_plane_state->degamma_changed = true;
  

• } else {
  

• 	vs_plane_state->degamma_changed = false;
  

• }
  

• } else if (property == vs_plane->watermark_prop) {

• ret = _vs_plane_set_property_blob_from_id(dev,
  

• 					  &vs_plane_state->watermark,
  

• 					  val,
  

• 					  sizeof(struct drm_vs_watermark));
  

• return ret;
  

• } else if (property == vs_plane->color_mgmt_prop) {

• ret = _vs_plane_set_property_blob_from_id(dev,
  

• 					  &vs_plane_state->color_mgmt,
  

• 					  val,
  

• 					  sizeof(struct drm_vs_color_mgmt));
  

• return ret;
  

• } else if (property == vs_plane->roi_prop) {

• ret = _vs_plane_set_property_blob_from_id(dev,
  

• 					  &vs_plane_state->roi,
  

• 					  val,
  

• 					  sizeof(struct drm_vs_roi));
  

• return ret;
  

• } else {

• return -EINVAL;
  

• }
• return 0;

+}

+static int vs_plane_atomic_get_property(struct drm_plane *plane,

• 			const struct drm_plane_state *state,
  

• 			struct drm_property *property,
  

• 			uint64_t *val)
  

+{

• struct vs_plane *vs_plane = to_vs_plane(plane);
• const struct vs_plane_state *vs_plane_state =

• container_of(state, const struct vs_plane_state, base);
  

• if (property == vs_plane->degamma_mode)

• *val = vs_plane_state->degamma;
  

• else if (property == vs_plane->watermark_prop)

• *val = (vs_plane_state->watermark) ?
  

• 			vs_plane_state->watermark->base.id : 0;
  

• else if (property == vs_plane->color_mgmt_prop)

• *val = (vs_plane_state->color_mgmt) ?
  

• 			vs_plane_state->color_mgmt->base.id : 0;
  

• else if (property == vs_plane->roi_prop)

• *val = (vs_plane_state->roi) ?
  

• 			vs_plane_state->roi->base.id : 0;
  

• else

• return -EINVAL;
  

• return 0;

+}

+static bool vs_format_mod_supported(struct drm_plane *plane,

• 		    u32 format,
  

• 		    u64 modifier)
  

+{

• int i;

•   /* We always have to allow these modifiers:
  

Indention seems off.

• • 1. Core DRM checks for LINEAR support if userspace does not provide modifiers.
• • 2. Not passing any modifiers is the same as explicitly passing INVALID.
• */
• if (modifier == DRM_FORMAT_MOD_LINEAR)

• return true;
  

• /* Check that the modifier is on the list of the plane's supported modifiers. */
• for (i = 0; i < plane->modifier_count; i++) {

• if (modifier == plane->modifiers[i])
  

• 	break;
  

• }
• if (i == plane->modifier_count)

• return false;
  

• return true;

+}

+const struct drm_plane_funcs vs_plane_funcs = {

• .update_plane = drm_atomic_helper_update_plane,
• .disable_plane = drm_atomic_helper_disable_plane,
• .destroy = vs_plane_destory,
• .reset = vs_plane_reset,
• .atomic_duplicate_state = vs_plane_atomic_duplicate_state,
• .atomic_destroy_state = vs_plane_atomic_destroy_state,
• .atomic_set_property = vs_plane_atomic_set_property,
• .atomic_get_property = vs_plane_atomic_get_property,
• .format_mod_supported = vs_format_mod_supported,

+};

+static unsigned char vs_get_plane_number(struct drm_framebuffer *fb) +{

• const struct drm_format_info *info;
• if (!fb)

• return 0;
  

• info = drm_format_info(fb->format->format);
• if (!info || info->num_planes > MAX_NUM_PLANES)

• return 0;
  

• return info->num_planes;

+}

+static int vs_plane_atomic_check(struct drm_plane *plane,

• 		 struct drm_atomic_state *state)
  

+{

• struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,

• 								 plane);
  

• struct vs_plane *vs_plane = to_vs_plane(plane);
• struct drm_framebuffer *fb = new_plane_state->fb;
• struct drm_crtc *crtc = new_plane_state->crtc;
• struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
• if (!crtc || !fb)

• return 0;
  

• //return vs_plane->funcs->check(vs_crtc->dev, vs_plane, new_plane_state);
• return vs_plane->funcs->check(vs_crtc->dev, plane, state);

+}

+static void vs_plane_atomic_update(struct drm_plane *plane,

• 		   struct drm_atomic_state *state)
  

+{

• struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,

• 							   plane);
  

• unsigned char i, num_planes;
• struct drm_framebuffer *fb;
• struct vs_plane *vs_plane = to_vs_plane(plane);
• //struct drm_plane_state *state = plane->state;
• struct vs_crtc *vs_crtc = to_vs_crtc(new_state->crtc);
• struct vs_plane_state *plane_state = to_vs_plane_state(new_state);
• //struct drm_format_name_buf *name = &plane_state->status.format_name;
• if (!new_state->fb || !new_state->crtc)

• return;
  

• fb = new_state->fb;
• num_planes = vs_get_plane_number(fb);
• for (i = 0; i < num_planes; i++) {

• struct vs_gem_object *vs_obj;
  

• vs_obj = vs_fb_get_gem_obj(fb, i);
  

• vs_plane->dma_addr[i] = vs_obj->iova + fb->offsets[i];
  

• }
• plane_state->status.src = drm_plane_state_src(new_state);
• plane_state->status.dest = drm_plane_state_dest(new_state);
• vs_plane->funcs->update(vs_crtc->dev, vs_plane, plane, state);

+}

+static void vs_plane_atomic_disable(struct drm_plane *plane,

• 		    struct drm_atomic_state *state)
  

+{

• struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,

• 								   plane);
  

• struct vs_plane *vs_plane = to_vs_plane(plane);
• struct vs_crtc *vs_crtc = to_vs_crtc(old_state->crtc);
• vs_plane->funcs->disable(vs_crtc->dev, vs_plane, old_state);

+}

+const struct drm_plane_helper_funcs vs_plane_helper_funcs = {

• .atomic_check = vs_plane_atomic_check,
• .atomic_update = vs_plane_atomic_update,
• .atomic_disable = vs_plane_atomic_disable,

+};

+static const struct drm_prop_enum_list vs_degamma_mode_enum_list[] = {

• { VS_DEGAMMA_DISABLE, "disabled" },
• { VS_DEGAMMA_BT709, "preset degamma for BT709" },
• { VS_DEGAMMA_BT2020, "preset degamma for BT2020" },

+};

+struct vs_plane *vs_plane_create(struct drm_device *drm_dev,

• 		 struct vs_plane_info *info,
  

• 		 unsigned int layer_num,
  

• 		 unsigned int possible_crtcs)
  

+{

• struct vs_plane *plane;
• int ret;
• if (!info)

• return NULL;
  

• plane = kzalloc(sizeof(*plane), GFP_KERNEL);
• if (!plane)

• return NULL;
  

• ret = drm_universal_plane_init(drm_dev, &plane->base, possible_crtcs,

• 		       &vs_plane_funcs, info->formats,
  

• 		       info->num_formats, info->modifiers, info->type,
  

• 		       info->name ? info->name : NULL);
  

• if (ret)

• goto err_free_plane;
  

• drm_plane_helper_add(&plane->base, &vs_plane_helper_funcs);
• /* Set up the plane properties */
• if (info->degamma_size) {

• plane->degamma_mode =
  

• drm_property_create_enum(drm_dev, 0,
  

• 			 "DEGAMMA_MODE",
  

• 			 vs_degamma_mode_enum_list,
  

• 			 ARRAY_SIZE(vs_degamma_mode_enum_list));
  

• if (!plane->degamma_mode)
  

• 	goto error_cleanup_plane;
  

• drm_object_attach_property(&plane->base.base,
  

• 			   plane->degamma_mode,
  

• 			   VS_DEGAMMA_DISABLE);
  

• }
• if (info->rotation) {

• ret = drm_plane_create_rotation_property(&plane->base,
  

• 					 DRM_MODE_ROTATE_0,
  

• 					 info->rotation);
  

• if (ret)
  

• 	goto error_cleanup_plane;
  

• }
• if (info->blend_mode) {

• ret = drm_plane_create_blend_mode_property(&plane->base,
  

• 					   info->blend_mode);
  

• if (ret)
  

• 	goto error_cleanup_plane;
  

• ret = drm_plane_create_alpha_property(&plane->base);
  

• if (ret)
  

• 	goto error_cleanup_plane;
  

• }
• if (info->color_encoding) {

• ret = drm_plane_create_color_properties(&plane->base,
  

• 					info->color_encoding,
  

• 					BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
  

• 					DRM_COLOR_YCBCR_BT709,
  

• 					DRM_COLOR_YCBCR_LIMITED_RANGE);
  

• if (ret)
  

• 	goto error_cleanup_plane;
  

• }
• if (info->zpos != 255) {

• ret = drm_plane_create_zpos_property(&plane->base, info->zpos, 0,
  

• 				     layer_num - 1);
  

• if (ret)
  

• 	goto error_cleanup_plane;
  

• } else {

• ret = drm_plane_create_zpos_immutable_property(&plane->base,
  

• 					       info->zpos);
  

• if (ret)
  

• 	goto error_cleanup_plane;
  

• }
• if (info->watermark) {

• plane->watermark_prop = drm_property_create(drm_dev, DRM_MODE_PROP_BLOB,
  

• 					    "WATERMARK", 0);
  

• if (!plane->watermark_prop)
  

• 	goto error_cleanup_plane;
  

• drm_object_attach_property(&plane->base.base, plane->watermark_prop, 0);
  

• }
• if (info->color_mgmt) {

• plane->color_mgmt_prop = drm_property_create(drm_dev, DRM_MODE_PROP_BLOB,
  

• 					     "COLOR_CONFIG", 0);
  

• if (!plane->color_mgmt_prop)
  

• 	goto error_cleanup_plane;
  

• drm_object_attach_property(&plane->base.base, plane->color_mgmt_prop, 0);
  

• }
• if (info->roi) {

• plane->roi_prop = drm_property_create(drm_dev, DRM_MODE_PROP_BLOB,
  

• 				      "ROI", 0);
  

• if (!plane->roi_prop)
  

• 	goto error_cleanup_plane;
  

• drm_object_attach_property(&plane->base.base, plane->roi_prop, 0);
  

• }
• return plane;

+error_cleanup_plane:

• drm_plane_cleanup(&plane->base);

+err_free_plane:

• kfree(plane);
• return NULL;

+} diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h new file mode 100644 index 000000000000..76ef3c3de045 --- /dev/null +++ b/drivers/gpu/drm/verisilicon/vs_plane.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
• */

+#ifndef __VS_PLANE_H__ +#define __VS_PLANE_H__

+#include <drm/drm_fourcc.h> +#include <drm/drm_plane_helper.h>

+#include "vs_fb.h" +#include "vs_type.h"

+struct vs_plane;

+struct vs_plane_funcs {

• void (*update)(struct device *dev, struct vs_plane *plane, struct drm_plane *drm_plane,

•        struct drm_atomic_state *state);
  

• void (*disable)(struct device *dev, struct vs_plane *plane,

• 	struct drm_plane_state *old_state);
  

• int (*check)(struct device *dev, struct drm_plane *plane,

•      struct drm_atomic_state *state);
  

+};

Your own internal API. Exactly the same problems as in the CRTC case.

+struct vs_plane_status {

• u32 tile_mode;
• struct drm_rect src;
• struct drm_rect dest;

+};

+struct vs_plane_state {

• struct drm_plane_state base;
• struct vs_plane_status status; /* for debugfs */
• struct drm_property_blob *watermark;
• struct drm_property_blob *color_mgmt;
• struct drm_property_blob *roi;
• u32 degamma;
• bool degamma_changed;

+};

+struct vs_plane {

• struct drm_plane base;
• u8 id;

I'm not sure what this is for. It's used for zpos. If that's the purpose, I'd rather call it default_zpos.

Best regards Thomas

• dma_addr_t dma_addr[MAX_NUM_PLANES];
• struct drm_property *degamma_mode;
• struct drm_property *watermark_prop;
• struct drm_property *color_mgmt_prop;
• struct drm_property *roi_prop;
• const struct vs_plane_funcs *funcs;

+};

+void vs_plane_destory(struct drm_plane *plane);

+struct vs_plane *vs_plane_create(struct drm_device *drm_dev,

• 		 struct vs_plane_info *info,
  

• 		 unsigned int layer_num,
  

• 		 unsigned int possible_crtcs);
  

+static inline struct vs_plane *to_vs_plane(struct drm_plane *plane) +{

• return container_of(plane, struct vs_plane, base);

+}

+static inline struct vs_plane_state * +to_vs_plane_state(struct drm_plane_state *state) +{

• return container_of(state, struct vs_plane_state, base);

+} +#endif /* __VS_PLANE_H__ */