Message-ID: <a49102edf09d26a885c02b206ff8f02aefcd9f29.camel@icenowy.me>
Date: Thu, 22 Jan 2026 19:30:14 +0800
From: Icenowy Zheng <uwu@...nowy.me>
To: Thomas Zimmermann <tzimmermann@...e.de>, Andrzej Hajda
 <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
 Robert Foss <rfoss@...nel.org>, Laurent Pinchart
 <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>,
 Jernej Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob
 Herring <robh@...nel.org>,  Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Drew Fustini <fustini@...nel.org>, Guo Ren
 <guoren@...nel.org>, Fu Wei <wefu@...hat.com>
Cc: Philipp Zabel <p.zabel@...gutronix.de>, Dmitry Baryshkov
 <lumag@...nel.org>,  Michal Wilczynski <m.wilczynski@...sung.com>, Luca
 Ceresoli <luca.ceresoli@...tlin.com>, Han Gao <rabenda.cn@...il.com>,  Yao
 Zi <ziyao@...root.org>, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,  devicetree@...r.kernel.org,
 linux-riscv@...ts.infradead.org, Han Gao <gaohan@...as.ac.cn>
Subject: Re: [PATCH v5 3/9] drm: verisilicon: add a driver for Verisilicon
 display controllers

在 2026-01-22星期四的 11:39 +0100，Thomas Zimmermann写道：
> Hi
> 
> Am 22.01.26 um 10:23 schrieb Icenowy Zheng:
> > 在 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.
> 
> These drivers only get away with it because many other drivers keep 
> quite. Otherwise the kernel log would be filled with init reports.
> Your 
> choice, but it's questionable if anyone ever cares except for
> debugging.
> 
> 
> > 
> > > > +
> > > > +       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?
> 
> In this case, atomic_update should only perform an update of the
> plane's 
> graphics buffer (scanout address, color format). The logic then is
> 
> enable and mode setting:
> 
>   atomic_update, plus
>   atomic_enable
> 
> disable:
> 
>   atomic_disable only
> 
> page flips:
> 
>   atomic_update only

The hardware seems to work well with this pattern.

However, I found that plane's atomic_enable is still a rare feature in
the kernel, and the found user of it, tidss, still need to handle
situations that the plane is invisible in atomic_update (and disable
the plane in the case).

> 
> See 
> https://elixir.bootlin.com/linux/v6.18.6/source/drivers/gpu/drm/drm_atomic_helper.c#L3022

The code gives me the feeling that the enablement status of a plane is
independent to the visibility status (because the code does not
reference visible field in the state), which totally confuses me about
the usage of atomic_enable.

So is it about some enablement of hardware unrelated to visibility?

> 
> That's usually cleaner. But there's hardware where
> update/enable/disable 
> is connected in such a way that a single atomic_update is better.
> 
> 
> > 
> > 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].
> 
> I cannot comment on the hardware. But on the DRM side, we always
> disable 
> the pipeline for we program a new display mode; and then do an enable
> step to program the new mode. This happens on the CRTC, but it also 
> affects the CRTC's planes accordingly. For pageflips, we only run the
> plane's atomic_update.
> 
> If you you see a page flip, but need a full mode-setting operation,
> you 
> can manually trigger it by setting drm_crtc_state.mode_changed from
> the 
> plane's atomic_check. Here's an example: 
> https://elixir.bootlin.com/linux/v6.18.6/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L503
> 
> If the DC8000 and DC8200 behave sufficiently differently, my advise
> is 
> to write multiple _funcs structs with custom helpers and pick the 
> correct one when you initialize the modesetting pipeline.

Then let me just totally forget DC8000 now. At least the DC8200
hardware fits the update+enable model.

> 
> Best regards
> Thomas
> 
> > 
> > [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< ===================
> 

Powered by blists - more mailing lists