Hi,

On Thu, Jan 16, 2025 at 07:52:30PM +0100, Sasha Finkelstein wrote:

On Wed, 15 Jan 2025 at 11:21, Dmitry Baryshkov
[off-list ref] wrote:

quoted

quoted

+     ret = drm_simple_encoder_init(drm, &adp->encoder, DRM_MODE_ENCODER_DSI);

This is being deprecated, please use drm_encoder_init() /
drmm_encoder_init() instead.

I seem to be unable to find any documentation or a mailing list post
to this effect. If you are referring to this patchset:
https://patchwork.kernel.org/project/linux-samsung-soc/patch/20200313201744.19773-3-sam@ravnborg.org/
according to my understanding, it has not been applied.
In this case, the simple encoder api provides all functionality i need,
and a replacement would just be a simple encoder, except copied and pasted
into this driver.

a drmm_encoder_(alloc|init)'d encoder with a NULL funcs pointer is
strictly equivalent and doesn't require any extra infrastructure or work
on your side compared to what you have here.

quoted

quoted

+static void adp_remove(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct drm_device *drm = dev_get_drvdata(dev);
+     struct adp_drv_private *adp = to_adp(drm);
+
+     adp_disable_vblank(adp);

Isn't it being helped by the DRM itself? Anyway, I'd suggest moving it
after DRM device unregistration and shutdown.

Not sure i follow, as this call disables generation of vblank interrupts,
shouldn't it be done before all drm structures are torn down, to prevent
the interrupt handler from potentially operating on objects that are in
an invalid state?

Your adp_crtc_atomic_disable() function calls drm_crtc_vblank_off()
already. drm_atomic_helper_shutdown() will call the CRTC disable hook if
relevant, so you're effectively disabling vblank twice here.

Maxime