On Mon, Nov 22, 2021 at 8:59 PM Cui, Flora <flora....@amd.com> wrote:
>
> [Public]
>
>
> Modprobe -r amdgpu get oops in amdgpu_vkms_sw_fini()
>
> for (i = 0; i < adev->mode_info.num_crtc; i++)
>
> if (adev->mode_info.crtcs[i])
>
>
> hrtimer_cancel(&adev->mode_info.crtcs[i]->vblank_timer);
>
> adev->mode_info.crtcs[i]->vblank_timer is not initiated as vkms init its own
> amdgpu_vkms_output-> vblank_hrtimer. This patch drop amdgpu_vkms_output->
> vblank_hrtimer and try with adev->mode_info.crtcs[i]->vblank_timer to keep
> align with amdgpu_dm & dce_vx_0.c
>
>
I think this might be better as two patches. The simple fix for this
problem would be:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
index ce982afeff91..b620a6a3cb9f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
@@ -504,8 +504,7 @@ static int amdgpu_vkms_sw_fini(void *handle)
int i = 0;
for (i = 0; i < adev->mode_info.num_crtc; i++)
- if (adev->mode_info.crtcs[i])
- hrtimer_cancel(&adev->mode_info.crtcs[i]->vblank_timer);
+ hrtimer_cancel(&adev->amdgpu_vkms_output[i].vblank_hrtimer);
kfree(adev->mode_info.bios_hardcoded_edid);
kfree(adev->amdgpu_vkms_output);
Then apply your patch on top to share more code with the existing dce files.
Alex
>
>
>
> From: Deucher, Alexander <alexander.deuc...@amd.com>
> Sent: 2021年11月23日 0:43
> To: Chen, Guchun <guchun.c...@amd.com>; Cui, Flora <flora....@amd.com>;
> amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/2] drm/amdgpu: fix vkms hrtimer settings
>
>
>
> [Public]
>
>
>
> Can you explain how the current code is failing? It's not immediately
> obvious to me. I'm not opposed to this change, it's just not clear to me
> where the current code fails.
>
>
>
> Alex
>
>
>
> ________________________________
>
> From: Chen, Guchun <guchun.c...@amd.com>
> Sent: Monday, November 22, 2021 8:49 AM
> To: Cui, Flora <flora....@amd.com>; amd-gfx@lists.freedesktop.org
> <amd-gfx@lists.freedesktop.org>; Deucher, Alexander
> <alexander.deuc...@amd.com>
> Subject: RE: [PATCH 1/2] drm/amdgpu: fix vkms hrtimer settings
>
>
>
> [Public]
>
> Series is:
> Reviewed-by: Guchun Chen <guchun.c...@amd.com>
>
> +Alex to comment this series as well.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Cui, Flora <flora....@amd.com>
> Sent: Monday, November 22, 2021 5:04 PM
> To: amd-gfx@lists.freedesktop.org; Chen, Guchun <guchun.c...@amd.com>
> Cc: Cui, Flora <flora....@amd.com>
> Subject: [PATCH 1/2] drm/amdgpu: fix vkms hrtimer settings
>
> otherwise adev->mode_info.crtcs[] is NULL
>
> Signed-off-by: Flora Cui <flora....@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 38 ++++++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.h | 5 ++--
> 2 files changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> index ce982afeff91..6c62c45e3e3e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> @@ -16,6 +16,8 @@
> #include "ivsrcid/ivsrcid_vislands30.h"
> #include "amdgpu_vkms.h"
> #include "amdgpu_display.h"
> +#include "atom.h"
> +#include "amdgpu_irq.h"
>
> /**
> * DOC: amdgpu_vkms
> @@ -41,14 +43,13 @@ static const u32 amdgpu_vkms_formats[] = {
>
> static enum hrtimer_restart amdgpu_vkms_vblank_simulate(struct hrtimer
> *timer) {
> - struct amdgpu_vkms_output *output = container_of(timer,
> - struct
> amdgpu_vkms_output,
> - vblank_hrtimer);
> - struct drm_crtc *crtc = &output->crtc;
> + struct amdgpu_crtc *amdgpu_crtc = container_of(timer, struct
> amdgpu_crtc, vblank_timer);
> + struct drm_crtc *crtc = &amdgpu_crtc->base;
> + struct amdgpu_vkms_output *output =
> +drm_crtc_to_amdgpu_vkms_output(crtc);
> u64 ret_overrun;
> bool ret;
>
> - ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> + ret_overrun = hrtimer_forward_now(&amdgpu_crtc->vblank_timer,
> output->period_ns);
> WARN_ON(ret_overrun != 1);
>
> @@ -65,22 +66,21 @@ static int amdgpu_vkms_enable_vblank(struct drm_crtc
> *crtc)
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> struct amdgpu_vkms_output *out =
> drm_crtc_to_amdgpu_vkms_output(crtc);
> + struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>
> drm_calc_timestamping_constants(crtc, &crtc->mode);
>
> - hrtimer_init(&out->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - out->vblank_hrtimer.function = &amdgpu_vkms_vblank_simulate;
> out->period_ns = ktime_set(0, vblank->framedur_ns);
> - hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
> + hrtimer_start(&amdgpu_crtc->vblank_timer, out->period_ns,
> +HRTIMER_MODE_REL);
>
> return 0;
> }
>
> static void amdgpu_vkms_disable_vblank(struct drm_crtc *crtc) {
> - struct amdgpu_vkms_output *out = drm_crtc_to_amdgpu_vkms_output(crtc);
> + struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>
> - hrtimer_cancel(&out->vblank_hrtimer);
> + hrtimer_cancel(&amdgpu_crtc->vblank_timer);
> }
>
> static bool amdgpu_vkms_get_vblank_timestamp(struct drm_crtc *crtc, @@
> -92,13 +92,14 @@ static bool amdgpu_vkms_get_vblank_timestamp(struct drm_crtc
> *crtc,
> unsigned int pipe = crtc->index;
> struct amdgpu_vkms_output *output =
> drm_crtc_to_amdgpu_vkms_output(crtc);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> + struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>
> if (!READ_ONCE(vblank->enabled)) {
> *vblank_time = ktime_get();
> return true;
> }
>
> - *vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
> + *vblank_time = READ_ONCE(amdgpu_crtc->vblank_timer.node.expires);
>
> if (WARN_ON(*vblank_time == vblank->time))
> return true;
> @@ -165,6 +166,8 @@ static const struct drm_crtc_helper_funcs
> amdgpu_vkms_crtc_helper_funcs = { static int amdgpu_vkms_crtc_init(struct
> drm_device *dev, struct drm_crtc *crtc,
> struct drm_plane *primary, struct drm_plane
> *cursor) {
> + struct amdgpu_device *adev = drm_to_adev(dev);
> + struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> int ret;
>
> ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor, @@
> -176,6 +179,17 @@ static int amdgpu_vkms_crtc_init(struct drm_device *dev,
> struct drm_crtc *crtc,
>
> drm_crtc_helper_add(crtc, &amdgpu_vkms_crtc_helper_funcs);
>
> + amdgpu_crtc->crtc_id = drm_crtc_index(crtc);
> + adev->mode_info.crtcs[drm_crtc_index(crtc)] = amdgpu_crtc;
> +
> + amdgpu_crtc->pll_id = ATOM_PPLL_INVALID;
> + amdgpu_crtc->encoder = NULL;
> + amdgpu_crtc->connector = NULL;
> + amdgpu_crtc->vsync_timer_enabled = AMDGPU_IRQ_STATE_DISABLE;
> +
> + hrtimer_init(&amdgpu_crtc->vblank_timer, CLOCK_MONOTONIC,
> HRTIMER_MODE_REL);
> + amdgpu_crtc->vblank_timer.function = &amdgpu_vkms_vblank_simulate;
> +
> return ret;
> }
>
> @@ -401,7 +415,7 @@ int amdgpu_vkms_output_init(struct drm_device *dev, {
> struct drm_connector *connector = &output->connector;
> struct drm_encoder *encoder = &output->encoder;
> - struct drm_crtc *crtc = &output->crtc;
> + struct drm_crtc *crtc = &output->crtc.base;
> struct drm_plane *primary, *cursor = NULL;
> int ret;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.h
> index 97f1b79c0724..4f8722ff37c2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.h
> @@ -10,15 +10,14 @@
> #define YRES_MAX 16384
>
> #define drm_crtc_to_amdgpu_vkms_output(target) \
> - container_of(target, struct amdgpu_vkms_output, crtc)
> + container_of(target, struct amdgpu_vkms_output, crtc.base)
>
> extern const struct amdgpu_ip_block_version amdgpu_vkms_ip_block;
>
> struct amdgpu_vkms_output {
> - struct drm_crtc crtc;
> + struct amdgpu_crtc crtc;
> struct drm_encoder encoder;
> struct drm_connector connector;
> - struct hrtimer vblank_hrtimer;
> ktime_t period_ns;
> struct drm_pending_vblank_event *event; };
> --
> 2.25.1
