Re: [PATCH 6/9] drm/verisilicon: Add drm crtc funcs

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 02.06.23 um 09:40 schrieb Keith Zhao:
> Add crtc driver which implements crtc related operation functions.
>
> Signed-off-by: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/verisilicon/Makefile  |   1 +
>   drivers/gpu/drm/verisilicon/vs_crtc.c | 388 ++++++++++++++++++++++++++
>   drivers/gpu/drm/verisilicon/vs_crtc.h |  74 +++++
>   drivers/gpu/drm/verisilicon/vs_type.h |  72 +++++
>   4 files changed, 535 insertions(+)
>   create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c
>   create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h
>   create mode 100644 drivers/gpu/drm/verisilicon/vs_type.h
>
> diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile
> index 38254dc5d98d..bae5fbab9bbb 100644
> --- a/drivers/gpu/drm/verisilicon/Makefile
> +++ b/drivers/gpu/drm/verisilicon/Makefile
> @@ -1,6 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   
>   vs_drm-objs := vs_drv.o \
> +		vs_crtc.o \
>   		vs_fb.o \
>   		vs_gem.o
>   
> diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c
> new file mode 100644
> index 000000000000..a9e742d7bd1a
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c
> @@ -0,0 +1,388 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/media-bus-format.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_vblank.h>
> +#include <drm/vs_drm.h>
> +
> +#include "vs_crtc.h"
> +
> +void vs_crtc_destroy(struct drm_crtc *crtc)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +
> +	drm_crtc_cleanup(crtc);
> +	kfree(vs_crtc);
> +}
> +
> +static void vs_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct vs_crtc_state *state;
> +
> +	if (crtc->state) {
> +		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> +
> +		state = to_vs_crtc_state(crtc->state);
> +		kfree(state);
> +		crtc->state = NULL;
> +	}
> +
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return;
> +
> +	__drm_atomic_helper_crtc_reset(crtc, &state->base);
> +
> +	state->sync_mode = VS_SINGLE_DC;
> +	state->output_fmt = MEDIA_BUS_FMT_RBG888_1X24;
> +	state->encoder_type = DRM_MODE_ENCODER_NONE;
> +}
> +
> +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 (WARN_ON(!crtc->state))
> +		return NULL;

I'd leave this check out. IIRC, crtc->state not supposed to be NULL 
here. Rather let it crash.

> +
> +	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->sync_mode = ori_state->sync_mode;
> +	state->output_fmt = ori_state->output_fmt;
> +	state->encoder_type = ori_state->encoder_type;
> +	state->bg_color = ori_state->bg_color;
> +	state->bpp = ori_state->bpp;
> +	state->sync_enable = ori_state->sync_enable;
> +	state->dither_enable = ori_state->dither_enable;
> +	state->underflow = ori_state->underflow;
> +
> +	return &state->base;
> +}
> +
> +static void vs_crtc_atomic_destroy_state(struct drm_crtc *crtc,
> +					 struct drm_crtc_state *state)
> +{
> +	__drm_atomic_helper_crtc_destroy_state(state);
> +	kfree(to_vs_crtc_state(state));
> +}
> +
> +static int vs_crtc_atomic_set_property(struct drm_crtc *crtc,
> +				       struct drm_crtc_state *state,
> +				       struct drm_property *property,
> +				       uint64_t val)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +	struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(state);
> +
> +	if (property == vs_crtc->sync_mode)
> +		vs_crtc_state->sync_mode = val;
> +	else if (property == vs_crtc->mmu_prefetch)
> +		vs_crtc_state->mmu_prefetch = val;
> +	else if (property == vs_crtc->bg_color)
> +		vs_crtc_state->bg_color = val;
> +	else if (property == vs_crtc->panel_sync)
> +		vs_crtc_state->sync_enable = val;
> +	else if (property == vs_crtc->dither)
> +		vs_crtc_state->dither_enable = val;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int vs_crtc_atomic_get_property(struct drm_crtc *crtc,
> +				       const struct drm_crtc_state *state,
> +				       struct drm_property *property,
> +				       uint64_t *val)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +	const struct vs_crtc_state *vs_crtc_state =
> +		container_of(state, const struct vs_crtc_state, base);
> +
> +	if (property == vs_crtc->sync_mode)
> +		*val = vs_crtc_state->sync_mode;
> +	else if (property == vs_crtc->mmu_prefetch)
> +		*val = vs_crtc_state->mmu_prefetch;
> +	else if (property == vs_crtc->bg_color)
> +		*val = vs_crtc_state->bg_color;
> +	else if (property == vs_crtc->panel_sync)
> +		*val = vs_crtc_state->sync_enable;
> +	else if (property == vs_crtc->dither)
> +		*val = vs_crtc_state->dither_enable;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int vs_crtc_late_register(struct drm_crtc *crtc)
> +{
> +	return 0;
> +}
> +
> +static int vs_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +
> +	vs_crtc->funcs->enable_vblank(vs_crtc->dev, true);
> +
> +	return 0;
> +}
> +
> +static void vs_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +
> +	vs_crtc->funcs->enable_vblank(vs_crtc->dev, false);
> +}
> +
> +static const struct drm_crtc_funcs vs_crtc_funcs = {
> +	.set_config		= drm_atomic_helper_set_config,
> +	.destroy		= vs_crtc_destroy,
> +	.page_flip		= drm_atomic_helper_page_flip,
> +	.reset			= vs_crtc_reset,
> +	.atomic_duplicate_state = vs_crtc_atomic_duplicate_state,
> +	.atomic_destroy_state	= vs_crtc_atomic_destroy_state,
> +	.atomic_set_property	= vs_crtc_atomic_set_property,
> +	.atomic_get_property	= vs_crtc_atomic_get_property,
> +	.late_register		= vs_crtc_late_register,
> +	.enable_vblank		= vs_crtc_enable_vblank,
> +	.disable_vblank		= vs_crtc_disable_vblank,
> +};
> +
> +static u8 cal_pixel_bits(u32 bus_format)
> +{
> +	u8 bpp;
> +
> +	switch (bus_format) {
> +	case MEDIA_BUS_FMT_RGB565_1X16:
> +	case MEDIA_BUS_FMT_UYVY8_1X16:
> +		bpp = 16;
> +		break;
> +	case MEDIA_BUS_FMT_RGB666_1X18:
> +	case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
> +		bpp = 18;
> +		break;
> +	case MEDIA_BUS_FMT_UYVY10_1X20:
> +		bpp = 20;
> +		break;
> +	case MEDIA_BUS_FMT_BGR888_1X24:
> +	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> +	case MEDIA_BUS_FMT_YUV8_1X24:
> +		bpp = 24;
> +		break;
> +	case MEDIA_BUS_FMT_RGB101010_1X30:
> +	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> +	case MEDIA_BUS_FMT_YUV10_1X30:
> +		bpp = 30;
> +		break;
> +	default:
> +		bpp = 24;
> +		break;
> +	}
> +
> +	return bpp;
> +}
> +
> +static bool vs_crtc_mode_fixup(struct drm_crtc *crtc,
> +			       const struct drm_display_mode *mode,
> +			       struct drm_display_mode *adjusted_mode)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +
> +	return vs_crtc->funcs->mode_fixup(vs_crtc->dev, mode, adjusted_mode);
> +}
> +
> +static void vs_crtc_atomic_enable(struct drm_crtc *crtc,
> +				  struct drm_atomic_state *state)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +	struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state);
> +
> +	vs_crtc_state->bpp = cal_pixel_bits(vs_crtc_state->output_fmt);
> +
> +	vs_crtc->funcs->enable(vs_crtc->dev, crtc);
> +	drm_crtc_vblank_on(crtc);
> +}
> +
> +static void vs_crtc_atomic_disable(struct drm_crtc *crtc,
> +				   struct drm_atomic_state *state)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +
> +	drm_crtc_vblank_off(crtc);
> +
> +	vs_crtc->funcs->disable(vs_crtc->dev, crtc);
> +
> +	if (crtc->state->event && !crtc->state->active) {
> +		spin_lock_irq(&crtc->dev->event_lock);
> +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +		spin_unlock_irq(&crtc->dev->event_lock);
> +
> +		crtc->state->event = NULL;
> +	}
> +}
> +
> +static void vs_crtc_atomic_begin(struct drm_crtc *crtc,
> +				 struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
> +
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +	struct device *dev = vs_crtc->dev;
> +	struct drm_property_blob *blob = crtc->state->gamma_lut;
> +	struct drm_color_lut *lut;
> +
> +	if (crtc_state->color_mgmt_changed) {
> +		if (blob && blob->length) {
> +			lut = blob->data;
> +			vs_crtc->funcs->set_gamma(dev, crtc, lut,
> +						  blob->length / sizeof(*lut));
> +			vs_crtc->funcs->enable_gamma(dev, crtc, true);
> +		} else {
> +			vs_crtc->funcs->enable_gamma(dev, crtc, false);
> +		}
> +	}
> +}
> +
> +static void vs_crtc_atomic_flush(struct drm_crtc *crtc,
> +				 struct drm_atomic_state *state)
> +{
> +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
> +	struct drm_pending_vblank_event *event = crtc->state->event;
> +
> +	vs_crtc->funcs->commit(vs_crtc->dev);
> +
> +	if (event) {
> +		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> +
> +		spin_lock_irq(&crtc->dev->event_lock);
> +		drm_crtc_arm_vblank_event(crtc, event);
> +		spin_unlock_irq(&crtc->dev->event_lock);
> +		crtc->state->event = NULL;
> +	}
> +}
> +
> +static const struct drm_crtc_helper_funcs vs_crtc_helper_funcs = {
> +	.mode_fixup = vs_crtc_mode_fixup,
> +	.atomic_enable	= vs_crtc_atomic_enable,
> +	.atomic_disable = vs_crtc_atomic_disable,
> +	.atomic_begin	= vs_crtc_atomic_begin,
> +	.atomic_flush	= vs_crtc_atomic_flush,
> +};
> +
> +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" },
> +};
> +
> +struct vs_crtc *vs_crtc_create(struct drm_device *drm_dev,
> +			       struct vs_dc_info *info)
> +{
> +	struct vs_crtc *crtc;
> +	int ret;
> +
> +	if (!info)
> +		return NULL;
> +
> +	crtc = kzalloc(sizeof(*crtc), GFP_KERNEL);
> +	if (!crtc)
> +		return NULL;
> +
> +	ret = drm_crtc_init_with_planes(drm_dev, &crtc->base,
> +					NULL, NULL, &vs_crtc_funcs,
> +					info->name ? info->name : NULL);
> +	if (ret)
> +		goto err_free_crtc;
> +
> +	drm_crtc_helper_add(&crtc->base, &vs_crtc_helper_funcs);
> +
> +	/* Set up the crtc properties */
> +	if (info->pipe_sync) {
> +		crtc->sync_mode = drm_property_create_enum(drm_dev, 0,
> +							   "SYNC_MODE",
> +							    vs_sync_mode_enum_list,
> +							    ARRAY_SIZE(vs_sync_mode_enum_list));
> +
> +		if (!crtc->sync_mode)
> +			goto err_cleanup_crts;
> +
> +		drm_object_attach_property(&crtc->base.base,
> +					   crtc->sync_mode,
> +					   VS_SINGLE_DC);
> +	}
> +
> +	if (info->gamma_size) {
> +		ret = drm_mode_crtc_set_gamma_size(&crtc->base,
> +						   info->gamma_size);
> +		if (ret)
> +			goto err_cleanup_crts;
> +
> +		drm_crtc_enable_color_mgmt(&crtc->base, 0, false,
> +					   info->gamma_size);
> +	}
> +
> +	if (info->background) {
> +		crtc->bg_color = drm_property_create_range(drm_dev, 0,
> +							   "BG_COLOR", 0, 0xffffffff);
> +
> +		if (!crtc->bg_color)
> +			goto err_cleanup_crts;
> +
> +		drm_object_attach_property(&crtc->base.base, crtc->bg_color, 0);
> +	}
> +
> +	if (info->panel_sync) {
> +		crtc->panel_sync = drm_property_create_bool(drm_dev, 0, "SYNC_ENABLED");
> +
> +		if (!crtc->panel_sync)
> +			goto err_cleanup_crts;
> +
> +		drm_object_attach_property(&crtc->base.base, crtc->panel_sync, 0);
> +	}
> +
> +	crtc->dither = drm_property_create_bool(drm_dev, 0, "DITHER_ENABLED");
> +	if (!crtc->dither)
> +		goto err_cleanup_crts;
> +
> +	drm_object_attach_property(&crtc->base.base, crtc->dither, 0);
> +
> +	crtc->max_bpc = info->max_bpc;
> +	crtc->color_formats = info->color_formats;
> +	return crtc;
> +
> +err_cleanup_crts:
> +	drm_crtc_cleanup(&crtc->base);
> +
> +err_free_crtc:
> +	kfree(crtc);
> +	return NULL;
> +}
> +
> +void vs_crtc_handle_vblank(struct drm_crtc *crtc, bool underflow)
> +{
> +	struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state);
> +
> +	drm_crtc_handle_vblank(crtc);
> +
> +	vs_crtc_state->underflow = underflow;
> +}
> diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.h b/drivers/gpu/drm/verisilicon/vs_crtc.h
> new file mode 100644
> index 000000000000..33b3b14249ce
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_crtc.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
> + */
> +
> +#ifndef __VS_CRTC_H__
> +#define __VS_CRTC_H__
> +
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +#include "vs_type.h"
> +
> +struct vs_crtc_funcs {
> +	void (*enable)(struct device *dev, struct drm_crtc *crtc);
> +	void (*disable)(struct device *dev, struct drm_crtc *crtc);
> +	bool (*mode_fixup)(struct device *dev,
> +			   const struct drm_display_mode *mode,
> +			   struct drm_display_mode *adjusted_mode);
> +	void (*set_gamma)(struct device *dev, struct drm_crtc *crtc,
> +			  struct drm_color_lut *lut, unsigned int size);
> +	void (*enable_gamma)(struct device *dev, struct drm_crtc *crtc,
> +			     bool enable);
> +	void (*enable_vblank)(struct device *dev, bool enable);
> +	void (*commit)(struct device *dev);
> +};

Why is this here? You are reproducing our interface with an internal 
interface. I know where this leads to: you have multiple chipset 
revisions and each has its own implemenation of these internal interfaces.

That will absolutely come back to haunt you in the long rung: the more 
chip revisions you support, the more obscure these internal interfaces 
and implentations become. And you won't be able to change these 
callbacks, as that affects all revisions. We've seen this with a few 
drivers. It will become unmaintainable.

A better approach is to treat DRM's atomic callback funcs and atomic 
helper funcs as your interface for each chip revision. So for each 
model, you implement a separate modesetting pipeline. When you add a new 
chip revision, you copy the previous chip's code into a new file and 
adopt it. If you find comon code among individual revisions, you can put 
it into a shared helper.  With this design, each chip revision stands on 
its own.

I suggest to study the mgag200 driver. It detects the chip revision very 
early and builds a chip-specific modesetting pipline. Although each chip 
is handled separately, a lot of shared code is in helpers. So the size 
of the driver remains small.

> +
> +struct vs_crtc_state {
> +	struct drm_crtc_state base;
> +
> +	u32 sync_mode;
> +	u32 output_fmt;
> +	u32 bg_color;
> +	u8 encoder_type;
> +	u8 mmu_prefetch;
> +	u8 bpp;
> +	bool sync_enable;
> +	bool dither_enable;
> +	bool underflow;
> +};
> +
> +struct vs_crtc {
> +	struct drm_crtc base;
> +	struct device *dev;

That's stored in base.dev already.

> +	struct drm_pending_vblank_event *event;

That's in drm_crtc_state.event already.

> +	unsigned int max_bpc;
> +	unsigned int color_formats; /* supported color format */

These come from a vs_dc_info. Why not just store a pointer to the info 
instead?

> +
> +	struct drm_property *sync_mode;
> +	struct drm_property *mmu_prefetch;
> +	struct drm_property *bg_color;
> +	struct drm_property *panel_sync;
> +	struct drm_property *dither;
> +
> +	const struct vs_crtc_funcs *funcs;

Please, see my rant why that's not a good idea.

> +};
> +
> +void vs_crtc_destroy(struct drm_crtc *crtc);
> +
> +struct vs_crtc *vs_crtc_create(struct drm_device *drm_dev,
> +			       struct vs_dc_info *info);
> +void vs_crtc_handle_vblank(struct drm_crtc *crtc, bool underflow);
> +
> +static inline struct vs_crtc *to_vs_crtc(struct drm_crtc *crtc)
> +{
> +	return container_of(crtc, struct vs_crtc, base);
> +}
> +
> +static inline struct vs_crtc_state *
> +to_vs_crtc_state(struct drm_crtc_state *state)
> +{
> +	return container_of(state, struct vs_crtc_state, base);
> +}
> +#endif /* __VS_CRTC_H__ */
> diff --git a/drivers/gpu/drm/verisilicon/vs_type.h b/drivers/gpu/drm/verisilicon/vs_type.h
> new file mode 100644
> index 000000000000..6f8db65a703d
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_type.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
> + */
> +
> +#ifndef __VS_TYPE_H__
> +#define __VS_TYPE_H__
> +
> +#include <linux/version.h>

Why?

Best regards
Thomas

> +
> +#include <drm/drm_plane.h>
> +#include <drm/drm_plane_helper.h>
> +
> +struct vs_plane_info {
> +	const char *name;
> +	u8 id;
> +	enum drm_plane_type type;
> +	unsigned int num_formats;
> +	const u32 *formats;
> +	u8 num_modifiers;
> +	const u64 *modifiers;
> +	unsigned int min_width;
> +	unsigned int min_height;
> +	unsigned int max_width;
> +	unsigned int max_height;
> +	unsigned int rotation;
> +	unsigned int blend_mode;
> +	unsigned int color_encoding;
> +
> +	/* 0 means no de-gamma LUT */
> +	unsigned int degamma_size;
> +
> +	int min_scale; /* 16.16 fixed point */
> +	int max_scale; /* 16.16 fixed point */
> +
> +	/* default zorder value,
> +	 * and 255 means unsupported zorder capability
> +	 */
> +	u8	 zpos;
> +
> +	bool watermark;
> +	bool color_mgmt;
> +	bool roi;
> +};
> +
> +struct vs_dc_info {
> +	const char *name;
> +
> +	u8 panel_num;
> +
> +	/* planes */
> +	u8 plane_num;
> +	const struct vs_plane_info *planes;
> +
> +	u8 layer_num;
> +	unsigned int max_bpc;
> +	unsigned int color_formats;
> +
> +	/* 0 means no gamma LUT */
> +	u16 gamma_size;
> +	u8 gamma_bits;
> +
> +	u16 pitch_alignment;
> +
> +	bool pipe_sync;
> +	bool mmu_prefetch;
> +	bool background;
> +	bool panel_sync;
> +	bool cap_dec;
> +};
> +
> +#endif /* __VS_TYPE_H__ */

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature
Description: OpenPGP digital signature