Re: [PATCH v5 3/9] drm: verisilicon: add a driver for Verisilicon display controllers

[Icenowy Zheng <uwu@icenowy.me>]

在 2026-01-21星期三的 13:56 +0100，Thomas Zimmermann写道：

=============== 8< ===================
> > +
> > +       dev_info(dev, "DC%x rev %x customer %x\n", dc-
> > >identity.model,
> > +                dc->identity.revision, dc->identity.customer_id);
> 
> drm_dbg().  Drivers should be quite by default.

Well for this kind of identification information I think driver is
usually not quiet, at least amdgpu (when doing IP discovery), panfrost
and etnaviv (which shares the same set of identification registers with
this driver) is reporting it.

> 
> > +
> > +       if (port_count > dc->identity.display_count) {
> > +               dev_err(dev, "too many downstream ports than HW
> > capability\n");
> > +               ret = -EINVAL;
> > +               goto err_rst_assert;
> > +       }
> > +
=============== 8< ===================
> > +
> > +       if (!state->visible || !fb) {
> > +               regmap_write(dc->regs, VSDC_FB_CONFIG(output), 0);
> > +               regmap_write(dc->regs, VSDC_FB_CONFIG_EX(output),
> > 0);
> > +               goto commit;
> > +       } else {
> > +               regmap_set_bits(dc->regs,
> > VSDC_FB_CONFIG_EX(output),
> > +                               VSDC_FB_CONFIG_EX_FB_EN);
> > +       }
> 
> Since you handle the plane on/plane off cases here, I'd advise to 
> implement atomic_enable and atomic_disable for the plane, if the 
> hardware allows it. (There is hardware were the current pattern makes
> sense though.)

If atomic_*able is going to be implemented, should atomic_update() keep
the plane on/off code?

BTW it seems that DC8200 has the shadow register capability that could
be manually commited but the older DC8000 has no (the
VSDC_FB_CONFIG_EX_COMMIT bit written below is a new thing) -- the
VSDC_FB_CONFIG register on DC8000 has a write-only bit that when
written with 0 the display is put to reset and when written with 1 the
display will start to run. This pattern seems to be not able to keep
the enabled state while programming (at least part of) new plane
configuration, see [1].

[1]
https://github.com/milkv-megrez/rockos-u-boot/blob/c9221cf2fa77d39c0b241ab4b030c708e7ebe279/drivers/video/eswin/eswin_dc_reg.h#L3579

> 
> > +
> > +       drm_format_to_vs_format(state->fb->format->format, &fmt);
> > +
> > +       regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
> > +                          VSDC_FB_CONFIG_FMT_MASK,
> > +                          VSDC_FB_CONFIG_FMT(fmt.color));
> > +       regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
> > +                          VSDC_FB_CONFIG_SWIZZLE_MASK,
> > +                          VSDC_FB_CONFIG_SWIZZLE(fmt.swizzle));
> > +       regmap_assign_bits(dc->regs, VSDC_FB_CONFIG(output),
> > +                          VSDC_FB_CONFIG_UV_SWIZZLE_EN,
> > fmt.uv_swizzle);
> > +
> > +       /* Get the physical address of the buffer in memory */
> > +       gem = drm_fb_dma_get_gem_obj(fb, 0);
> > +
> > +       /* Compute the start of the displayed memory */
> > +       bpp = fb->format->cpp[0];
> > +       dma_addr = gem->dma_addr + fb->offsets[0];
> > +
> > +       /* Fixup framebuffer address for src coordinates */
> > +       dma_addr += (state->src.x1 >> 16) * bpp;
> 
> bpp is deprecated and should be avoided in new code. You can compute
> the 
> offset with drm_format_min_pitch():
> 
> drm_format_min_pitch(fb->format, 0, state->src.x1 >> 16 )
> 
> Best regards
> Thomas
> 
> > +       dma_addr += (state->src.y1 >> 16) * fb->pitches[0];
> > +
> > +       regmap_write(dc->regs, VSDC_FB_ADDRESS(output),
> > +                    lower_32_bits(dma_addr));
> > +       regmap_write(dc->regs, VSDC_FB_STRIDE(output),
> > +                    fb->pitches[0]);
> > +
> > +       regmap_write(dc->regs, VSDC_FB_TOP_LEFT(output),
> > +                    VSDC_MAKE_PLANE_POS(state->crtc_x, state-
> > >crtc_y));
> > +       regmap_write(dc->regs, VSDC_FB_BOTTOM_RIGHT(output),
> > +                    VSDC_MAKE_PLANE_POS(state->crtc_x + state-
> > >crtc_w,
> > +                                        state->crtc_y + state-
> > >crtc_h));
> > +       regmap_write(dc->regs, VSDC_FB_SIZE(output),
> > +                    VSDC_MAKE_PLANE_SIZE(state->crtc_w, state-
> > >crtc_h));
> > +
> > +       regmap_write(dc->regs, VSDC_FB_BLEND_CONFIG(output),
> > +                    VSDC_FB_BLEND_CONFIG_BLEND_DISABLE);
> > +commit:
> > +       regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> > +                       VSDC_FB_CONFIG_EX_COMMIT);
> > +}
> > +
=============== 8< ===================