[Linaro-mm-sig] Re: [PATCH v2 5/6] drm/vs: Add KMS crtc&plane

On 2023/10/25 21:57, Maxime Ripard wrote:

On Wed, Oct 25, 2023 at 06:39:56PM +0800, Keith Zhao wrote:

...

+static struct drm_crtc_state * +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) +{

• struct vs_crtc_state *ori_state;
• struct vs_crtc_state *state;
• if (!crtc->state)

• return NULL;
  

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

• return NULL;
  

• __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
• state->output_fmt = ori_state->output_fmt;

That field is never set in your patch.

...

• state->encoder_type = ori_state->encoder_type;

That isn't either, and it's not clear why you would need the encoder_type stored in the CRTC?

...

• state->bpp = ori_state->bpp;

You seem to derive that from output_fmt, it doesn't need to be in the CRTC state.

...

• state->underflow = ori_state->underflow;

Assuming you're setting this from the interrupt handler, it's unsafe, you shouldn't do that. What are you using it for?

I am going to use the crtc_debugfs function for printing. crtc_debugfs will use it But now I'd better delete it

...

+static const struct drm_prop_enum_list vs_sync_mode_enum_list[] = {

• { VS_SINGLE_DC, "single dc mode" },
• { VS_MULTI_DC_PRIMARY, "primary dc for multi dc mode" },
• { VS_MULTI_DC_SECONDARY, "secondary dc for multi dc mode" },

+};

Custom driver properties are a no-go: https://docs.kernel.org/gpu/drm-kms.html#requirements

And

https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements

...

+void vs_dc_enable(struct vs_dc *dc, struct drm_crtc *crtc) +{

• struct vs_crtc_state *crtc_state = to_vs_crtc_state(crtc->state);
• struct drm_display_mode *mode = &crtc->state->adjusted_mode;
• struct dc_hw_display display;

Why are you rolling your own structure here, if it's exactly equivalent to what drm_display_mode and the crtc_state provide?

My original intention was to make the hardware part purer. and want to decouple hardware from drm struct.

so I define the own structure between drm and hardware. Maybe doing this will make both the hardware and drm happy

...

+void vs_dc_commit(struct vs_dc *dc) +{

• dc_hw_enable_shadow_register(&dc->hw, false);
• dc_hw_commit(&dc->hw);
• if (dc->first_frame)

• dc->first_frame = false;
  

• dc_hw_enable_shadow_register(&dc->hw, true);

+}

It's not clear to me what you're trying to do here, does the hardware have latched registers that are only updated during vblank?

...

+static int dc_bind(struct device *dev, struct device *master, void *data) +{

• struct drm_device *drm_dev = data;
• struct vs_dc *dc = dev_get_drvdata(dev);
• struct device_node *port;
• struct vs_crtc *crtc;
• struct vs_dc_info *dc_info;
• struct vs_plane *plane;
• struct vs_plane_info *plane_info;
• int i, ret;
• u32 ctrc_mask = 0;
• if (!drm_dev || !dc) {

• dev_err(dev, "devices are not created.\n");
  

• return -ENODEV;
  

• }
• ret = dc_init(dev);
• if (ret < 0) {

• drm_err(drm_dev, "Failed to initialize DC hardware.\n");
  

• return ret;
  

• }
• port = of_get_child_by_name(dev->of_node, "port");
• if (!port) {

• drm_err(drm_dev, "no port node found\n");
  

• return -ENODEV;
  

• }
• of_node_put(port);
• dc_info = dc->hw.info;
• for (i = 0; i < dc_info->panel_num; i++) {

• crtc = vs_crtc_create(drm_dev, dc_info);
  

• if (!crtc) {
  

• 	drm_err(drm_dev, "Failed to create CRTC.\n");
  

• 	ret = -ENOMEM;
  

• 	return ret;
  

• }
  

• crtc->base.port = port;
  

• crtc->dev = dev;
  

• dc->crtc[i] = crtc;
  

• ctrc_mask |= drm_crtc_mask(&crtc->base);
  

• }
• for (i = 0; i < dc_info->plane_num; i++) {

• plane_info = (struct vs_plane_info *)&dc_info->planes[i];
  

• if (!strcmp(plane_info->name, "Primary") || !strcmp(plane_info->name, "Cursor")) {
  

• 	plane = vs_plane_create(drm_dev, plane_info, dc_info->layer_num,
  

• 				drm_crtc_mask(&dc->crtc[0]->base));
  

• } else if (!strcmp(plane_info->name, "Primary_1") ||
  

• 		   !strcmp(plane_info->name, "Cursor_1")) {
  

Please use an enum and an id there.

...

+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;

+}

Same story than above for properties

Honestly, that driver is pretty massive, and you should be simplifying it a lot of you want the initial contribution to be as smooth as possible.

Things like all the tiling formats, the underflowing handling, all those properties, etc can (and should) be added in a second step once the foundations are in.

Maxime

ok , Thanks for reminding me. I will clarify my next goal and be more likely to simplify features.