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

[Keith Zhao <keith.zhao@starfivetech.com>]

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.