在 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< ===================
