[PATCH] drm/meson: switch to a managed drm device
Anastasia Belova
abelova at astralinux.ru
Fri Aug 30 08:47:16 UTC 2024
Hi,
30/08/24 11:29, neil.armstrong at linaro.org пишет:
> On 30/08/2024 10:23, Anastasia Belova wrote:
>> Hi,
>>
>> 29/08/24 15:14, Neil Armstrong пишет:
>>> Hi,
>>>
>>> On 28/08/2024 13:04, Anastasia Belova wrote:
>>>> Switch to a managed drm device to cleanup some error handling
>>>> and make future work easier.
>>>>
>>>> Fix dereference of NULL in meson_drv_bind_master by removing
>>>> drm_dev_put(drm) before meson_encoder_*_remove where drm
>>>> dereferenced.
>>>
>>> Please send the fix separately with a Fixes tag.
>>>
>>
>> This fix can't be separated from the patch. drm_dev_put may be
>> removed only while switching to a managed drm. Otherwise
>> a check could be added before calling meson_encoder_*_remove.
>> But it would become redundant after switching to a managed drm.
>>
>> I may send the second version of this patch with Fixes tag, so all
>> changes could be applied to older versions.
>
> Reading the currenrt code, component_unbind_all(dev, drm) is called
> after drm_dev_put(drm), which is quite wrong aswell.
>
> So perhaps we should keep the kref on drm until component_unbind_all()
> is called in the first case, no ?
Right, but with a managed drm we don't need to release kref ourselves.
The comment for commit is not complete. Information about dereference
of NULL in component_unbind_all may be added in the second version, as
well as Fixes tag.
Anastasia Belova
>
> Neil
>
>>
>> Thanks,
>> Anastasia Belova
>>
>>> Thanks,
>>> Neil
>>>
>>>>
>>>> Co-developed by Linux Verification Center (linuxtesting.org).
>>>>
>>>> Signed-off-by: Anastasia Belova <abelova at astralinux.ru>
>>>> ---
>>>> drivers/gpu/drm/meson/meson_crtc.c | 10 +--
>>>> drivers/gpu/drm/meson/meson_drv.c | 71
>>>> ++++++++++------------
>>>> drivers/gpu/drm/meson/meson_drv.h | 2 +-
>>>> drivers/gpu/drm/meson/meson_encoder_cvbs.c | 8 +--
>>>> drivers/gpu/drm/meson/meson_overlay.c | 8 +--
>>>> drivers/gpu/drm/meson/meson_plane.c | 10 +--
>>>> 6 files changed, 51 insertions(+), 58 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c
>>>> b/drivers/gpu/drm/meson/meson_crtc.c
>>>> index d70616da8ce2..e1c0bf3baeea 100644
>>>> --- a/drivers/gpu/drm/meson/meson_crtc.c
>>>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
>>>> @@ -662,13 +662,13 @@ void meson_crtc_irq(struct meson_drm *priv)
>>>> drm_crtc_handle_vblank(priv->crtc);
>>>> - spin_lock_irqsave(&priv->drm->event_lock, flags);
>>>> + 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);
>>>> + spin_unlock_irqrestore(&priv->drm.event_lock, flags);
>>>> }
>>>> int meson_crtc_create(struct meson_drm *priv)
>>>> @@ -677,18 +677,18 @@ int meson_crtc_create(struct meson_drm *priv)
>>>> struct drm_crtc *crtc;
>>>> int ret;
>>>> - meson_crtc = devm_kzalloc(priv->drm->dev, sizeof(*meson_crtc),
>>>> + meson_crtc = devm_kzalloc(priv->drm.dev, sizeof(*meson_crtc),
>>>> GFP_KERNEL);
>>>> if (!meson_crtc)
>>>> return -ENOMEM;
>>>> meson_crtc->priv = priv;
>>>> crtc = &meson_crtc->base;
>>>> - ret = drm_crtc_init_with_planes(priv->drm, crtc,
>>>> + ret = drm_crtc_init_with_planes(&priv->drm, crtc,
>>>> priv->primary_plane, NULL,
>>>> &meson_crtc_funcs, "meson_crtc");
>>>> if (ret) {
>>>> - dev_err(priv->drm->dev, "Failed to init CRTC\n");
>>>> + dev_err(priv->drm.dev, "Failed to init CRTC\n");
>>>> return ret;
>>>> }
>>>> diff --git a/drivers/gpu/drm/meson/meson_drv.c
>>>> b/drivers/gpu/drm/meson/meson_drv.c
>>>> index 4bd0baa2a4f5..2e7c2e7c7b82 100644
>>>> --- a/drivers/gpu/drm/meson/meson_drv.c
>>>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>>>> @@ -182,7 +182,6 @@ static int meson_drv_bind_master(struct device
>>>> *dev, bool has_components)
>>>> struct platform_device *pdev = to_platform_device(dev);
>>>> const struct meson_drm_match_data *match;
>>>> struct meson_drm *priv;
>>>> - struct drm_device *drm;
>>>> struct resource *res;
>>>> void __iomem *regs;
>>>> int ret, i;
>>>> @@ -197,17 +196,13 @@ static int meson_drv_bind_master(struct
>>>> device *dev, bool has_components)
>>>> if (!match)
>>>> return -ENODEV;
>>>> - drm = drm_dev_alloc(&meson_driver, dev);
>>>> - if (IS_ERR(drm))
>>>> - return PTR_ERR(drm);
>>>> + priv = devm_drm_dev_alloc(dev, &meson_driver,
>>>> + struct meson_drm, drm);
>>>> - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> - if (!priv) {
>>>> - ret = -ENOMEM;
>>>> - goto free_drm;
>>>> - }
>>>> - drm->dev_private = priv;
>>>> - priv->drm = drm;
>>>> + if (IS_ERR(priv))
>>>> + return PTR_ERR(priv);
>>>> +
>>>> + priv->drm.dev_private = priv;
>>>> priv->dev = dev;
>>>> priv->compat = match->compat;
>>>> priv->afbcd.ops = match->afbcd_ops;
>>>> @@ -215,7 +210,7 @@ static int meson_drv_bind_master(struct device
>>>> *dev, bool has_components)
>>>> regs = devm_platform_ioremap_resource_byname(pdev, "vpu");
>>>> if (IS_ERR(regs)) {
>>>> ret = PTR_ERR(regs);
>>>> - goto free_drm;
>>>> + goto remove_encoders;
>>>> }
>>>> priv->io_base = regs;
>>>> @@ -223,13 +218,13 @@ static int meson_drv_bind_master(struct
>>>> device *dev, bool has_components)
>>>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hhi");
>>>> if (!res) {
>>>> ret = -EINVAL;
>>>> - goto free_drm;
>>>> + goto remove_encoders;
>>>> }
>>>> /* Simply ioremap since it may be a shared register zone */
>>>> regs = devm_ioremap(dev, res->start, resource_size(res));
>>>> if (!regs) {
>>>> ret = -EADDRNOTAVAIL;
>>>> - goto free_drm;
>>>> + goto remove_encoders;
>>>> }
>>>> priv->hhi = devm_regmap_init_mmio(dev, regs,
>>>> @@ -237,18 +232,18 @@ static int meson_drv_bind_master(struct
>>>> device *dev, bool has_components)
>>>> if (IS_ERR(priv->hhi)) {
>>>> dev_err(&pdev->dev, "Couldn't create the HHI regmap\n");
>>>> ret = PTR_ERR(priv->hhi);
>>>> - goto free_drm;
>>>> + goto remove_encoders;
>>>> }
>>>> priv->canvas = meson_canvas_get(dev);
>>>> if (IS_ERR(priv->canvas)) {
>>>> ret = PTR_ERR(priv->canvas);
>>>> - goto free_drm;
>>>> + goto remove_encoders;
>>>> }
>>>> ret = meson_canvas_alloc(priv->canvas, &priv->canvas_id_osd1);
>>>> if (ret)
>>>> - goto free_drm;
>>>> + goto remove_encoders;
>>>> ret = meson_canvas_alloc(priv->canvas, &priv->canvas_id_vd1_0);
>>>> if (ret)
>>>> goto free_canvas_osd1;
>>>> @@ -261,7 +256,7 @@ static int meson_drv_bind_master(struct device
>>>> *dev, bool has_components)
>>>> priv->vsync_irq = platform_get_irq(pdev, 0);
>>>> - ret = drm_vblank_init(drm, 1);
>>>> + ret = drm_vblank_init(&priv->drm, 1);
>>>> if (ret)
>>>> goto free_canvas_vd1_2;
>>>> @@ -284,10 +279,10 @@ static int meson_drv_bind_master(struct
>>>> device *dev, bool has_components)
>>>> ret = drmm_mode_config_init(drm);
>>>> if (ret)
>>>> goto free_canvas_vd1_2;
>>>> - drm->mode_config.max_width = 3840;
>>>> - drm->mode_config.max_height = 2160;
>>>> - drm->mode_config.funcs = &meson_mode_config_funcs;
>>>> - drm->mode_config.helper_private = &meson_mode_config_helpers;
>>>> + priv->drm.mode_config.max_width = 3840;
>>>> + priv->drm.mode_config.max_height = 2160;
>>>> + priv->drm.mode_config.funcs = &meson_mode_config_funcs;
>>>> + priv->drm.mode_config.helper_private =
>>>> &meson_mode_config_helpers;
>>>> /* Hardware Initialization */
>>>> @@ -308,9 +303,9 @@ static int meson_drv_bind_master(struct
>>>> device *dev, bool has_components)
>>>> goto exit_afbcd;
>>>> if (has_components) {
>>>> - ret = component_bind_all(dev, drm);
>>>> + ret = component_bind_all(dev, &priv->drm);
>>>> if (ret) {
>>>> - dev_err(drm->dev, "Couldn't bind all components\n");
>>>> + dev_err(priv->drm.dev, "Couldn't bind all components\n");
>>>> /* Do not try to unbind */
>>>> has_components = false;
>>>> goto exit_afbcd;
>>>> @@ -339,26 +334,26 @@ static int meson_drv_bind_master(struct
>>>> device *dev, bool has_components)
>>>> if (ret)
>>>> goto exit_afbcd;
>>>> - ret = request_irq(priv->vsync_irq, meson_irq, 0,
>>>> drm->driver->name, drm);
>>>> + ret = request_irq(priv->vsync_irq, meson_irq, 0,
>>>> priv->drm.driver->name, &priv->drm);
>>>> if (ret)
>>>> goto exit_afbcd;
>>>> - drm_mode_config_reset(drm);
>>>> + drm_mode_config_reset(&priv->drm);
>>>> - drm_kms_helper_poll_init(drm);
>>>> + drm_kms_helper_poll_init(&priv->drm);
>>>> platform_set_drvdata(pdev, priv);
>>>> - ret = drm_dev_register(drm, 0);
>>>> + ret = drm_dev_register(&priv->drm, 0);
>>>> if (ret)
>>>> goto uninstall_irq;
>>>> - drm_fbdev_dma_setup(drm, 32);
>>>> + drm_fbdev_dma_setup(&priv->drm, 32);
>>>> return 0;
>>>> uninstall_irq:
>>>> - free_irq(priv->vsync_irq, drm);
>>>> + free_irq(priv->vsync_irq, &priv->drm);
>>>> exit_afbcd:
>>>> if (priv->afbcd.ops)
>>>> priv->afbcd.ops->exit(priv);
>>>> @@ -370,15 +365,14 @@ static int meson_drv_bind_master(struct
>>>> device *dev, bool has_components)
>>>> meson_canvas_free(priv->canvas, priv->canvas_id_vd1_0);
>>>> free_canvas_osd1:
>>>> meson_canvas_free(priv->canvas, priv->canvas_id_osd1);
>>>> -free_drm:
>>>> - drm_dev_put(drm);
>>>> +remove_encoders:
>>>> meson_encoder_dsi_remove(priv);
>>>> meson_encoder_hdmi_remove(priv);
>>>> meson_encoder_cvbs_remove(priv);
>>>> if (has_components)
>>>> - component_unbind_all(dev, drm);
>>>> + component_unbind_all(dev, &priv->drm);
>>>> return ret;
>>>> }
>>>> @@ -391,7 +385,7 @@ static int meson_drv_bind(struct device *dev)
>>>> static void meson_drv_unbind(struct device *dev)
>>>> {
>>>> struct meson_drm *priv = dev_get_drvdata(dev);
>>>> - struct drm_device *drm = priv->drm;
>>>> + struct drm_device *drm = &priv->drm;
>>>> if (priv->canvas) {
>>>> meson_canvas_free(priv->canvas, priv->canvas_id_osd1);
>>>> @@ -404,7 +398,6 @@ static void meson_drv_unbind(struct device *dev)
>>>> drm_kms_helper_poll_fini(drm);
>>>> drm_atomic_helper_shutdown(drm);
>>>> free_irq(priv->vsync_irq, drm);
>>>> - drm_dev_put(drm);
>>>> meson_encoder_dsi_remove(priv);
>>>> meson_encoder_hdmi_remove(priv);
>>>> @@ -428,7 +421,7 @@ static int __maybe_unused
>>>> meson_drv_pm_suspend(struct device *dev)
>>>> if (!priv)
>>>> return 0;
>>>> - return drm_mode_config_helper_suspend(priv->drm);
>>>> + return drm_mode_config_helper_suspend(&priv->drm);
>>>> }
>>>> static int __maybe_unused meson_drv_pm_resume(struct device *dev)
>>>> @@ -445,7 +438,7 @@ static int __maybe_unused
>>>> meson_drv_pm_resume(struct device *dev)
>>>> if (priv->afbcd.ops)
>>>> priv->afbcd.ops->init(priv);
>>>> - return drm_mode_config_helper_resume(priv->drm);
>>>> + return drm_mode_config_helper_resume(&priv->drm);
>>>> }
>>>> static void meson_drv_shutdown(struct platform_device *pdev)
>>>> @@ -455,8 +448,8 @@ static void meson_drv_shutdown(struct
>>>> platform_device *pdev)
>>>> if (!priv)
>>>> return;
>>>> - drm_kms_helper_poll_fini(priv->drm);
>>>> - drm_atomic_helper_shutdown(priv->drm);
>>>> + drm_kms_helper_poll_fini(&priv->drm);
>>>> + drm_atomic_helper_shutdown(&priv->drm);
>>>> }
>>>> /*
>>>> diff --git a/drivers/gpu/drm/meson/meson_drv.h
>>>> b/drivers/gpu/drm/meson/meson_drv.h
>>>> index 3f9345c14f31..c4c6c810cb20 100644
>>>> --- a/drivers/gpu/drm/meson/meson_drv.h
>>>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>>>> @@ -53,7 +53,7 @@ struct meson_drm {
>>>> u8 canvas_id_vd1_1;
>>>> u8 canvas_id_vd1_2;
>>>> - struct drm_device *drm;
>>>> + struct drm_device drm;
>>>> struct drm_crtc *crtc;
>>>> struct drm_plane *primary_plane;
>>>> struct drm_plane *overlay_plane;
>>>> diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
>>>> b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
>>>> index d1191de855d9..ddca22c8c1ff 100644
>>>> --- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
>>>> +++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
>>>> @@ -104,7 +104,7 @@ static int meson_encoder_cvbs_get_modes(struct
>>>> drm_bridge *bridge,
>>>> for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
>>>> struct meson_cvbs_mode *meson_mode = &meson_cvbs_modes[i];
>>>> - mode = drm_mode_duplicate(priv->drm, &meson_mode->mode);
>>>> + mode = drm_mode_duplicate(&priv->drm, &meson_mode->mode);
>>>> if (!mode) {
>>>> dev_err(priv->dev, "Failed to create a new display
>>>> mode\n");
>>>> return 0;
>>>> @@ -221,7 +221,7 @@ static const struct drm_bridge_funcs
>>>> meson_encoder_cvbs_bridge_funcs = {
>>>> int meson_encoder_cvbs_probe(struct meson_drm *priv)
>>>> {
>>>> - struct drm_device *drm = priv->drm;
>>>> + struct drm_device *drm = &priv->drm;
>>>> struct meson_encoder_cvbs *meson_encoder_cvbs;
>>>> struct drm_connector *connector;
>>>> struct device_node *remote;
>>>> @@ -256,7 +256,7 @@ int meson_encoder_cvbs_probe(struct meson_drm
>>>> *priv)
>>>> meson_encoder_cvbs->priv = priv;
>>>> /* Encoder */
>>>> - ret = drm_simple_encoder_init(priv->drm,
>>>> &meson_encoder_cvbs->encoder,
>>>> + ret = drm_simple_encoder_init(&priv->drm,
>>>> &meson_encoder_cvbs->encoder,
>>>> DRM_MODE_ENCODER_TVDAC);
>>>> if (ret)
>>>> return dev_err_probe(priv->dev, ret,
>>>> @@ -273,7 +273,7 @@ int meson_encoder_cvbs_probe(struct meson_drm
>>>> *priv)
>>>> }
>>>> /* Initialize & attach Bridge Connector */
>>>> - connector = drm_bridge_connector_init(priv->drm,
>>>> &meson_encoder_cvbs->encoder);
>>>> + connector = drm_bridge_connector_init(&priv->drm,
>>>> &meson_encoder_cvbs->encoder);
>>>> if (IS_ERR(connector))
>>>> return dev_err_probe(priv->dev, PTR_ERR(connector),
>>>> "Unable to create CVBS bridge connector\n");
>>>> diff --git a/drivers/gpu/drm/meson/meson_overlay.c
>>>> b/drivers/gpu/drm/meson/meson_overlay.c
>>>> index 7f98de38842b..60ee7f758723 100644
>>>> --- a/drivers/gpu/drm/meson/meson_overlay.c
>>>> +++ b/drivers/gpu/drm/meson/meson_overlay.c
>>>> @@ -484,7 +484,7 @@ static void meson_overlay_atomic_update(struct
>>>> drm_plane *plane,
>>>> interlace_mode = new_state->crtc->mode.flags &
>>>> DRM_MODE_FLAG_INTERLACE;
>>>> - spin_lock_irqsave(&priv->drm->event_lock, flags);
>>>> + spin_lock_irqsave(&priv->drm.event_lock, flags);
>>>> if ((fb->modifier & DRM_FORMAT_MOD_AMLOGIC_FBC(0, 0)) ==
>>>> DRM_FORMAT_MOD_AMLOGIC_FBC(0, 0)) {
>>>> @@ -717,7 +717,7 @@ static void meson_overlay_atomic_update(struct
>>>> drm_plane *plane,
>>>> priv->viu.vd1_enabled = true;
>>>> - spin_unlock_irqrestore(&priv->drm->event_lock, flags);
>>>> + spin_unlock_irqrestore(&priv->drm.event_lock, flags);
>>>> DRM_DEBUG_DRIVER("\n");
>>>> }
>>>> @@ -838,7 +838,7 @@ int meson_overlay_create(struct meson_drm *priv)
>>>> DRM_DEBUG_DRIVER("\n");
>>>> - meson_overlay = devm_kzalloc(priv->drm->dev,
>>>> sizeof(*meson_overlay),
>>>> + meson_overlay = devm_kzalloc(priv->drm.dev,
>>>> sizeof(*meson_overlay),
>>>> GFP_KERNEL);
>>>> if (!meson_overlay)
>>>> return -ENOMEM;
>>>> @@ -846,7 +846,7 @@ int meson_overlay_create(struct meson_drm *priv)
>>>> meson_overlay->priv = priv;
>>>> plane = &meson_overlay->base;
>>>> - drm_universal_plane_init(priv->drm, plane, 0xFF,
>>>> + drm_universal_plane_init(&priv->drm, plane, 0xFF,
>>>> &meson_overlay_funcs,
>>>> supported_drm_formats,
>>>> ARRAY_SIZE(supported_drm_formats),
>>>> diff --git a/drivers/gpu/drm/meson/meson_plane.c
>>>> b/drivers/gpu/drm/meson/meson_plane.c
>>>> index b43ac61201f3..13be94309bf4 100644
>>>> --- a/drivers/gpu/drm/meson/meson_plane.c
>>>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>>>> @@ -157,7 +157,7 @@ static void meson_plane_atomic_update(struct
>>>> drm_plane *plane,
>>>> * Update Buffer
>>>> * Enable Plane
>>>> */
>>>> - spin_lock_irqsave(&priv->drm->event_lock, flags);
>>>> + spin_lock_irqsave(&priv->drm.event_lock, flags);
>>>> /* Check if AFBC decoder is required for this buffer */
>>>> if ((meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) ||
>>>> @@ -393,7 +393,7 @@ static void meson_plane_atomic_update(struct
>>>> drm_plane *plane,
>>>> priv->viu.osd1_enabled = true;
>>>> - spin_unlock_irqrestore(&priv->drm->event_lock, flags);
>>>> + spin_unlock_irqrestore(&priv->drm.event_lock, flags);
>>>> }
>>>> static void meson_plane_atomic_disable(struct drm_plane *plane,
>>>> @@ -536,7 +536,7 @@ int meson_plane_create(struct meson_drm *priv)
>>>> const uint64_t *format_modifiers = format_modifiers_default;
>>>> int ret;
>>>> - meson_plane = devm_kzalloc(priv->drm->dev,
>>>> sizeof(*meson_plane),
>>>> + meson_plane = devm_kzalloc(priv->drm.dev, sizeof(*meson_plane),
>>>> GFP_KERNEL);
>>>> if (!meson_plane)
>>>> return -ENOMEM;
>>>> @@ -549,14 +549,14 @@ int meson_plane_create(struct meson_drm *priv)
>>>> else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>>>> format_modifiers = format_modifiers_afbc_g12a;
>>>> - ret = drm_universal_plane_init(priv->drm, plane, 0xFF,
>>>> + ret = drm_universal_plane_init(&priv->drm, plane, 0xFF,
>>>> &meson_plane_funcs,
>>>> supported_drm_formats,
>>>> ARRAY_SIZE(supported_drm_formats),
>>>> format_modifiers,
>>>> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
>>>> if (ret) {
>>>> - devm_kfree(priv->drm->dev, meson_plane);
>>>> + devm_kfree(priv->drm.dev, meson_plane);
>>>> return ret;
>>>> }
>>>
>>
>
More information about the dri-devel
mailing list