From: "Christian König" <deathsimple@vodafone.de>
To: "Michel Dänzer" <michel@daenzer.net>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Date: Mon, 30 Jun 2014 14:31:59 +0200 [thread overview]
Message-ID: <53B158BF.5060907@vodafone.de> (raw)
In-Reply-To: <53B12F38.2050208@daenzer.net>
Am 30.06.2014 11:34, schrieb Michel Dänzer:
> On 27.06.2014 19:47, Christian König wrote:
>> Am 27.06.2014 11:44, schrieb Michel Dänzer:
>>> On 27.06.2014 17:18, Christian König wrote:
>>>> Am 27.06.2014 04:58, schrieb Michel Dänzer:
>>>>> On 26.06.2014 19:39, Christian König wrote:
>>>>>> Am 26.06.2014 11:29, schrieb Michel Dänzer:
>>>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>>
>>>>>>> Prevents radeon_crtc_handle_flip() from running before
>>>>>>> radeon_flip_work_func(), resulting in a kernel panic due to
>>>>>>> the BUG_ON() in drm_vblank_put().
>>>>>>>
>>>>>>> Tested-by: Dieter Nützel <Dieter@nuetzel-hh.de> Signed-off-by:
>>>>>>> Michel Dänzer <michel.daenzer@amd.com>
>>>>>> Does patch #2 alone fixes the problem as well?
>>>>> It should avoid the panic as well.
> I've sent a v2 of that patch with an updated commit log. Alex, please get
> that into 3.16 ASAP to prevent people from running into the panic.
>
>
>>>>>> I would rather want to avoid turning the pflip interrupt on and
>>>>>> off.
>>>>> What's the problem with that? It's not like we're saving any
>>>>> register writes by not doing it.
>>>> We don't? As far as I can see we reprogram all interrupt registers
>>>> if any of the interrupts changed,
>>> Maybe I'm missing something, but: radeon_irq_kms_pflip_irq_get/put()
>>> call radeon_irq_set() every time, as there can only be one active page
>>> flip per CRTC. And radeon_irq_set() always writes the same registers,
>>> only the bits it writes to them change depending on which interrupts the
>>> driver is currently interested in.
>> We first turn on the vblank interrupt which results in a
>> radeon_irq_set() and then turn on the pflip, which results in another
>> radeon_irq_set() .
> The DRM core delays disabling the hardware vblank interrupt, so
> radeon_irq_set() should only be called once for that.
>
> radeon_irq_kms_pflip_irq_get/put() always call radeon_irq_set() even before
> my changes.
>
>
> Anyway, for the issues surrounding the pflip interrupt, I took a step back
> and considered a fundamentally different approach: It occurred to me that a
> lot of the issues we've been struggling with are related to programming the
> flips to the hardware such that they execute during the vertical blank
> period. We don't know exactly when the hardware update happens or when would
> be a good time to program the flip. So why bother with that at all?
I've considered this as well, but at this time still hoped that we could
completely replace the vblank interrupt with the pflip interrupt.
> The patch below (only fleshed out for CIK) moves the programming of the
> flip to the hardware back to the vertical blank interrupt handler, but
> changes it to execute during the horizontal blank period. In addition to
> allowing the pflip interrupt handling, radeon_crtc_handle_vblank() and
> radeon_flip_pending() to be removed, this should make adding support for
> asynchronous flips trivial and support for replacing pending flips much
> easier. What do you think?
It also allows us to stop fiddling with the update pending bit as well,
e.g. no more busy waiting for it to become high in evergreen_page_flip.
And as far as I can see it still support all not so trivial use cases
like switching to hblank if rendering isn't completed before a certain
timeout and dynamic refresh etc...
Yeah, sounds like a really good idea to me.
>> The delay between vblank start and the flip being executed seemed to be
>> depending on the pixel clock (which makes sense because the CRTC is
>> driven by it), so when it might work ok for a 50Hz mode we can still run
>> into problems with 24Hz modes.
> I couldn't see any tearing with this patch at 640x480 at 60 Hz and reduced
> blanking, which should have a lower pixel clock and shorter vertical blank
> period than 1920x1080 at 24 Hz?
In theory that should do the trick as well. But let's just make patches
for it and then we can make a request to the people who originally
reported the problem to test the whole implementation.
That should give us enough confidence that it will work correctly,
Christian.
>
>
> commit 7b2861589f05c5c5c752108ce105161ebc0d6bf5
> Author: Michel Dänzer <michel.daenzer@amd.com>
> Date: Mon Jun 30 17:57:01 2014 +0900
>
> drm/radeon: Program page flips to execute in hblank instead of vblank
>
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index e911898..2842013 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -1284,6 +1284,10 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> break;
> }
>
> + /* Make sure updates happen at vertical blank */
> + WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset, 0);
> + WREG32(EVERGREEN_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 0);
> +
> WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
> upper_32_bits(fb_location));
> WREG32(EVERGREEN_GRPH_SECONDARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
> @@ -1321,15 +1325,6 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
> WREG32(EVERGREEN_VIEWPORT_SIZE + radeon_crtc->crtc_offset,
> (viewport_w << 16) | viewport_h);
>
> - /* pageflip setup */
> - /* make sure flip is at vb rather than hb */
> - tmp = RREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset);
> - tmp &= ~EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN;
> - WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset, tmp);
> -
> - /* set pageflip to happen anywhere in vblank interval */
> - WREG32(EVERGREEN_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 0);
> -
> if (!atomic && fb && fb != crtc->primary->fb) {
> radeon_fb = to_radeon_framebuffer(fb);
> rbo = gem_to_radeon_bo(radeon_fb->obj);
> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> index 0f4b38f..d0a994c 100644
> --- a/drivers/gpu/drm/radeon/cik.c
> +++ b/drivers/gpu/drm/radeon/cik.c
> @@ -7143,25 +7143,6 @@ int cik_irq_set(struct radeon_device *rdev)
> WREG32(LB_INTERRUPT_MASK + EVERGREEN_CRTC5_REGISTER_OFFSET, crtc6);
> }
>
> - if (rdev->num_crtc >= 2) {
> - WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_MASK);
> - WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_MASK);
> - }
> - if (rdev->num_crtc >= 4) {
> - WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_MASK);
> - WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_MASK);
> - }
> - if (rdev->num_crtc >= 6) {
> - WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_MASK);
> - WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_MASK);
> - }
> -
> WREG32(DC_HPD1_INT_CONTROL, hpd1);
> WREG32(DC_HPD2_INT_CONTROL, hpd2);
> WREG32(DC_HPD3_INT_CONTROL, hpd3);
> @@ -7215,12 +7196,6 @@ static inline void cik_irq_ack(struct radeon_device *rdev)
> EVERGREEN_CRTC5_REGISTER_OFFSET);
> }
>
> - if (rdev->irq.stat_regs.cik.d1grph_int & GRPH_PFLIP_INT_OCCURRED)
> - WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC0_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_CLEAR);
> - if (rdev->irq.stat_regs.cik.d2grph_int & GRPH_PFLIP_INT_OCCURRED)
> - WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC1_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_CLEAR);
> if (rdev->irq.stat_regs.cik.disp_int & LB_D1_VBLANK_INTERRUPT)
> WREG32(LB_VBLANK_STATUS + EVERGREEN_CRTC0_REGISTER_OFFSET, VBLANK_ACK);
> if (rdev->irq.stat_regs.cik.disp_int & LB_D1_VLINE_INTERRUPT)
> @@ -7231,12 +7206,6 @@ static inline void cik_irq_ack(struct radeon_device *rdev)
> WREG32(LB_VLINE_STATUS + EVERGREEN_CRTC1_REGISTER_OFFSET, VLINE_ACK);
>
> if (rdev->num_crtc >= 4) {
> - if (rdev->irq.stat_regs.cik.d3grph_int & GRPH_PFLIP_INT_OCCURRED)
> - WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC2_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_CLEAR);
> - if (rdev->irq.stat_regs.cik.d4grph_int & GRPH_PFLIP_INT_OCCURRED)
> - WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC3_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_CLEAR);
> if (rdev->irq.stat_regs.cik.disp_int_cont2 & LB_D3_VBLANK_INTERRUPT)
> WREG32(LB_VBLANK_STATUS + EVERGREEN_CRTC2_REGISTER_OFFSET, VBLANK_ACK);
> if (rdev->irq.stat_regs.cik.disp_int_cont2 & LB_D3_VLINE_INTERRUPT)
> @@ -7248,12 +7217,6 @@ static inline void cik_irq_ack(struct radeon_device *rdev)
> }
>
> if (rdev->num_crtc >= 6) {
> - if (rdev->irq.stat_regs.cik.d5grph_int & GRPH_PFLIP_INT_OCCURRED)
> - WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC4_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_CLEAR);
> - if (rdev->irq.stat_regs.cik.d6grph_int & GRPH_PFLIP_INT_OCCURRED)
> - WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC5_REGISTER_OFFSET,
> - GRPH_PFLIP_INT_CLEAR);
> if (rdev->irq.stat_regs.cik.disp_int_cont4 & LB_D5_VBLANK_INTERRUPT)
> WREG32(LB_VBLANK_STATUS + EVERGREEN_CRTC4_REGISTER_OFFSET, VBLANK_ACK);
> if (rdev->irq.stat_regs.cik.disp_int_cont4 & LB_D5_VLINE_INTERRUPT)
> @@ -7459,7 +7422,7 @@ restart_ih:
> wake_up(&rdev->irq.vblank_queue);
> }
> if (atomic_read(&rdev->irq.pflip[0]))
> - radeon_crtc_handle_vblank(rdev, 0);
> + radeon_crtc_handle_flip(rdev, 0);
> rdev->irq.stat_regs.cik.disp_int &= ~LB_D1_VBLANK_INTERRUPT;
> DRM_DEBUG("IH: D1 vblank\n");
> }
> @@ -7485,7 +7448,7 @@ restart_ih:
> wake_up(&rdev->irq.vblank_queue);
> }
> if (atomic_read(&rdev->irq.pflip[1]))
> - radeon_crtc_handle_vblank(rdev, 1);
> + radeon_crtc_handle_flip(rdev, 1);
> rdev->irq.stat_regs.cik.disp_int_cont &= ~LB_D2_VBLANK_INTERRUPT;
> DRM_DEBUG("IH: D2 vblank\n");
> }
> @@ -7511,7 +7474,7 @@ restart_ih:
> wake_up(&rdev->irq.vblank_queue);
> }
> if (atomic_read(&rdev->irq.pflip[2]))
> - radeon_crtc_handle_vblank(rdev, 2);
> + radeon_crtc_handle_flip(rdev, 2);
> rdev->irq.stat_regs.cik.disp_int_cont2 &= ~LB_D3_VBLANK_INTERRUPT;
> DRM_DEBUG("IH: D3 vblank\n");
> }
> @@ -7537,7 +7500,7 @@ restart_ih:
> wake_up(&rdev->irq.vblank_queue);
> }
> if (atomic_read(&rdev->irq.pflip[3]))
> - radeon_crtc_handle_vblank(rdev, 3);
> + radeon_crtc_handle_flip(rdev, 3);
> rdev->irq.stat_regs.cik.disp_int_cont3 &= ~LB_D4_VBLANK_INTERRUPT;
> DRM_DEBUG("IH: D4 vblank\n");
> }
> @@ -7563,7 +7526,7 @@ restart_ih:
> wake_up(&rdev->irq.vblank_queue);
> }
> if (atomic_read(&rdev->irq.pflip[4]))
> - radeon_crtc_handle_vblank(rdev, 4);
> + radeon_crtc_handle_flip(rdev, 4);
> rdev->irq.stat_regs.cik.disp_int_cont4 &= ~LB_D5_VBLANK_INTERRUPT;
> DRM_DEBUG("IH: D5 vblank\n");
> }
> @@ -7589,7 +7552,7 @@ restart_ih:
> wake_up(&rdev->irq.vblank_queue);
> }
> if (atomic_read(&rdev->irq.pflip[5]))
> - radeon_crtc_handle_vblank(rdev, 5);
> + radeon_crtc_handle_flip(rdev, 5);
> rdev->irq.stat_regs.cik.disp_int_cont5 &= ~LB_D6_VBLANK_INTERRUPT;
> DRM_DEBUG("IH: D6 vblank\n");
> }
> @@ -7605,15 +7568,6 @@ restart_ih:
> break;
> }
> break;
> - case 8: /* D1 page flip */
> - case 10: /* D2 page flip */
> - case 12: /* D3 page flip */
> - case 14: /* D4 page flip */
> - case 16: /* D5 page flip */
> - case 18: /* D6 page flip */
> - DRM_DEBUG("IH: D%d flip\n", ((src_id - 8) >> 1) + 1);
> - radeon_crtc_handle_flip(rdev, (src_id - 8) >> 1);
> - break;
> case 42: /* HPD hotplug */
> switch (src_data) {
> case 0:
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index 0443183..4798d61 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -1317,7 +1317,10 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base)
> {
> struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
> u32 tmp = RREG32(EVERGREEN_GRPH_UPDATE + radeon_crtc->crtc_offset);
> - int i;
> +
> + /* Take surface updates at horizontal blank */
> + WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset,
> + EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN);
>
> /* Lock the graphics update lock */
> tmp |= EVERGREEN_GRPH_UPDATE_LOCK;
> @@ -1334,15 +1337,7 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base)
> WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS + radeon_crtc->crtc_offset,
> (u32)crtc_base);
>
> - /* Wait for update_pending to go high. */
> - for (i = 0; i < rdev->usec_timeout; i++) {
> - if (RREG32(EVERGREEN_GRPH_UPDATE + radeon_crtc->crtc_offset) & EVERGREEN_GRPH_SURFACE_UPDATE_PENDING)
> - break;
> - udelay(1);
> - }
> - DRM_DEBUG("Update pending now high. Unlocking vupdate_lock.\n");
> -
> - /* Unlock the lock, so double-buffering can take place inside vblank */
> + /* Unlock the lock, so double-buffering can take place inside hblank */
> tmp &= ~EVERGREEN_GRPH_UPDATE_LOCK;
> WREG32(EVERGREEN_GRPH_UPDATE + radeon_crtc->crtc_offset, tmp);
> }
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 8e8ef08..c8b7f26 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -681,6 +681,7 @@ struct radeon_flip_work {
> struct work_struct unpin_work;
> struct radeon_device *rdev;
> int crtc_id;
> + uint64_t base;
> struct drm_framebuffer *fb;
> struct drm_pending_vblank_event *event;
> struct radeon_bo *old_rbo;
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 65882cd..9a7554c 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -354,6 +354,9 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
> return;
> }
>
> + /* do the flip (mmio) */
> + radeon_page_flip(rdev, radeon_crtc->crtc_id, work->base);
> +
> /* Pageflip completed. Clean up. */
> radeon_crtc->flip_status = RADEON_FLIP_NONE;
> radeon_crtc->flip_work = NULL;
> @@ -479,9 +482,7 @@ static void radeon_flip_work_func(struct work_struct *__work)
> /* set the proper interrupt */
> radeon_irq_kms_pflip_irq_get(rdev, radeon_crtc->crtc_id);
>
> - /* do the flip (mmio) */
> - radeon_page_flip(rdev, radeon_crtc->crtc_id, base);
> -
> + work->base = base;
> radeon_crtc->flip_status = RADEON_FLIP_SUBMITTED;
> spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> up_read(&rdev->exclusive_lock);
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2014-06-30 12:32 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-26 9:29 [PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed Michel Dänzer
2014-06-26 9:29 ` [PATCH 2/2] drm/radeon: Track the status of a page flip more explicitly Michel Dänzer
2014-06-30 9:12 ` [PATCH v2] " Michel Dänzer
2014-06-30 9:14 ` Christian König
2014-07-01 16:23 ` Alex Deucher
2014-06-26 10:39 ` [PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed Christian König
2014-06-26 11:51 ` Dieter Nützel
2014-06-27 0:53 ` Dieter Nützel
2014-06-27 1:03 ` Michel Dänzer
2014-06-27 1:08 ` Dieter Nützel
2014-06-27 2:06 ` Dieter Nützel
2014-06-27 3:03 ` Michel Dänzer
2014-06-27 3:17 ` Michel Dänzer
2014-06-27 2:58 ` Michel Dänzer
2014-06-27 8:18 ` Christian König
2014-06-27 9:44 ` Michel Dänzer
2014-06-27 10:47 ` Christian König
2014-06-30 9:34 ` Michel Dänzer
2014-06-30 12:31 ` Christian König [this message]
2014-06-30 16:22 ` Dieter Nützel
2014-07-01 8:14 ` [PATCH 1/2] drm/radeon: Program page flips to execute in hblank instead of vblank Michel Dänzer
2014-07-01 8:14 ` [PATCH 2/2] drm/radeon: Remove radeon_kms_pflip_irq_get/put() Michel Dänzer
2014-07-01 10:12 ` [PATCH 1/2] drm/radeon: Program page flips to execute in hblank instead of vblank Christian König
2014-07-02 3:55 ` [PATCH v2 " Michel Dänzer
2014-07-02 3:55 ` [PATCH v2 2/2] drm/radeon: Remove radeon_kms_pflip_irq_get/put() and the pflip atomic Michel Dänzer
2014-07-02 10:10 ` [PATCH 1/2] drm/radeon: Move pinning the BO back to radeon_crtc_page_flip() Michel Dänzer
2014-07-02 10:10 ` [PATCH 2/2] drm/radeon: Complete page flip even if waiting on the BO fence fails Michel Dänzer
2014-07-02 11:42 ` [PATCH 1/2] drm/radeon: Move pinning the BO back to radeon_crtc_page_flip() Christian König
2014-07-02 11:35 ` [PATCH v2 1/2] drm/radeon: Program page flips to execute in hblank instead of vblank Christian König
2014-07-01 16:46 ` [PATCH " Dieter Nützel
2014-07-02 0:29 ` Michel Dänzer
2014-07-02 3:01 ` Dieter Nützel
2014-07-02 3:11 ` Michel Dänzer
2014-07-02 3:52 ` Michel Dänzer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53B158BF.5060907@vodafone.de \
--to=deathsimple@vodafone.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=michel@daenzer.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.