Re: [PATCH] drm: get rid of DRM_DEBUG_* log calls in drm core, files drm_{b,c}*.c
On Thu, 30 Dec 2021, Claudio Suarez wrote:
> DRM_DEBUG_* and DRM_* log calls are deprecated.
> Change them to drm_dbg_* / drm_{err,info,...} calls in drm core
> files.
>
> To avoid making a very big patch, this change is split in
> smaller patches. This one includes drm_{b,c}*.c
Personally, I'd split it further to smaller patches.
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/drm_blend.c | 6 +-
> drivers/gpu/drm/drm_bridge.c | 6 +-
> drivers/gpu/drm/drm_bufs.c | 116 +--
> drivers/gpu/drm/drm_client_modeset.c | 109 +
> drivers/gpu/drm/drm_color_mgmt.c | 6 +-
> drivers/gpu/drm/drm_connector.c | 37 +
> drivers/gpu/drm/drm_context.c| 18 ++---
> drivers/gpu/drm/drm_crtc.c | 40 -
> drivers/gpu/drm/drm_crtc_helper.c| 61 +++---
> 9 files changed, 211 insertions(+), 188 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index ec37cbfabb50..4a988815f998 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -450,7 +450,7 @@ static int drm_atomic_helper_crtc_normalize_zpos(struct
> drm_crtc *crtc,
> int i, n = 0;
> int ret = 0;
>
> - DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] calculating normalized zpos values\n",
>crtc->base.id, crtc->name);
Throughout the patch, please fix the indentation on the following
lines. The lines should be aligned at the next column after the opening
brace "(" in the function call, like they used to be.
>
> states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
> @@ -469,7 +469,7 @@ static int drm_atomic_helper_crtc_normalize_zpos(struct
> drm_crtc *crtc,
> goto done;
> }
> states[n++] = plane_state;
> - DRM_DEBUG_ATOMIC("[PLANE:%d:%s] processing zpos value %d\n",
> + drm_dbg_atomic(dev, "[PLANE:%d:%s] processing zpos value %d\n",
>plane->base.id, plane->name,
>plane_state->zpos);
> }
> @@ -480,7 +480,7 @@ static int drm_atomic_helper_crtc_normalize_zpos(struct
> drm_crtc *crtc,
> plane = states[i]->plane;
>
> states[i]->normalized_zpos = i;
> - DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",
> + drm_dbg_atomic(dev, "[PLANE:%d:%s] normalized zpos value %d\n",
>plane->base.id, plane->name, i);
> }
> crtc_state->zpos_changed = true;
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c96847fc0ebc..b108377b4b40 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -288,10 +288,12 @@ int drm_bridge_attach(struct drm_encoder *encoder,
> struct drm_bridge *bridge,
> list_del(&bridge->chain_node);
>
> #ifdef CONFIG_OF
> - DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
> + drm_err(encoder->dev,
> + "failed to attach bridge %pOF to encoder %s: %d\n",
> bridge->of_node, encoder->name, ret);
> #else
> - DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
> + drm_err(encoder->dev,
> + "failed to attach bridge to encoder %s: %d\n",
> encoder->name, ret);
> #endif
>
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index fcca21e8efac..dd8e100e120c 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -171,8 +171,8 @@ static int drm_addmap_core(struct drm_device *dev,
> resource_size_t offset,
> kfree(map);
> return -EINVAL;
> }
> - DRM_DEBUG("offset = 0x%08llx, size = 0x%08lx, type = %d\n",
> - (unsigned long long)map->offset, map->size, map->type);
> + drm_dev_dbg(dev, "offset = 0x%08llx, size = 0x%08lx, type = %d\n",
> + (unsigned long long)map->offset, map->size, map->type);
Please avoid drm_dev_dbg() when struct drm_device is available,
throughout the patch. DRM_DEBUG() -> drm_dbg_core() in drm_*.[ch].
Clearly dev is struct drm_device here, and in a lot of places, so you're
passing an incompatible pointer to drm_dev_dbg(), which is for when you
only have struct device available.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm: get rid of DRM_DEBUG_* log calls in drm core, files drm_{b,c}*.c
On Thu, 30 Dec 2021, Claudio Suarez wrote: > Please, don't apply this patch. I have to review it,. Please make sure you build with DRM_LEGACY=y to include drm_bufs.c in the build. If you Cc: intel-...@lists.freedesktop.org on the patch, you'll get Intel CI results on the patch too. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v5 0/2] GPD Win Max display fixes
On Wed, 29 Dec 2021, Anisse Astier wrote: > This patch series is for making the GPD Win Max display usable with > Linux. Thanks, pushed to drm-intel-next. Sorry for the delay in dealing with the patches. BR, Jani. > > The GPD Win Max is a small laptop, and its eDP panel does not send an > EDID over DPCD; the EDID is instead available in the intel opregion, in > mailbox #5 [1] > > The second patch is just to fix the orientation of the panel. > > Changes since v1: > - rebased on drm-tip > - squashed patch 1 & 2 > - picked up Reviewed-by from Hans de Goede (thanks for the review) > > Changes since v2: > - rebased on drm-tip > - updated commit message > > When v2 was initially sent [3] Ville Syrjälä suggested that it might be > a good idea to use the ACPI _DDC method instead to get the EDID, to > cover a wider range of hardware. Unfortunately, it doesn't seem > available on GPD Win Max, so I think this work should be done > independently, and this patch series considered separately. > > Change since v3: > - edits following Jani's review: > - The EDID from the opregion is now only used as a fallback: if we > cannot get any edid from the edp connector, then we attempt to get > it from the opregion. This works for the GPD Win Max. > - all other remarks should have been taken into account > - rebased on drm-tip > - added Co-developed-by > - reordered signed-off-by and reviewed-by in second patch (thanks >Maarten!) > > Changes since v4: > - checkpatch.pl fixes > - rebased on drm-tip > - Note: patch #1 is incomplete, still missing Jani's signed-off-by > > > [1]: https://gitlab.freedesktop.org/drm/intel/-/issues/3454 > [2]: > https://patchwork.kernel.org/project/intel-gfx/patch/20200828061941.17051-1-jani.nik...@intel.com/ > [3]: > https://patchwork.kernel.org/project/intel-gfx/patch/20210531204642.4907-2-ani...@astier.eu/ > > > Anisse Astier (2): > drm/i915/opregion: add support for mailbox #5 EDID > drm: Add orientation quirk for GPD Win Max > > .../gpu/drm/drm_panel_orientation_quirks.c| 6 ++ > drivers/gpu/drm/i915/display/intel_dp.c | 7 +++ > drivers/gpu/drm/i915/display/intel_opregion.c | 55 ++- > drivers/gpu/drm/i915/display/intel_opregion.h | 10 > 4 files changed, 77 insertions(+), 1 deletion(-) -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] gpu/drm: fix potential memleak in error branch
On Tue, 16 Nov 2021, Bernard Zhao wrote:
> This patch try to fix potential memleak in error branch.
Please elaborate.
BR,
Jani.
>
> Signed-off-by: Bernard Zhao
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 22 --
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index f3d79eda94bb..f73b180dee73 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -5501,7 +5501,10 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
>int max_lane_count, int max_link_rate,
>int conn_base_id)
> {
> - struct drm_dp_mst_topology_state *mst_state;
> + struct drm_dp_mst_topology_state *mst_state = NULL;
> +
> + mgr->payloads = NULL;
> + mgr->proposed_vcpis = NULL;
>
> mutex_init(&mgr->lock);
> mutex_init(&mgr->qlock);
> @@ -5523,7 +5526,7 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
>*/
> mgr->delayed_destroy_wq = alloc_ordered_workqueue("drm_dp_mst_wq", 0);
> if (mgr->delayed_destroy_wq == NULL)
> - return -ENOMEM;
> + goto out;
>
> INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work);
> INIT_WORK(&mgr->tx_work, drm_dp_tx_work);
> @@ -5539,18 +5542,18 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
> mgr->conn_base_id = conn_base_id;
> if (max_payloads + 1 > sizeof(mgr->payload_mask) * 8 ||
> max_payloads + 1 > sizeof(mgr->vcpi_mask) * 8)
> - return -EINVAL;
> + goto failed;
> mgr->payloads = kcalloc(max_payloads, sizeof(struct drm_dp_payload),
> GFP_KERNEL);
> if (!mgr->payloads)
> - return -ENOMEM;
> + goto failed;
> mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi
> *), GFP_KERNEL);
> if (!mgr->proposed_vcpis)
> - return -ENOMEM;
> + goto failed;
> set_bit(0, &mgr->payload_mask);
>
> mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> if (mst_state == NULL)
> - return -ENOMEM;
> + goto failed;
>
> mst_state->total_avail_slots = 63;
> mst_state->start_slot = 1;
> @@ -5563,6 +5566,13 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
> &drm_dp_mst_topology_state_funcs);
>
> return 0;
> +
> +failed:
> + kfree(mgr->proposed_vcpis);
> + kfree(mgr->payloads);
> + destroy_workqueue(mgr->delayed_destroy_wq);
> +out:
> + return -ENOMEM;
> }
> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 3/3] drm/atomic: Make private objs proper objects
On Wed, 12 Jul 2017, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä
>
> Make the atomic private object stuff less special by introducing proper
> base classes for the object and its state. Drivers can embed these in
> their own appropriate objects, after which these things will work
> exactly like the plane/crtc/connector states during atomic operations.
>
> v2: Reorder to not depend on drm_dynarray (Daniel)
>
> Cc: Dhinakaran Pandiyan
> Cc: Daniel Vetter
> Reviewed-by: Daniel Vetter #v1
> Signed-off-by: Ville Syrjälä
Stumbled upon an old commit
commit a4370c777406c2810e37fafd166ccddecdb2a60c
Author: Ville Syrjälä
Date: Wed Jul 12 18:51:02 2017 +0300
drm/atomic: Make private objs proper objects
which is this patch.
> @@ -3050,8 +3043,7 @@ struct drm_dp_mst_topology_state
> *drm_atomic_get_mst_topology_state(struct drm_a
> struct drm_device *dev = mgr->dev;
>
> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> - return drm_atomic_get_private_obj_state(state, mgr,
> - &mst_state_funcs);
> + return to_dp_mst_topology_state(drm_atomic_get_private_obj_state(state,
> &mgr->base));
> }
> EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
I don't think this combines well with...
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 177ab6f86855..d55abb75f29a 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -404,12 +404,17 @@ struct drm_dp_payload {
> int vcpi;
> };
>
> +#define to_dp_mst_topology_state(x) container_of(x, struct
> drm_dp_mst_topology_state, base)
...this in case of error pointers that
drm_atomic_get_private_obj_state() may return.
Is that right, or am I just ready to 'shutdown -h now' for 2021...?
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
[pull] amdgpu drm-fixes-5.16
Hi Dave, Daniel, One small suspend/resume fix, and the rest is restoring the runtime pm behavior that changed for some people when efifb dropped its extra runtime pm reference. Unfortunately the issue has not been something we've been able to reproduce consistently. The patches here basically re-enable the previous behavior, but in the driver if the device is the one that was used by the firmware framebuffer. It is limited to the device used by firmware since I don't want to regress hybrid graphics laptops and other systems that have been working fine years. The first patch adds a helper to fbmem.c to determine if the adapter is the one used by firmware. This needs a more indepth analysis, but with the holidays and the late state of the 5.16 cycle, this seems like the best option. The following changes since commit ce9b333c73a5a8707f2f446a837a6ca743ddcffd: Merge branch 'drm-misc-fixes' of ssh://git.freedesktop.org/git/drm/drm-misc into drm-fixes (2021-12-31 11:40:29 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-fixes-5.16-2021-12-31 for you to fetch changes up to b95dc06af3e683d6b7ddbbae178b2b2a21ee8b2b: drm/amdgpu: disable runpm if we are the primary adapter (2021-12-31 08:57:45 -0500) amd-drm-fixes-5.16-2021-12-31: amdgpu: - Suspend/resume fix - Restore runtime pm behavior with efifb Alex Deucher (2): fbdev: fbmem: add a helper to determine if an aperture is used by a fw fb drm/amdgpu: disable runpm if we are the primary adapter Evan Quan (1): drm/amd/pm: keep the BACO feature enabled for suspend drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 28 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 8 +- drivers/video/fbdev/core/fbmem.c | 47 +++ include/linux/fb.h| 1 + 6 files changed, 90 insertions(+), 1 deletion(-)
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436
--- Comment #8 from spassw...@web.de ---
Just did a bit of experimentation and changed dc_get_hpd_state_dcn21 (in
5.16-rc7) to
uint32_t dc_get_hpd_state_dcn21(struct irq_service *irq_service, enum
dc_irq_source source)
{
const struct irq_source_info *info;
uint32_t addr;
uint32_t value;
uint32_t current_status;
info = find_irq_source_info(irq_service, source);
if (!info)
return 0;
addr = info->status_reg;
if (!addr)
return 0;
value = dm_read_reg(irq_service->ctx, addr);
//current_status =
// get_reg_field_value(
// value,
// HPD0_DC_HPD_INT_STATUS,
// DC_HPD_SENSE);
//return current_status;
return 0;
}
This also leads to working suspend resume, the get_reg_field_value seems to
trigger the problems.
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
[PATCH] drm: mxsfb: Shutdown the display on remove
When the device is unbound from the driver, the display may be active. Make sure it gets shut down. Signed-off-by: Marek Vasut Cc: Daniel Vetter Cc: Laurent Pinchart Cc: Lucas Stach Cc: Sam Ravnborg Cc: Stefan Agner Cc: Thomas Zimmermann --- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index 86d78634a9799..6d7a3aeff50b0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -374,6 +374,7 @@ static int mxsfb_remove(struct platform_device *pdev) struct drm_device *drm = platform_get_drvdata(pdev); drm_dev_unregister(drm); + drm_atomic_helper_shutdown(drm); mxsfb_unload(drm); drm_dev_put(drm); -- 2.34.1
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 --- Comment #9 from spassw...@web.de --- get_reg_field_value(value, HPD0_DC_HPD_INT_STATUS, DC_HPD_SENSE) is just a macro expanding to an inline function, i.e. current_status = (value & HPD0_DC_HPD_INT_STATUS__DC_HPD_SENSE_MASK) >> HPD0_DC_HPD_IN_STATUS__DC_HPD_SENSE__SHIFT; with ..._MASK = 0x2 being 0x2 and ...__SHIFT = 0x1, problems seem to occur when current_status != 0. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH] drm: mxsfb: Shutdown the display on remove
Hi Marek, Thank you for the patch. On Fri, Dec 31, 2021 at 05:00:56PM +0100, Marek Vasut wrote: > When the device is unbound from the driver, the display may be active. > Make sure it gets shut down. > > Signed-off-by: Marek Vasut > Cc: Daniel Vetter > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Sam Ravnborg > Cc: Stefan Agner > Cc: Thomas Zimmermann > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 86d78634a9799..6d7a3aeff50b0 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -374,6 +374,7 @@ static int mxsfb_remove(struct platform_device *pdev) > struct drm_device *drm = platform_get_drvdata(pdev); > > drm_dev_unregister(drm); > + drm_atomic_helper_shutdown(drm); That looks reasonable. While at it, should you also implement the .shutdown driver operation ? Reviewed-by: Laurent Pinchart > mxsfb_unload(drm); > drm_dev_put(drm); > -- Regards, Laurent Pinchart
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 Mario Limonciello (supe...@gmail.com) changed: What|Removed |Added CC||supe...@gmail.com --- Comment #10 from Mario Limonciello (supe...@gmail.com) --- current_status != 0 causes rn_update_clocks to behave differently, basically rn_vbios_smu_set_dcn_low_power_state can never get called. I wonder if we want to just not apply this WA as part of the suspend/resume paths, but only during the runtime paths? -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 201957] amdgpu: ring gfx timeout
https://bugzilla.kernel.org/show_bug.cgi?id=201957 Spencer (s...@nandre.com) changed: What|Removed |Added CC||s...@nandre.com --- Comment #53 from Spencer (s...@nandre.com) --- (In reply to roman from comment #52) > I can confirm that > amdgpu.dpm=0 > removes the issue > on an AMD Radeon PRO FIJI (Dual Fury) kernel: 5.15.10|FW: > 20211027.1d00989-1|mesa: 21.3.2-1 > > Works perfectly fine in Gnome as long as there is no application accessing > the 2nd GPU. In sourse games it works fine for me but in many non-source games it'll just fucking die. Anyways, now I cant boot withouth dpm, it freezes, meaning that source games will crash, along with Risk of Rain 2 and others. > Hopefully @Alex can do/forward this since this is a P1 blocking issue and > open for 3 years. I can only hope it gets fixed one day soon. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
