[6/9] drm/verisilicon: Add drm crtc funcs
Commit Message
Add crtc driver which implements crtc related operation functions.
Signed-off-by: Keith Zhao <keith.zhao@starfivetech.com>
---
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
Comments
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@starfivetech.com>
> ---
> 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)
On 2023/6/30 19:55, Thomas Zimmermann wrote:
> 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@starfivetech.com>
>> ---
>> 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.
>
hi Thomas:
I'm trying to understand what you're thinking
1. Different chip ids should have their own independent drm_dev, and should not be supported based on a same drm_dev.
2. diff chip id , for example dc8200 , dc9000,
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);
};
static const struct vs_crtc_funcs vs_dc8200_crtc_funcs = {...}
static const struct vs_crtc_funcs vs_dc9200_crtc_funcs = {...}
struct vs_drm_private {
struct drm_device base;
struct device *dma_dev;
struct iommu_domain *domain;
unsigned int pitch_alignment;
const struct vs_crtc_funcs *funcs;
};
for dc8200 vs_drm_bind to make funcs= &vs_dc8200_crtc_funcs
for dc9000 vs_drm_bind to make funcs= &vs_dc9200_crtc_funcs
so when run dc8200 modesetting pipline
I get drm_dev , get the funcs points , then I can call dc8200 internal interfaces .
Welcome to correct my thoughts.
thanks
>> +
>> +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?
>
hi Thomas ,
this line should be deleted.
Historical reasons It has been compatible with multiple kernel versions
> 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__ */
>
Hi Keith,
On Fri, Jul 21, 2023 at 07:57:24PM +0800, Keith Zhao wrote:
> >> +
> >> +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.
> >
> hi Thomas:
> I'm trying to understand what you're thinking
I am not Thomas, but let me try to put a few words on this.
> 1. Different chip ids should have their own independent drm_dev, and should not be supported based on a same drm_dev.
Yes, this part is correct understood.
> 2. diff chip id , for example dc8200 , dc9000,
>
> 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);
> };
No - the idea is that you populate crtc_funcs direct.
Drop struct vs_crtc_funcs - just fill out your own crtc_funcs structure.
If it turns out that most of the crtc operations are the same then share
them. Avoid the extra layer of indirection that you have with struct vs_crtc_funcs
as this is not needed when you use the pattern described by Thomas.
> static const struct vs_crtc_funcs vs_dc8200_crtc_funcs = {...}
> static const struct vs_crtc_funcs vs_dc9200_crtc_funcs = {...}
>
> struct vs_drm_private {
> struct drm_device base;
> struct device *dma_dev;
> struct iommu_domain *domain;
> unsigned int pitch_alignment;
This parts looks fine.
>
> const struct vs_crtc_funcs *funcs;
No, here you need a pointer to struct crtc_funcs or a struct that embeds
crtc_funcs.
> };
If you, after reading this, thinks you need struct vs_crtc_funcs, then
try to take an extra look at mgag200. It is not needed.
I hope this helps.
Sam
@@ -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
new file mode 100644
@@ -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;
+
+ 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;
+}
new file mode 100644
@@ -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);
+};
+
+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;
+ struct drm_pending_vblank_event *event;
+ unsigned int max_bpc;
+ unsigned int color_formats; /* supported color format */
+
+ 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;
+};
+
+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__ */
new file mode 100644
@@ -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>
+
+#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__ */