Re: [RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller
From: Neil Armstrong
Date: Mon Nov 28 2016 - 04:35:14 EST
Hi Daniel,
On 11/28/2016 09:16 AM, Daniel Vetter wrote:
> On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote:
>> The Amlogic Meson Display controller is composed of several components :
>>
>> DMC|---------------VPU (Video Processing Unit)----------------|------HHI------|
>> | vd1 _______ _____________ _________________ | |
>> D |-------| |----| | | | | HDMI PLL |
>> D | vd2 | VIU | | Video Post | | Video Encoders |<---|-----VCLK |
>> R |-------| |----| Processing | | | | |
>> | osd2 | | | |---| Enci ----------|----|-----VDAC------|
>> R |-------| CSC |----| Scalers | | Encp ----------|----|----HDMI-TX----|
>> A | osd1 | | | Blenders | | Encl ----------|----|---------------|
>> M |-------|______|----|____________| |________________| | |
>> ___|__________________________________________________________|_______________|
>>
>>
>> VIU: Video Input Unit
>> ---------------------
>>
>> The Video Input Unit is in charge of the pixel scanout from the DDR memory.
>> It fetches the frames addresses, stride and parameters from the "Canvas" memory.
>> This part is also in charge of the CSC (Colorspace Conversion).
>> It can handle 2 OSD Planes and 2 Video Planes.
>>
>> VPP: Video Processing Unit
>> --------------------------
>>
>> The Video Processing Unit is in charge if the scaling and blending of the
>> various planes into a single pixel stream.
>> There is a special "pre-blending" used by the video planes with a dedicated
>> scaler and a "post-blending" to merge with the OSD Planes.
>> The OSD planes also have a dedicated scaler for one of the OSD.
>>
>> VENC: Video Encoders
>> --------------------
>>
>> The VENC is composed of the multiple pixel encoders :
>> - ENCI : Interlace Video encoder for CVBS and Interlace HDMI
>> - ENCP : Progressive Video Encoder for HDMI
>> - ENCL : LCD LVDS Encoder
>> The VENC Unit gets a Pixel Clocks (VCLK) from a dedicated HDMI PLL and clock
>> tree and provides the scanout clock to the VPP and VIU.
>> The ENCI is connected to a single VDAC for Composite Output.
>> The ENCI and ENCP are connected to an on-chip HDMI Transceiver.
>>
>> This driver is a DRM/KMS driver using the following DRM components :
>> - GEM-CMA
>> - PRIME-CMA
>> - Atomic Modesetting
>> - FBDev-CMA
>>
>> For the following SoCs :
>> - GXBB Family (S905)
>> - GXL Family (S905X, S905D)
>> - GXM Family (S912)
>>
>> The current driver only supports the CVBS PAL/NTSC output modes, but the
>> CRTC/Planes management should support bigger modes.
>> But Advanced Colorspace Conversion, Scaling and HDMI Modes will be added in
>> a second time.
>>
>> The Device Tree bindings makes use of the endpoints video interface definitions
>> to connect to the optional CVBS and in the future the HDMI Connector nodes.
>>
>> HDMI Support is planned for a next release.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>
> Few small comments below, but looks reasonable overall. Once you have acks
> for the DT part pls submit the entire series as a pull request to Dave
> Airlie (with an additional patch to add a MAINTAINERS entry).
Thanks for the review.
Ok, will add the MAINTAINERS entry.
>
> Cheers, Daniel
>
>> ---
>> drivers/gpu/drm/Kconfig | 2 +
>> drivers/gpu/drm/Makefile | 1 +
>> drivers/gpu/drm/meson/Kconfig | 8 +
>> drivers/gpu/drm/meson/Makefile | 5 +
>> drivers/gpu/drm/meson/meson_canvas.c | 96 +++
>> drivers/gpu/drm/meson/meson_canvas.h | 31 +
>> drivers/gpu/drm/meson/meson_crtc.c | 176 ++++
>> drivers/gpu/drm/meson/meson_crtc.h | 34 +
>> drivers/gpu/drm/meson/meson_cvbs.c | 293 +++++++
>> drivers/gpu/drm/meson/meson_cvbs.h | 32 +
>> drivers/gpu/drm/meson/meson_drv.c | 383 +++++++++
>> drivers/gpu/drm/meson/meson_drv.h | 68 ++
>> drivers/gpu/drm/meson/meson_plane.c | 150 ++++
>> drivers/gpu/drm/meson/meson_plane.h | 32 +
>> drivers/gpu/drm/meson/meson_registers.h | 1395 +++++++++++++++++++++++++++++++
>> drivers/gpu/drm/meson/meson_vclk.c | 169 ++++
>> drivers/gpu/drm/meson/meson_vclk.h | 36 +
>> drivers/gpu/drm/meson/meson_venc.c | 286 +++++++
>> drivers/gpu/drm/meson/meson_venc.h | 77 ++
>> drivers/gpu/drm/meson/meson_viu.c | 497 +++++++++++
>> drivers/gpu/drm/meson/meson_viu.h | 37 +
>> drivers/gpu/drm/meson/meson_vpp.c | 189 +++++
>> drivers/gpu/drm/meson/meson_vpp.h | 43 +
>> 23 files changed, 4040 insertions(+)
>> create mode 100644 drivers/gpu/drm/meson/Kconfig
>> create mode 100644 drivers/gpu/drm/meson/Makefile
>> create mode 100644 drivers/gpu/drm/meson/meson_canvas.c
>> create mode 100644 drivers/gpu/drm/meson/meson_canvas.h
>> create mode 100644 drivers/gpu/drm/meson/meson_crtc.c
>> create mode 100644 drivers/gpu/drm/meson/meson_crtc.h
>> create mode 100644 drivers/gpu/drm/meson/meson_cvbs.c
>> create mode 100644 drivers/gpu/drm/meson/meson_cvbs.h
>> create mode 100644 drivers/gpu/drm/meson/meson_drv.c
>> create mode 100644 drivers/gpu/drm/meson/meson_drv.h
>> create mode 100644 drivers/gpu/drm/meson/meson_plane.c
>> create mode 100644 drivers/gpu/drm/meson/meson_plane.h
>> create mode 100644 drivers/gpu/drm/meson/meson_registers.h
>> create mode 100644 drivers/gpu/drm/meson/meson_vclk.c
>> create mode 100644 drivers/gpu/drm/meson/meson_vclk.h
>> create mode 100644 drivers/gpu/drm/meson/meson_venc.c
>> create mode 100644 drivers/gpu/drm/meson/meson_venc.h
>> create mode 100644 drivers/gpu/drm/meson/meson_viu.c
>> create mode 100644 drivers/gpu/drm/meson/meson_viu.h
>> create mode 100644 drivers/gpu/drm/meson/meson_vpp.c
>> create mode 100644 drivers/gpu/drm/meson/meson_vpp.h
>>
[...]
>> +
>> +static void meson_crtc_atomic_begin(struct drm_crtc *crtc,
>> + struct drm_crtc_state *state)
>> +{
>> + struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
>> + unsigned long flags;
>> +
>> + if (crtc->state->event) {
>> + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>> +
>> + spin_lock_irqsave(&crtc->dev->event_lock, flags);
>> + meson_crtc->event = crtc->state->event;
>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>> + crtc->state->event = NULL;
>
> If you set this to NULL here
>> + }
>> +}
>> +
>> +static void meson_crtc_atomic_flush(struct drm_crtc *crtc,
>> + struct drm_crtc_state *old_crtc_state)
>> +{
>> + struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
>> + struct drm_pending_vblank_event *event = crtc->state->event;
>> +
>> + if (meson_crtc->priv->viu.osd1_enabled)
>> + meson_crtc->priv->viu.osd1_commit = true;
>> +
>> + if (event) {
>> + crtc->state->event = NULL;
>> +
>> + spin_lock_irq(&crtc->dev->event_lock);
>> + if (drm_crtc_vblank_get(crtc) == 0)
>> + drm_crtc_arm_vblank_event(crtc, event);
>> + else
>> + drm_crtc_send_vblank_event(crtc, event);
>> + spin_unlock_irq(&crtc->dev->event_lock);
>> + }
>
> This here becomes dead code. And indeed it is, since you have your own
> special crtc/vblank irq handling code right below. Please remove to avoid
> confusion.
Ok, will clarify.
>
>> +}
>> +
>> +static const struct drm_crtc_helper_funcs meson_crtc_helper_funcs = {
>> + .enable = meson_crtc_enable,
>> + .disable = meson_crtc_disable,
>> + .atomic_begin = meson_crtc_atomic_begin,
>> + .atomic_flush = meson_crtc_atomic_flush,
>> +};
>> +
>> +void meson_crtc_irq(struct meson_drm *priv)
>> +{
>> + struct meson_crtc *meson_crtc = to_meson_crtc(priv->crtc);
>> + unsigned long flags;
>> +
>> + meson_viu_sync_osd1(priv);
>> +
>> + drm_crtc_handle_vblank(priv->crtc);
>> +
>> + spin_lock_irqsave(&priv->drm->event_lock, flags);
>> + if (meson_crtc->event) {
>> + drm_crtc_send_vblank_event(priv->crtc, meson_crtc->event);
>> + drm_crtc_vblank_put(priv->crtc);
>> + meson_crtc->event = NULL;
>> + }
>> + spin_unlock_irqrestore(&priv->drm->event_lock, flags);
>> +}
>> +
[...]
>> +
>> +static int meson_cvbs_encoder_atomic_check(struct drm_encoder *encoder,
>> + struct drm_crtc_state *crtc_state,
>> + struct drm_connector_state *conn_state)
>> +{
>> + return 0;
>> +}
>
> Dummy atomic_check isn't needed, pls remove.
OK, with your following comments, will replace with a proper mode check here.
>> +
>> +static void meson_cvbs_encoder_disable(struct drm_encoder *encoder)
>> +{
>> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder);
>> +
>> + meson_venci_cvbs_disable(meson_cvbs->priv);
>> +}
>> +
>> +static void meson_cvbs_encoder_enable(struct drm_encoder *encoder)
>> +{
>> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder);
>> +
>> + meson_venci_cvbs_enable(meson_cvbs->priv);
>> +}
>
> Personally I'd remove the indirection above, more direct code is easier to
> read.
I understand, I'll maybe change the meson_venci_cvbs_XXable to be directly added to the ops.
I want to keep the registers setup in separate files and keep a clean DRM/HW separation.
>> +
>> +static void meson_cvbs_encoder_mode_set(struct drm_encoder *encoder,
>> + struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode)
>> +{
>> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder);
>> + int i;
>> +
>> + drm_mode_debug_printmodeline(mode);
>> +
>> + for (i = 0; i < ARRAY_SIZE(meson_cvbs_modes); ++i) {
>> + struct meson_cvbs_mode *meson_mode = &meson_cvbs_modes[i];
>> +
>> + if (drm_mode_equal(mode, &meson_mode->mode)) {
>> + meson_cvbs->mode = meson_mode;
>> +
>> + meson_venci_cvbs_mode_set(meson_cvbs->priv,
>> + meson_mode->enci);
>> + break;
>> + }
>> + }
>> +}
>
> What happens if userspace sets a mode you don't have? I guess you do need
> a real atomic_check, so you can return -EINVAL if that's the case ;-)
Will add a proper atomic_check !
>
>> +
>> +static const struct drm_encoder_helper_funcs meson_cvbs_encoder_helper_funcs = {
>> + .atomic_check = meson_cvbs_encoder_atomic_check,
>> + .disable = meson_cvbs_encoder_disable,
>> + .enable = meson_cvbs_encoder_enable,
>> + .mode_set = meson_cvbs_encoder_mode_set,
>> +};
>> +
>> +/* Connector */
>> +
>> +static void meson_cvbs_connector_destroy(struct drm_connector *connector)
>> +{
>> + drm_connector_cleanup(connector);
>> +}
>> +
>> +static enum drm_connector_status
>> +meson_cvbs_connector_detect(struct drm_connector *connector, bool force)
>> +{
>
> FIXME: Implement load-detect?
Actually it's not implemented anywhere on Amlogic SoCs, will add a FIXME comment here !
>
>> + return connector_status_connected;
>> +}
>> +
>> +static int meson_cvbs_connector_get_modes(struct drm_connector *connector)
>> +{
>> + struct drm_device *dev = connector->dev;
>> + struct drm_display_mode *mode;
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(meson_cvbs_modes); ++i) {
>> + struct meson_cvbs_mode *meson_mode = &meson_cvbs_modes[i];
>> +
>> + mode = drm_mode_duplicate(dev, &meson_mode->mode);
>> + if (!mode) {
>> + DRM_ERROR("Failed to create a new display mode\n");
>> + return 0;
>> + }
>> +
>> + drm_mode_probed_add(connector, mode);
>> + }
>> +
>> + return i;
>> +}
>> +
>> +static int meson_cvbs_connector_mode_valid(struct drm_connector *connector,
>> + struct drm_display_mode *mode)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(meson_cvbs_modes); ++i) {
>> + struct meson_cvbs_mode *meson_mode = &meson_cvbs_modes[i];
>> +
>> + if (drm_mode_equal(mode, &meson_mode->mode))
>> + return MODE_OK;
>> + }
>> +
>> + return MODE_BAD;
>> +}
>
> Ok, this is confusion, but I thought the docs explain this. mode_valid is
> only to validate the modes added in get_modes. This is useful for outputs
> which add modes from an EDID, but not in this case. Having a mode_valid
> unfortunately doesn't ensure that these modes will be rejected in a
> modeset, for that you need a separate mode_fixup or atomic_check on the
> encoder.
>
> It's a bit a long-standing issue, but not entirely non-trivial to fix up:
> In the general case the atomic_check is for a specific configuration,
> whereaas mode_valid must only filter modes that won't work in any
> configuration. For display blocks with lots of shared resources there's a
> big difference between the two.
>
> Please double-check the kerneldoc for all these hooks, and if this is not
> clearly enough explained for you then pls raise this (or even better,
> submit at patch).
OK, for now it seems quite clear, but I clearly missed the atomic_check case.
>
>> +
>> +static const struct drm_connector_funcs meson_cvbs_connector_funcs = {
>> + .dpms = drm_atomic_helper_connector_dpms,
>> + .detect = meson_cvbs_connector_detect,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .destroy = meson_cvbs_connector_destroy,
>> + .reset = drm_atomic_helper_connector_reset,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
[...]
>> +
>> +static int meson_drv_bind(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct meson_drm *priv;
>> + struct drm_device *drm;
>> + struct resource *res;
>> + void __iomem *regs;
>> + int ret;
>> +
>> + drm = drm_dev_alloc(&meson_driver, dev);
>> + if (IS_ERR(drm))
>> + return PTR_ERR(drm);
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv) {
>> + ret = -ENOMEM;
>> + goto free_drm;
>> + }
>> + drm->dev_private = priv;
>> + priv->drm = drm;
>> + priv->dev = dev;
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base");
>> + regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(regs))
>> + return PTR_ERR(regs);
>> +
>> + priv->io_base = regs;
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hhi");
>> + /* Simply ioremap since it may be a shared register zone */
>> + regs = devm_ioremap(dev, res->start, resource_size(res));
>> + if (!regs)
>> + return -EADDRNOTAVAIL;
>> +
>> + priv->hhi = devm_regmap_init_mmio(dev, regs,
>> + &meson_regmap_config);
>> + if (IS_ERR(priv->hhi)) {
>> + dev_err(&pdev->dev, "Couldn't create the HHI regmap\n");
>> + return PTR_ERR(priv->hhi);
>> + }
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dmc");
>> + /* Simply ioremap since it may be a shared register zone */
>> + regs = devm_ioremap(dev, res->start, resource_size(res));
>> + if (!regs)
>> + return -EADDRNOTAVAIL;
>> +
>> + priv->dmc = devm_regmap_init_mmio(dev, regs,
>> + &meson_regmap_config);
>> + if (IS_ERR(priv->dmc)) {
>> + dev_err(&pdev->dev, "Couldn't create the DMC regmap\n");
>> + return PTR_ERR(priv->dmc);
>> + }
>> +
>> + priv->vsync_irq = platform_get_irq(pdev, 0);
>> +
>> + /* Hardware Initialization */
>> +
>> + meson_vpp_init(priv);
>> + meson_viu_init(priv);
>> + meson_venc_init(priv);
>> +
>> + drm_vblank_init(drm, 1);
>> + drm_mode_config_init(drm);
>> +
>> + /* Components Initialization */
>> +
>> + ret = component_bind_all(drm->dev, drm);
>> + if (ret) {
>> + dev_err(drm->dev, "Couldn't bind all components\n");
>> + goto free_drm;
>> + }
>> +
>> + ret = meson_plane_create(priv);
>> + if (ret)
>> + goto free_drm;
>> +
>> + ret = meson_crtc_create(priv);
>> + if (ret)
>> + goto free_drm;
>> +
>> + ret = drm_irq_install(drm, priv->vsync_irq);
>> + if (ret)
>> + goto free_drm;
>> +
>> + drm_mode_config_reset(drm);
>> + drm->mode_config.max_width = 8192;
>> + drm->mode_config.max_height = 8192;
>> + drm->mode_config.funcs = &meson_mode_config_funcs;
>> +
>> + priv->fbdev = drm_fbdev_cma_init(drm, 32,
>> + drm->mode_config.num_crtc,
>> + drm->mode_config.num_connector);
>> + if (IS_ERR(priv->fbdev)) {
>> + ret = PTR_ERR(priv->fbdev);
>> + goto free_drm;
>> + }
>> +
>> + drm_kms_helper_poll_init(drm);
>> +
>> + ret = drm_dev_register(drm, 0);
>> + if (ret)
>> + goto free_drm;
>> +
>> + platform_set_drvdata(pdev, priv);
>
> You need this before the drm_dev_register call I think.
Would be cleaner indeed.
>> +
>> + return 0;
>> +
>> +free_drm:
>> + drm_dev_unref(drm);
>> +
>> + return ret;
>> +}
>> +
[...]
>> +
>> +static int meson_plane_atomic_check(struct drm_plane *plane,
>> + struct drm_plane_state *state)
>> +{
>> + struct drm_rect src = {
>> + .x1 = state->src_x,
>> + .y1 = state->src_y,
>> + .x2 = state->src_x + state->src_w,
>> + .y2 = state->src_y + state->src_h,
>> + };
>> + struct drm_rect dest = {
>> + .x1 = state->crtc_x,
>> + .y1 = state->crtc_y,
>> + .x2 = state->crtc_x + state->crtc_w,
>> + .y2 = state->crtc_y + state->crtc_h,
>> + };
>> +
>> + if (state->fb) {
>> + int ret;
>> +
>> + ret = drm_rect_calc_hscale(&src, &dest,
>> + DRM_PLANE_HELPER_NO_SCALING,
>> + DRM_PLANE_HELPER_NO_SCALING);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = drm_rect_calc_vscale(&src, &dest,
>> + DRM_PLANE_HELPER_NO_SCALING,
>> + DRM_PLANE_HELPER_NO_SCALING);
>> + if (ret < 0)
>> + return ret;
>> + }
>
> drm_plane_helper_check_state gives you the above in less code.
Ok
>
>> +
>> + return 0;
>> +}
>> +
>> +static void meson_plane_atomic_update(struct drm_plane *plane,
>> + struct drm_plane_state *old_state)
>> +{
>> + struct meson_plane *meson_plane = to_meson_plane(plane);
>> +
>> + /*
>> + * Update Coordinates
>> + * Update Formats
>> + * Update Buffer
>> + * Enable Plane
>> + */
>> + meson_viu_update_osd1(meson_plane->priv, plane);
>> + meson_canvas_update_osd1_buffer(meson_plane->priv, plane);
>> +}
>> +
[...]
>> +
>> +void meson_viu_update_osd1(struct meson_drm *priv, struct drm_plane *plane)
>> +{
>> + struct drm_plane_state *state = plane->state;
>> + struct drm_framebuffer *fb = state->fb;
>> + struct drm_rect src = {
>> + .x1 = (state->src_x),
>> + .y1 = (state->src_y),
>> + .x2 = (state->src_x + state->src_w),
>> + .y2 = (state->src_y + state->src_h),
>> + };
>> + struct drm_rect dest = {
>> + .x1 = state->crtc_x,
>> + .y1 = state->crtc_y,
>> + .x2 = state->crtc_x + state->crtc_w,
>> + .y2 = state->crtc_y + state->crtc_h,
>> + };
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&priv->drm->event_lock, flags);
>> +
>> + /* Enable OSD and BLK0, set max global alpha */
>> + priv->viu.osd1_ctrl_stat = OSD_ENABLE |
>> + (0xFF << OSD_GLOBAL_ALPHA_SHIFT) |
>> + OSD_BLK0_ENABLE;
>> +
>> + /* Set up BLK0 to point to the right canvas */
>> + priv->viu.osd1_blk0_cfg[0] = ((MESON_CANVAS_ID_OSD1 << OSD_CANVAS_SEL) |
>> + OSD_ENDIANNESS_LE);
>> +
>> + /* On GXBB, Use the old non-HDR RGB2YUV converter */
>> + if (meson_vpu_is_compatible(priv, "amlogic,meson-gxbb-vpu"))
>> + priv->viu.osd1_blk0_cfg[0] |= OSD_OUTPUT_COLOR_RGB;
>> +
>> + switch (fb->pixel_format) {
>> + case DRM_FORMAT_XRGB8888:
>> + /* For XRGB, replace the pixel's alpha by 0xFF */
>> + writel_bits_relaxed(OSD_REPLACE_EN, OSD_REPLACE_EN,
>> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
>> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
>> + OSD_COLOR_MATRIX_32_ARGB;
>> + break;
>> + case DRM_FORMAT_ARGB8888:
>> + /* For ARGB, use the pixel's alpha */
>> + writel_bits_relaxed(OSD_REPLACE_EN, 0,
>> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
>> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
>> + OSD_COLOR_MATRIX_32_ARGB;
>> + break;
>> + case DRM_FORMAT_RGB888:
>> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_24 |
>> + OSD_COLOR_MATRIX_24_RGB;
>> + break;
>> + case DRM_FORMAT_RGB565:
>> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_16 |
>> + OSD_COLOR_MATRIX_16_RGB565;
>> + break;
>> + };
>> +
>> + if (state->crtc->mode.flags & DRM_MODE_FLAG_INTERLACE) {
>> + priv->viu.osd1_interlace = true;
>> +
>> + dest.y1 /= 2;
>> + dest.y2 /= 2;
>> + } else {
>> + priv->viu.osd1_interlace = true;
>> + meson_vpp_disable_interlace_vscaler_osd1(priv);
>> + }
>> +
>> + /*
>> + * The format of these registers is (x2 << 16 | x1),
>> + * where x2 is exclusive.
>> + * e.g. +30x1920 would be (1919 << 16) | 30
>> + */
>> + priv->viu.osd1_blk0_cfg[1] = ((fixed16_to_int(src.x2) - 1) << 16) |
>> + fixed16_to_int(src.x1);
>> + priv->viu.osd1_blk0_cfg[2] = ((fixed16_to_int(src.y2) - 1) << 16) |
>> + fixed16_to_int(src.y1);
>> + priv->viu.osd1_blk0_cfg[3] = ((dest.x2 - 1) << 16) | dest.x1;
>> + priv->viu.osd1_blk0_cfg[4] = ((dest.y2 - 1) << 16) | dest.y1;
>> +
>> + spin_unlock_irqrestore(&priv->drm->event_lock, flags);
>> +}
>> +
>> +void meson_viu_sync_osd1(struct meson_drm *priv)
>> +{
>> + /* Update the OSD registers */
>> + if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>> + writel_relaxed(priv->viu.osd1_ctrl_stat,
>> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
>> + writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>> + priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
>> + writel_relaxed(priv->viu.osd1_blk0_cfg[1],
>> + priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W1));
>> + writel_relaxed(priv->viu.osd1_blk0_cfg[2],
>> + priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W2));
>> + writel_relaxed(priv->viu.osd1_blk0_cfg[3],
>> + priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W3));
>> + writel_relaxed(priv->viu.osd1_blk0_cfg[4],
>> + priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W4));
>> +
>> + if (priv->viu.osd1_interlace) {
>> + struct drm_plane *plane = priv->primary_plane;
>> + struct drm_plane_state *state = plane->state;
>> + struct drm_rect dest = {
>> + .x1 = state->crtc_x,
>> + .y1 = state->crtc_y,
>> + .x2 = state->crtc_x + state->crtc_w,
>> + .y2 = state->crtc_y + state->crtc_h,
>> + };
>> +
>> + meson_vpp_setup_interlace_vscaler_osd1(priv, &dest);
>> + }
>> +
>> + meson_vpp_enable_osd1(priv);
>> +
>> + priv->viu.osd1_commit = false;
>> + }
>> +}
>
> Again I'd remove the indirection and for these put them into your plane
> implementation directly.
OK, I'll make them callable from the DRM ops directly.
>
>> +
>> +
>> +/* OSD csc defines */
>> +
>> +enum viu_matrix_sel_e {
>> + VIU_MATRIX_OSD_EOTF = 0,
>> + VIU_MATRIX_OSD,
>> +};
>> +
>> +enum viu_lut_sel_e {
>> + VIU_LUT_OSD_EOTF = 0,
>> + VIU_LUT_OSD_OETF,
>> +};
>> +
>> +#define COEFF_NORM(a) ((int)((((a) * 2048.0) + 1) / 2))
>> +#define MATRIX_5X3_COEF_SIZE 24
>> +
>> +#define EOTF_COEFF_NORM(a) ((int)((((a) * 4096.0) + 1) / 2))
>> +#define EOTF_COEFF_SIZE 10
>> +#define EOTF_COEFF_RIGHTSHIFT 1
>> +
>> +static int RGB709_to_YUV709l_coeff[MATRIX_5X3_COEF_SIZE] = {
>> + 0, 0, 0, /* pre offset */
>> + COEFF_NORM(0.181873), COEFF_NORM(0.611831), COEFF_NORM(0.061765),
>> + COEFF_NORM(-0.100251), COEFF_NORM(-0.337249), COEFF_NORM(0.437500),
>> + COEFF_NORM(0.437500), COEFF_NORM(-0.397384), COEFF_NORM(-0.040116),
>> + 0, 0, 0, /* 10'/11'/12' */
>> + 0, 0, 0, /* 20'/21'/22' */
>> + 64, 512, 512, /* offset */
>> + 0, 0, 0 /* mode, right_shift, clip_en */
>> +};
>> +
>> +/* eotf matrix: bypass */
>> +static int eotf_bypass_coeff[EOTF_COEFF_SIZE] = {
>> + EOTF_COEFF_NORM(1.0), EOTF_COEFF_NORM(0.0), EOTF_COEFF_NORM(0.0),
>> + EOTF_COEFF_NORM(0.0), EOTF_COEFF_NORM(1.0), EOTF_COEFF_NORM(0.0),
>> + EOTF_COEFF_NORM(0.0), EOTF_COEFF_NORM(0.0), EOTF_COEFF_NORM(1.0),
>> + EOTF_COEFF_RIGHTSHIFT /* right shift */
>> +};
>> +
>> +void meson_viu_set_osd_matrix(struct meson_drm *priv,
>> + enum viu_matrix_sel_e m_select,
>> + int *m, bool csc_on)
>> +{
>> + if (m_select == VIU_MATRIX_OSD) {
>> + /* osd matrix, VIU_MATRIX_0 */
>> + writel(((m[0] & 0xfff) << 16) | (m[1] & 0xfff),
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_PRE_OFFSET0_1));
>> + writel(m[2] & 0xfff,
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_PRE_OFFSET2));
>> + writel(((m[3] & 0x1fff) << 16) | (m[4] & 0x1fff),
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_COEF00_01));
>> + writel(((m[5] & 0x1fff) << 16) | (m[6] & 0x1fff),
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_COEF02_10));
>> + writel(((m[7] & 0x1fff) << 16) | (m[8] & 0x1fff),
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_COEF11_12));
>> + writel(((m[9] & 0x1fff) << 16) | (m[10] & 0x1fff),
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_COEF20_21));
>> +
>> + if (m[21]) {
>> + writel(((m[11] & 0x1fff) << 16) | (m[12] & 0x1fff),
>> + priv->io_base +
>> + _REG(VIU_OSD1_MATRIX_COEF22_30));
>> + writel(((m[13] & 0x1fff) << 16) | (m[14] & 0x1fff),
>> + priv->io_base +
>> + _REG(VIU_OSD1_MATRIX_COEF31_32));
>> + writel(((m[15] & 0x1fff) << 16) | (m[16] & 0x1fff),
>> + priv->io_base +
>> + _REG(VIU_OSD1_MATRIX_COEF40_41));
>> + writel(m[17] & 0x1fff, priv->io_base +
>> + _REG(VIU_OSD1_MATRIX_COLMOD_COEF42));
>> + } else
>> + writel((m[11] & 0x1fff) << 16, priv->io_base +
>> + _REG(VIU_OSD1_MATRIX_COEF22_30));
>> +
>> + writel(((m[18] & 0xfff) << 16) | (m[19] & 0xfff),
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_OFFSET0_1));
>> + writel(m[20] & 0xfff,
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_OFFSET2));
>> +
>> + writel_bits_relaxed(3 << 30, m[21] << 30,
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_COLMOD_COEF42));
>> + writel_bits_relaxed(7 << 16, m[22] << 16,
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_COLMOD_COEF42));
>> +
>> + /* 23 reserved for clipping control */
>> + writel_bits_relaxed(BIT(0), csc_on ? BIT(0) : 0,
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_CTRL));
>> + writel_bits_relaxed(BIT(1), 0,
>> + priv->io_base + _REG(VIU_OSD1_MATRIX_CTRL));
>> + } else if (m_select == VIU_MATRIX_OSD_EOTF) {
>> + int i;
>> +
>> + /* osd eotf matrix, VIU_MATRIX_OSD_EOTF */
>> + for (i = 0; i < 5; i++)
>> + writel(((m[i * 2] & 0x1fff) << 16) |
>> + (m[i * 2 + 1] & 0x1fff), priv->io_base +
>> + _REG(VIU_OSD1_EOTF_CTL + i + 1));
>> +
>> + writel_bits_relaxed(BIT(30), csc_on ? BIT(30) : 0,
>> + priv->io_base + _REG(VIU_OSD1_EOTF_CTL));
>> + writel_bits_relaxed(BIT(31), csc_on ? BIT(31) : 0,
>> + priv->io_base + _REG(VIU_OSD1_EOTF_CTL));
>> + }
>> +}
>> +
>> +#define OSD_EOTF_LUT_SIZE 33
>> +#define OSD_OETF_LUT_SIZE 41
>> +
>> +void meson_viu_set_osd_lut(struct meson_drm *priv, enum viu_lut_sel_e lut_sel,
>> + unsigned int *r_map, unsigned int *g_map,
>> + unsigned int *b_map,
>> + bool csc_on)
>> +{
>> + unsigned int addr_port;
>> + unsigned int data_port;
>> + unsigned int ctrl_port;
>> + int i;
>> +
>> + if (lut_sel == VIU_LUT_OSD_EOTF) {
>> + addr_port = VIU_OSD1_EOTF_LUT_ADDR_PORT;
>> + data_port = VIU_OSD1_EOTF_LUT_DATA_PORT;
>> + ctrl_port = VIU_OSD1_EOTF_CTL;
>> + } else if (lut_sel == VIU_LUT_OSD_OETF) {
>> + addr_port = VIU_OSD1_OETF_LUT_ADDR_PORT;
>> + data_port = VIU_OSD1_OETF_LUT_DATA_PORT;
>> + ctrl_port = VIU_OSD1_OETF_CTL;
>> + } else
>> + return;
>> +
>> + if (lut_sel == VIU_LUT_OSD_OETF) {
>> + writel(0, priv->io_base + _REG(addr_port));
>> +
>> + for (i = 0; i < 20; i++)
>> + writel(r_map[i * 2] | (r_map[i * 2 + 1] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + writel(r_map[OSD_OETF_LUT_SIZE - 1] | (g_map[0] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + for (i = 0; i < 20; i++)
>> + writel(g_map[i * 2 + 1] | (g_map[i * 2 + 2] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + for (i = 0; i < 20; i++)
>> + writel(b_map[i * 2] | (b_map[i * 2 + 1] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + writel(b_map[OSD_OETF_LUT_SIZE - 1],
>> + priv->io_base + _REG(data_port));
>> +
>> + if (csc_on)
>> + writel_bits_relaxed(0x7 << 29, 7 << 29,
>> + priv->io_base + _REG(ctrl_port));
>> + else
>> + writel_bits_relaxed(0x7 << 29, 0,
>> + priv->io_base + _REG(ctrl_port));
>> + } else if (lut_sel == VIU_LUT_OSD_EOTF) {
>> + writel(0, priv->io_base + _REG(addr_port));
>> +
>> + for (i = 0; i < 20; i++)
>> + writel(r_map[i * 2] | (r_map[i * 2 + 1] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + writel(r_map[OSD_EOTF_LUT_SIZE - 1] | (g_map[0] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + for (i = 0; i < 20; i++)
>> + writel(g_map[i * 2 + 1] | (g_map[i * 2 + 2] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + for (i = 0; i < 20; i++)
>> + writel(b_map[i * 2] | (b_map[i * 2 + 1] << 16),
>> + priv->io_base + _REG(data_port));
>> +
>> + writel(b_map[OSD_EOTF_LUT_SIZE - 1],
>> + priv->io_base + _REG(data_port));
>> +
>> + if (csc_on)
>> + writel_bits_relaxed(7 << 27, 7 << 27,
>> + priv->io_base + _REG(ctrl_port));
>> + else
>> + writel_bits_relaxed(7 << 27, 0,
>> + priv->io_base + _REG(ctrl_port));
>> +
>> + writel_bits_relaxed(BIT(31), BIT(31),
>> + priv->io_base + _REG(ctrl_port));
>> + }
>> +}
>> +
>> +/* eotf lut: linear */
>> +static unsigned int eotf_33_linear_mapping[OSD_EOTF_LUT_SIZE] = {
>> + 0x0000, 0x0200, 0x0400, 0x0600,
>> + 0x0800, 0x0a00, 0x0c00, 0x0e00,
>> + 0x1000, 0x1200, 0x1400, 0x1600,
>> + 0x1800, 0x1a00, 0x1c00, 0x1e00,
>> + 0x2000, 0x2200, 0x2400, 0x2600,
>> + 0x2800, 0x2a00, 0x2c00, 0x2e00,
>> + 0x3000, 0x3200, 0x3400, 0x3600,
>> + 0x3800, 0x3a00, 0x3c00, 0x3e00,
>> + 0x4000
>> +};
>> +
>> +/* osd oetf lut: linear */
>> +static unsigned int oetf_41_linear_mapping[OSD_OETF_LUT_SIZE] = {
>> + 0, 0, 0, 0,
>> + 0, 32, 64, 96,
>> + 128, 160, 196, 224,
>> + 256, 288, 320, 352,
>> + 384, 416, 448, 480,
>> + 512, 544, 576, 608,
>> + 640, 672, 704, 736,
>> + 768, 800, 832, 864,
>> + 896, 928, 960, 992,
>> + 1023, 1023, 1023, 1023,
>> + 1023
>> +};
>
> Might be fun to expose this using the color manager stuff, see
> drm_crtc_enable_color_mgmt().
Yes, I'll use them when the HDMI stuff lands ! This will certainly need such helpers !
>> +
>> +static void meson_viu_load_matrix(struct meson_drm *priv)
>> +{
>> + /* eotf lut bypass */
>> + meson_viu_set_osd_lut(priv, VIU_LUT_OSD_EOTF,
>> + eotf_33_linear_mapping, /* R */
>> + eotf_33_linear_mapping, /* G */
>> + eotf_33_linear_mapping, /* B */
>> + false);
>> +
>> + /* eotf matrix bypass */
>> + meson_viu_set_osd_matrix(priv, VIU_MATRIX_OSD_EOTF,
>> + eotf_bypass_coeff,
>> + false);
>> +
>> + /* oetf lut bypass */
>> + meson_viu_set_osd_lut(priv, VIU_LUT_OSD_OETF,
>> + oetf_41_linear_mapping, /* R */
>> + oetf_41_linear_mapping, /* G */
>> + oetf_41_linear_mapping, /* B */
>> + false);
>> +
>> + /* osd matrix RGB709 to YUV709 limit */
>> + meson_viu_set_osd_matrix(priv, VIU_MATRIX_OSD,
>> + RGB709_to_YUV709l_coeff,
>> + true);
>> +}
>> +
Neil