Hi, just following up on this patch. Thanks
On Tue, Sep 15, 2026 at 1:05 AM Mohammed Anees < [email protected]> wrote: > The DC scanout position helper already returns vblank start, vblank end, > horizontal position and vertical position as separate values. > > dm_crtc_get_scanoutpos() currently packs these values into the legacy > register-style vbl and position values, only for the amdgpu base driver > to unpack them again. > > Change page_flip_get_scanoutpos() to return the individual values > directly. Preserve the full 32 bit scanout position values returned > by DC instead of truncating them to 13 bits through legacy packed > register representation. > > For the legacy DCE implementations, keep decoding the packed hardware > register values in the backend before returning them through the common > interface. This keeps the register specific representation local to the > DCE code. > > Signed-off-by: Mohammed Anees <[email protected]> > --- > Changes in v2: > - Move scanout position assignments to earlier place. > - Restore the descriptive scanout position variable names. > - Link to v1: > https://lore.kernel.org/all/[email protected]/ > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 21 ++++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.h | 5 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 8 ++- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 20 +++++-- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 20 +++++-- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 20 +++++-- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 +++++----- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +- > .../display/amdgpu_dm/tests/amdgpu_dm_test.c | 56 ++++++++++++------- > 9 files changed, 125 insertions(+), 59 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index 59ea338b554c..88fb9b1abed2 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -1762,7 +1762,8 @@ int amdgpu_display_get_crtc_scanoutpos(struct > drm_device *dev, > int *hpos, ktime_t *stime, ktime_t *etime, > const struct drm_display_mode *mode) > { > - u32 vbl = 0, position = 0; > + u32 scan_vbl_start = 0, scan_vbl_end = 0; > + u32 scan_vpos = 0, scan_hpos = 0; > int vbl_start, vbl_end, vtotal, ret = 0; > bool in_vbl = true; > > @@ -1774,8 +1775,11 @@ int amdgpu_display_get_crtc_scanoutpos(struct > drm_device *dev, > if (stime) > *stime = ktime_get(); > > - if (amdgpu_display_page_flip_get_scanoutpos(adev, pipe, &vbl, > &position) == 0) > + if (amdgpu_display_page_flip_get_scanoutpos(adev, pipe, > + &scan_vbl_start, > &scan_vbl_end, > + &scan_vpos, > &scan_hpos) == 0) { > ret |= DRM_SCANOUTPOS_VALID; > + } > > /* Get optional system timestamp after query. */ > if (etime) > @@ -1783,16 +1787,15 @@ int amdgpu_display_get_crtc_scanoutpos(struct > drm_device *dev, > > /* preempt_enable_rt() should go right here in PREEMPT_RT > patchset. */ > > - /* Decode into vertical and horizontal scanout position. */ > - *vpos = position & 0x1fff; > - *hpos = (position >> 16) & 0x1fff; > + *vpos = scan_vpos; > + *hpos = scan_hpos; > > /* Valid vblank area boundaries from gpu retrieved? */ > - if (vbl > 0) { > - /* Yes: Decode. */ > + if (scan_vbl_start || scan_vbl_end) { > ret |= DRM_SCANOUTPOS_ACCURATE; > - vbl_start = vbl & 0x1fff; > - vbl_end = (vbl >> 16) & 0x1fff; > + vbl_start = scan_vbl_start; > + vbl_end = scan_vbl_end; > } else { > /* No: Fake something reasonable which gives at least ok > results. */ > vbl_start = mode->crtc_vdisplay; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h > index 49a29bf47a37..9de1891f50f4 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h > @@ -33,7 +33,10 @@ > #define amdgpu_display_hpd_get_gpio_reg(adev) > (adev)->mode_info.funcs->hpd_get_gpio_reg((adev)) > #define amdgpu_display_bandwidth_update(adev) > (adev)->mode_info.funcs->bandwidth_update((adev)) > #define amdgpu_display_page_flip(adev, crtc, base, async) > (adev)->mode_info.funcs->page_flip((adev), (crtc), (base), (async)) > -#define amdgpu_display_page_flip_get_scanoutpos(adev, crtc, vbl, pos) > (adev)->mode_info.funcs->page_flip_get_scanoutpos((adev), (crtc), (vbl), > (pos)) > +#define amdgpu_display_page_flip_get_scanoutpos(adev, crtc, > v_blank_start, v_blank_end, \ > + v_position, h_position) \ > + ((adev)->mode_info.funcs->page_flip_get_scanoutpos((adev), (crtc), > \ > + (v_blank_start), (v_blank_end), (v_position), > (h_position))) > #define amdgpu_display_add_encoder(adev, e, s, c) > (adev)->mode_info.funcs->add_encoder((adev), (e), (s), (c)) > #define amdgpu_display_add_connector(adev, ci, sd, ct, ib, coi, h, r) > (adev)->mode_info.funcs->add_connector((adev), (ci), (sd), (ct), (ib), > (coi), (h), (r)) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > index 7c784277396a..bb4d60698ee9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > @@ -277,8 +277,12 @@ struct amdgpu_display_funcs { > /* pageflipping */ > void (*page_flip)(struct amdgpu_device *adev, > int crtc_id, u64 crtc_base, bool async); > - int (*page_flip_get_scanoutpos)(struct amdgpu_device *adev, int > crtc, > - u32 *vbl, u32 *position); > + int (*page_flip_get_scanoutpos)(struct amdgpu_device *adev, > + int crtc, > + u32 *v_blank_start, > + u32 *v_blank_end, > + u32 *v_position, > + u32 *h_position); > /* display topology setup */ > void (*add_encoder)(struct amdgpu_device *adev, > uint32_t encoder_enum, > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > index 7de7adc8f225..cd12190fb962 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > @@ -256,14 +256,26 @@ static void dce_v10_0_page_flip(struct amdgpu_device > *adev, > RREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS + amdgpu_crtc->crtc_offset); > } > > -static int dce_v10_0_crtc_get_scanoutpos(struct amdgpu_device *adev, int > crtc, > - u32 *vbl, u32 *position) > +static int dce_v10_0_crtc_get_scanoutpos(struct amdgpu_device *adev, > + int crtc, > + u32 *v_blank_start, > + u32 *v_blank_end, > + u32 *v_position, > + u32 *h_position) > { > + u32 vbl, position; > + > if ((crtc < 0) || (crtc >= adev->mode_info.num_crtc)) > return -EINVAL; > > - *vbl = RREG32(mmCRTC_V_BLANK_START_END + crtc_offsets[crtc]); > - *position = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]); > + vbl = RREG32(mmCRTC_V_BLANK_START_END + crtc_offsets[crtc]); > + position = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]); > + > + *v_blank_start = vbl & 0x1fff; > + *v_blank_end = (vbl >> 16) & 0x1fff; > + > + *v_position = position & 0x1fff; > + *h_position = (position >> 16) & 0x1fff; > > return 0; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > index 3c1d6901127d..f2c60074f0f4 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > @@ -220,14 +220,26 @@ static void dce_v6_0_page_flip(struct amdgpu_device > *adev, > RREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS + amdgpu_crtc->crtc_offset); > } > > -static int dce_v6_0_crtc_get_scanoutpos(struct amdgpu_device *adev, int > crtc, > - u32 *vbl, u32 *position) > +static int dce_v6_0_crtc_get_scanoutpos(struct amdgpu_device *adev, > + int crtc, > + u32 *v_blank_start, > + u32 *v_blank_end, > + u32 *v_position, > + u32 *h_position) > { > + u32 vbl, position; > + > if ((crtc < 0) || (crtc >= adev->mode_info.num_crtc)) > return -EINVAL; > > - *vbl = RREG32(mmCRTC_V_BLANK_START_END + crtc_offsets[crtc]); > - *position = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]); > + vbl = RREG32(mmCRTC_V_BLANK_START_END + crtc_offsets[crtc]); > + position = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]); > + > + *v_blank_start = vbl & 0x1fff; > + *v_blank_end = (vbl >> 16) & 0x1fff; > + > + *v_position = position & 0x1fff; > + *h_position = (position >> 16) & 0x1fff; > > return 0; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > index a17944d5e843..5efdb5eea450 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > @@ -204,14 +204,26 @@ static void dce_v8_0_page_flip(struct amdgpu_device > *adev, > RREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS + amdgpu_crtc->crtc_offset); > } > > -static int dce_v8_0_crtc_get_scanoutpos(struct amdgpu_device *adev, int > crtc, > - u32 *vbl, u32 *position) > +static int dce_v8_0_crtc_get_scanoutpos(struct amdgpu_device *adev, > + int crtc, > + u32 *v_blank_start, > + u32 *v_blank_end, > + u32 *v_position, > + u32 *h_position) > { > + u32 vbl, position; > + > if ((crtc < 0) || (crtc >= adev->mode_info.num_crtc)) > return -EINVAL; > > - *vbl = RREG32(mmCRTC_V_BLANK_START_END + crtc_offsets[crtc]); > - *position = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]); > + vbl = RREG32(mmCRTC_V_BLANK_START_END + crtc_offsets[crtc]); > + position = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]); > + > + *v_blank_start = vbl & 0x1fff; > + *v_blank_end = (vbl >> 16) & 0x1fff; > + > + *v_position = position & 0x1fff; > + *h_position = (position >> 16) & 0x1fff; > > return 0; > } > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index df649ae66de2..3e8e45756bd4 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -188,10 +188,13 @@ STATIC_IFN_KUNIT u32 dm_vblank_get_counter(struct > amdgpu_device *adev, int crtc) > } > EXPORT_IF_KUNIT(dm_vblank_get_counter); > > -STATIC_IFN_KUNIT int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, > int crtc, > - u32 *vbl, u32 *position) > +STATIC_IFN_KUNIT int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, > + int crtc, > + u32 *v_blank_start, > + u32 *v_blank_end, > + u32 *v_position, > + u32 *h_position) > { > - u32 v_blank_start = 0, v_blank_end = 0, h_position = 0, v_position > = 0; > struct amdgpu_crtc *acrtc = NULL; > struct dc *dc = adev->dm.dc; > > @@ -209,18 +212,16 @@ STATIC_IFN_KUNIT int dm_crtc_get_scanoutpos(struct > amdgpu_device *adev, int crtc > if (dc && dc->caps.ips_support && dc->idle_optimizations_allowed) > dc_allow_idle_optimizations(dc, false); > > - /* > - * TODO rework base driver to use values directly. > - * for now parse it back into reg-format > - */ > - dc_stream_get_scanoutpos(acrtc->dm_irq_params.stream, > - &v_blank_start, > - &v_blank_end, > - &h_position, > - &v_position); > - > - *position = v_position | (h_position << 16); > - *vbl = v_blank_start | (v_blank_end << 16); > + if (!dc_stream_get_scanoutpos(acrtc->dm_irq_params.stream, > + v_blank_start, > + v_blank_end, > + h_position, > + v_position)) { > + *v_blank_start = 0; > + *v_blank_end = 0; > + *v_position = 0; > + *h_position = 0; > + } > > return 0; > } > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > index c99a84bb040e..a13042951a4b 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > @@ -1169,7 +1169,8 @@ int dm_set_powergating_state(struct amdgpu_ip_block > *ip_block, > void dm_bandwidth_update(struct amdgpu_device *adev); > u32 dm_vblank_get_counter(struct amdgpu_device *adev, int crtc); > int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, int crtc, > - u32 *vbl, u32 *position); > + u32 *v_blank_start, u32 *v_blank_end, > + u32 *v_position, u32 *h_position); > struct dm_atomic_state *dm_atomic_get_new_state(struct drm_atomic_commit > *state); > struct drm_private_state *dm_atomic_duplicate_state(struct > drm_private_obj *obj); > void dm_atomic_destroy_state(struct drm_private_obj *obj, > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c > index 6335fc23e977..88d868178068 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c > @@ -158,15 +158,19 @@ static void > dm_test_vblank_get_counter_no_stream(struct kunit *test) > static void dm_test_crtc_get_scanoutpos_invalid_crtc(struct kunit *test) > { > struct amdgpu_device *adev = dm_kunit_alloc_adev(test); > - u32 vbl = 0; > - u32 position = 0; > + u32 v_blank_start = 0; > + u32 v_blank_end = 0; > + u32 v_position = 0; > + u32 h_position = 0; > > adev->mode_info.num_crtc = 1; > > - KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, -1, &vbl, > &position), > - -EINVAL); > - KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 1, &vbl, > &position), > - -EINVAL); > + KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, -1, > + &v_blank_start, > &v_blank_end, > + &v_position, > &h_position), -EINVAL); > + KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 1, > + &v_blank_start, > &v_blank_end, > + &v_position, > &h_position), -EINVAL); > } > > /** > @@ -177,8 +181,10 @@ static void > dm_test_crtc_get_scanoutpos_no_stream(struct kunit *test) > { > struct amdgpu_device *adev = dm_kunit_alloc_adev(test); > struct amdgpu_crtc *acrtc; > - u32 vbl = 0; > - u32 position = 0; > + u32 v_blank_start = 0; > + u32 v_blank_end = 0; > + u32 v_position = 0; > + u32 h_position = 0; > > acrtc = kunit_kzalloc(test, sizeof(*acrtc), GFP_KERNEL); > KUNIT_ASSERT_NOT_NULL(test, acrtc); > @@ -186,9 +192,13 @@ static void > dm_test_crtc_get_scanoutpos_no_stream(struct kunit *test) > adev->mode_info.num_crtc = 1; > adev->mode_info.crtcs[0] = acrtc; > > - KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 0, &vbl, > &position), 0); > - KUNIT_EXPECT_EQ(test, vbl, 0U); > - KUNIT_EXPECT_EQ(test, position, 0U); > + KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 0, > + &v_blank_start, > &v_blank_end, > + &v_position, > &h_position), 0); > + KUNIT_EXPECT_EQ(test, v_blank_start, 0U); > + KUNIT_EXPECT_EQ(test, v_blank_end, 0U); > + KUNIT_EXPECT_EQ(test, v_position, 0U); > + KUNIT_EXPECT_EQ(test, h_position, 0U); > } > > /** > @@ -1324,14 +1334,20 @@ static void > dm_test_crtc_get_scanoutpos_unmapped_stream(struct kunit *test) > { > struct amdgpu_device *adev = dm_kunit_alloc_adev(test); > struct dc *dc = dm_kunit_alloc_dc_with_ctx(test); > - u32 vbl = 0xdeadbeef; > - u32 position = 0xdeadbeef; > + u32 v_blank_start = 0xdeadbeef; > + u32 v_blank_end = 0xdeadbeef; > + u32 v_position = 0xdeadbeef; > + u32 h_position = 0xdeadbeef; > > dm_test_crtc_with_stream(test, adev, dc); > > - KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 0, &vbl, > &position), 0); > - KUNIT_EXPECT_EQ(test, vbl, 0U); > - KUNIT_EXPECT_EQ(test, position, 0U); > + KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 0, > + &v_blank_start, > &v_blank_end, > + &v_position, > &h_position), 0); > + KUNIT_EXPECT_EQ(test, v_blank_start, 0U); > + KUNIT_EXPECT_EQ(test, v_blank_end, 0U); > + KUNIT_EXPECT_EQ(test, v_position, 0U); > + KUNIT_EXPECT_EQ(test, h_position, 0U); > } > > /** > @@ -1342,14 +1358,16 @@ static void > dm_test_crtc_get_scanoutpos_exits_idle(struct kunit *test) > { > struct amdgpu_device *adev = dm_kunit_alloc_adev(test); > struct dc *dc = dm_kunit_alloc_dc_with_ctx(test); > - u32 vbl = 0; > - u32 position = 0; > + u32 v_blank_start = 0, v_blank_end = 0; > + u32 v_position = 0, h_position = 0; > > dm_test_crtc_with_stream(test, adev, dc); > dc->caps.ips_support = true; > dc->idle_optimizations_allowed = true; > > - KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 0, &vbl, > &position), 0); > + KUNIT_EXPECT_EQ(test, dm_crtc_get_scanoutpos(adev, 0, > + &v_blank_start, > &v_blank_end, > + &v_position, > &h_position), 0); > } > > static struct drm_atomic_commit *dm_test_alloc_commit(struct kunit *test, > -- > 2.55.0 > >
