[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Christian König
deathsimple at vodafone.de
Mon Jun 30 05:31:59 PDT 2014
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 at 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 at nuetzel-hh.de> Signed-off-by:
>>>>>>> Michel Dänzer <michel.daenzer at 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 at 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);
>
>
More information about the dri-devel
mailing list