Message ID | 1403774992-3323-1-git-send-email-michel@daenzer.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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? I would rather want to avoid turning the pflip interrupt on and off. Christian. > --- > drivers/gpu/drm/radeon/cik.c | 18 ++++++++++-------- > drivers/gpu/drm/radeon/evergreen.c | 18 ++++++++++-------- > drivers/gpu/drm/radeon/r600.c | 12 ++++++++---- > drivers/gpu/drm/radeon/si.c | 18 ++++++++++-------- > 4 files changed, 38 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c > index 0f4b38f..7f522a4 100644 > --- a/drivers/gpu/drm/radeon/cik.c > +++ b/drivers/gpu/drm/radeon/cik.c > @@ -7145,21 +7145,21 @@ int cik_irq_set(struct radeon_device *rdev) > > if (rdev->num_crtc >= 2) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); > } > if (rdev->num_crtc >= 4) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); > } > if (rdev->num_crtc >= 6) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); > } > > WREG32(DC_HPD1_INT_CONTROL, hpd1); > @@ -7611,8 +7611,10 @@ restart_ih: > 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); > + src_id = (src_id - 8) >> 1; > + DRM_DEBUG("IH: D%d flip\n", src_id + 1); > + if (atomic_read(&rdev->irq.pflip[src_id])) > + radeon_crtc_handle_flip(rdev, src_id); > break; > case 42: /* HPD hotplug */ > switch (src_data) { > diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c > index 0443183..fa6e320 100644 > --- a/drivers/gpu/drm/radeon/evergreen.c > +++ b/drivers/gpu/drm/radeon/evergreen.c > @@ -4542,20 +4542,20 @@ int evergreen_irq_set(struct radeon_device *rdev) > } > > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); > if (rdev->num_crtc >= 4) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); > } > if (rdev->num_crtc >= 6) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); > } > > WREG32(DC_HPD1_INT_CONTROL, hpd1); > @@ -4950,8 +4950,10 @@ restart_ih: > 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); > + src_id = (src_id - 8) >> 1; > + DRM_DEBUG("IH: D%d flip\n", src_id + 1); > + if (atomic_read(&rdev->irq.pflip[src_id])) > + radeon_crtc_handle_flip(rdev, src_id); > break; > case 42: /* HPD hotplug */ > switch (src_data) { > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c > index ae54f76..2f9a2af 100644 > --- a/drivers/gpu/drm/radeon/r600.c > +++ b/drivers/gpu/drm/radeon/r600.c > @@ -3614,8 +3614,10 @@ int r600_irq_set(struct radeon_device *rdev) > WREG32(CP_INT_CNTL, cp_int_cntl); > WREG32(DMA_CNTL, dma_cntl); > WREG32(DxMODE_INT_MASK, mode_int); > - WREG32(D1GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); > - WREG32(D2GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); > + WREG32(D1GRPH_INTERRUPT_CONTROL, > + atomic_read(&rdev->irq.pflip[0]) ? DxGRPH_PFLIP_INT_MASK : 0); > + WREG32(D2GRPH_INTERRUPT_CONTROL, > + atomic_read(&rdev->irq.pflip[1]) ? DxGRPH_PFLIP_INT_MASK : 0); > WREG32(GRBM_INT_CNTL, grbm_int_cntl); > if (ASIC_IS_DCE3(rdev)) { > WREG32(DC_HPD1_INT_CONTROL, hpd1); > @@ -3920,11 +3922,13 @@ restart_ih: > break; > case 9: /* D1 pflip */ > DRM_DEBUG("IH: D1 flip\n"); > - radeon_crtc_handle_flip(rdev, 0); > + if (atomic_read(&rdev->irq.pflip[0])) > + radeon_crtc_handle_flip(rdev, 0); > break; > case 11: /* D2 pflip */ > DRM_DEBUG("IH: D2 flip\n"); > - radeon_crtc_handle_flip(rdev, 1); > + if (atomic_read(&rdev->irq.pflip[1])) > + radeon_crtc_handle_flip(rdev, 1); > break; > case 19: /* HPD/DAC hotplug */ > switch (src_data) { > diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c > index c128bde..5d37ea5 100644 > --- a/drivers/gpu/drm/radeon/si.c > +++ b/drivers/gpu/drm/radeon/si.c > @@ -5919,21 +5919,21 @@ int si_irq_set(struct radeon_device *rdev) > > if (rdev->num_crtc >= 2) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); > } > if (rdev->num_crtc >= 4) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); > } > if (rdev->num_crtc >= 6) { > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); > WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, > - GRPH_PFLIP_INT_MASK); > + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); > } > > if (!ASIC_IS_NODCE(rdev)) { > @@ -6303,8 +6303,10 @@ restart_ih: > 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); > + src_id = (src_id - 8) >> 1; > + DRM_DEBUG("IH: D%d flip\n", src_id + 1); > + if (atomic_read(&rdev->irq.pflip[src_id])) > + radeon_crtc_handle_flip(rdev, src_id); > break; > case 42: /* HPD hotplug */ > switch (src_data) {
Am 26.06.2014 12:39, schrieb Christian König: > 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? > > I would rather want to avoid turning the pflip interrupt on and off. > > Christian. Christian, I'll try for you after the doctor (my wife and I are sick over the last several weeks(!) ;-() and before 'Fußball' ;-))) --- That's why I'm on the 'hobel' that much and not in our forest, etc... But our two children are OK... All the above is my 'new/second' life... ...before that I've studied computer science with medicine as second part and had a deeper look into medical imagery. Cheers, Dieter >> --- >> drivers/gpu/drm/radeon/cik.c | 18 ++++++++++-------- >> drivers/gpu/drm/radeon/evergreen.c | 18 ++++++++++-------- >> drivers/gpu/drm/radeon/r600.c | 12 ++++++++---- >> drivers/gpu/drm/radeon/si.c | 18 ++++++++++-------- >> 4 files changed, 38 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/cik.c >> b/drivers/gpu/drm/radeon/cik.c >> index 0f4b38f..7f522a4 100644 >> --- a/drivers/gpu/drm/radeon/cik.c >> +++ b/drivers/gpu/drm/radeon/cik.c >> @@ -7145,21 +7145,21 @@ int cik_irq_set(struct radeon_device *rdev) >> if (rdev->num_crtc >= 2) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 4) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 6) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> WREG32(DC_HPD1_INT_CONTROL, hpd1); >> @@ -7611,8 +7611,10 @@ restart_ih: >> 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); >> + src_id = (src_id - 8) >> 1; >> + DRM_DEBUG("IH: D%d flip\n", src_id + 1); >> + if (atomic_read(&rdev->irq.pflip[src_id])) >> + radeon_crtc_handle_flip(rdev, src_id); >> break; >> case 42: /* HPD hotplug */ >> switch (src_data) { >> diff --git a/drivers/gpu/drm/radeon/evergreen.c >> b/drivers/gpu/drm/radeon/evergreen.c >> index 0443183..fa6e320 100644 >> --- a/drivers/gpu/drm/radeon/evergreen.c >> +++ b/drivers/gpu/drm/radeon/evergreen.c >> @@ -4542,20 +4542,20 @@ int evergreen_irq_set(struct radeon_device >> *rdev) >> } >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); >> if (rdev->num_crtc >= 4) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 6) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> WREG32(DC_HPD1_INT_CONTROL, hpd1); >> @@ -4950,8 +4950,10 @@ restart_ih: >> 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); >> + src_id = (src_id - 8) >> 1; >> + DRM_DEBUG("IH: D%d flip\n", src_id + 1); >> + if (atomic_read(&rdev->irq.pflip[src_id])) >> + radeon_crtc_handle_flip(rdev, src_id); >> break; >> case 42: /* HPD hotplug */ >> switch (src_data) { >> diff --git a/drivers/gpu/drm/radeon/r600.c >> b/drivers/gpu/drm/radeon/r600.c >> index ae54f76..2f9a2af 100644 >> --- a/drivers/gpu/drm/radeon/r600.c >> +++ b/drivers/gpu/drm/radeon/r600.c >> @@ -3614,8 +3614,10 @@ int r600_irq_set(struct radeon_device *rdev) >> WREG32(CP_INT_CNTL, cp_int_cntl); >> WREG32(DMA_CNTL, dma_cntl); >> WREG32(DxMODE_INT_MASK, mode_int); >> - WREG32(D1GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); >> - WREG32(D2GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); >> + WREG32(D1GRPH_INTERRUPT_CONTROL, >> + atomic_read(&rdev->irq.pflip[0]) ? DxGRPH_PFLIP_INT_MASK : >> 0); >> + WREG32(D2GRPH_INTERRUPT_CONTROL, >> + atomic_read(&rdev->irq.pflip[1]) ? DxGRPH_PFLIP_INT_MASK : >> 0); >> WREG32(GRBM_INT_CNTL, grbm_int_cntl); >> if (ASIC_IS_DCE3(rdev)) { >> WREG32(DC_HPD1_INT_CONTROL, hpd1); >> @@ -3920,11 +3922,13 @@ restart_ih: >> break; >> case 9: /* D1 pflip */ >> DRM_DEBUG("IH: D1 flip\n"); >> - radeon_crtc_handle_flip(rdev, 0); >> + if (atomic_read(&rdev->irq.pflip[0])) >> + radeon_crtc_handle_flip(rdev, 0); >> break; >> case 11: /* D2 pflip */ >> DRM_DEBUG("IH: D2 flip\n"); >> - radeon_crtc_handle_flip(rdev, 1); >> + if (atomic_read(&rdev->irq.pflip[1])) >> + radeon_crtc_handle_flip(rdev, 1); >> break; >> case 19: /* HPD/DAC hotplug */ >> switch (src_data) { >> diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c >> index c128bde..5d37ea5 100644 >> --- a/drivers/gpu/drm/radeon/si.c >> +++ b/drivers/gpu/drm/radeon/si.c >> @@ -5919,21 +5919,21 @@ int si_irq_set(struct radeon_device *rdev) >> if (rdev->num_crtc >= 2) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 4) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 6) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (!ASIC_IS_NODCE(rdev)) { >> @@ -6303,8 +6303,10 @@ restart_ih: >> 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); >> + src_id = (src_id - 8) >> 1; >> + DRM_DEBUG("IH: D%d flip\n", src_id + 1); >> + if (atomic_read(&rdev->irq.pflip[src_id])) >> + radeon_crtc_handle_flip(rdev, src_id); >> break; >> case 42: /* HPD hotplug */ >> switch (src_data) { > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 26.06.2014 12:39, schrieb Christian König: > 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? With #2 alone I get this during boot up (before plymouth): [ 11.942342] [drm] fb depth is 24 [ 11.942344] [drm] pitch is 7680 [ 11.942739] fbcon: radeondrmfb (fb0) is primary device [ 11.943090] [drm:radeon_crtc_handle_flip] *ERROR* radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) [ 11.943912] Console: switching to colour frame buffer device 240x67 [ 11.995623] radeon 0000:01:00.0: fb0: radeondrmfb frame buffer device [ 11.995628] radeon 0000:01:00.0: registered panic notifier [ 11.998112] [drm] Initialized radeon 2.39.0 20080528 for 0000:01:00.0 on minor 0 [ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR* radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) And during X / KDE start, again: [ 30.960442] [drm:radeon_crtc_handle_flip] *ERROR* radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) [ 31.343925] [drm:radeon_crtc_handle_flip] *ERROR* radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) [ 37.687138] [drm:radeon_crtc_handle_flip] *ERROR* radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) [ 187.005621] [drm:radeon_crtc_handle_flip] *ERROR* radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) Much more in dmesg. (See dmesg-3.16-rc2-Michel-2.xz) But with Michel's #1+2 and 3 I got this in Xorg.0.log: (See Xorg.0.log.old.xz) (EE) [mi] EQ overflowing. Additional events will be discarded until existing events are processed. (EE) (EE) Backtrace: > I would rather want to avoid turning the pflip interrupt on and off. I'm under the impression, that #2 alone is more responsive, but. Sorry, after Fußball ;-) Dieter >> --- >> drivers/gpu/drm/radeon/cik.c | 18 ++++++++++-------- >> drivers/gpu/drm/radeon/evergreen.c | 18 ++++++++++-------- >> drivers/gpu/drm/radeon/r600.c | 12 ++++++++---- >> drivers/gpu/drm/radeon/si.c | 18 ++++++++++-------- >> 4 files changed, 38 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/cik.c >> b/drivers/gpu/drm/radeon/cik.c >> index 0f4b38f..7f522a4 100644 >> --- a/drivers/gpu/drm/radeon/cik.c >> +++ b/drivers/gpu/drm/radeon/cik.c >> @@ -7145,21 +7145,21 @@ int cik_irq_set(struct radeon_device *rdev) >> if (rdev->num_crtc >= 2) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 4) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 6) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> WREG32(DC_HPD1_INT_CONTROL, hpd1); >> @@ -7611,8 +7611,10 @@ restart_ih: >> 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); >> + src_id = (src_id - 8) >> 1; >> + DRM_DEBUG("IH: D%d flip\n", src_id + 1); >> + if (atomic_read(&rdev->irq.pflip[src_id])) >> + radeon_crtc_handle_flip(rdev, src_id); >> break; >> case 42: /* HPD hotplug */ >> switch (src_data) { >> diff --git a/drivers/gpu/drm/radeon/evergreen.c >> b/drivers/gpu/drm/radeon/evergreen.c >> index 0443183..fa6e320 100644 >> --- a/drivers/gpu/drm/radeon/evergreen.c >> +++ b/drivers/gpu/drm/radeon/evergreen.c >> @@ -4542,20 +4542,20 @@ int evergreen_irq_set(struct radeon_device >> *rdev) >> } >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); >> if (rdev->num_crtc >= 4) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 6) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> WREG32(DC_HPD1_INT_CONTROL, hpd1); >> @@ -4950,8 +4950,10 @@ restart_ih: >> 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); >> + src_id = (src_id - 8) >> 1; >> + DRM_DEBUG("IH: D%d flip\n", src_id + 1); >> + if (atomic_read(&rdev->irq.pflip[src_id])) >> + radeon_crtc_handle_flip(rdev, src_id); >> break; >> case 42: /* HPD hotplug */ >> switch (src_data) { >> diff --git a/drivers/gpu/drm/radeon/r600.c >> b/drivers/gpu/drm/radeon/r600.c >> index ae54f76..2f9a2af 100644 >> --- a/drivers/gpu/drm/radeon/r600.c >> +++ b/drivers/gpu/drm/radeon/r600.c >> @@ -3614,8 +3614,10 @@ int r600_irq_set(struct radeon_device *rdev) >> WREG32(CP_INT_CNTL, cp_int_cntl); >> WREG32(DMA_CNTL, dma_cntl); >> WREG32(DxMODE_INT_MASK, mode_int); >> - WREG32(D1GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); >> - WREG32(D2GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); >> + WREG32(D1GRPH_INTERRUPT_CONTROL, >> + atomic_read(&rdev->irq.pflip[0]) ? DxGRPH_PFLIP_INT_MASK : >> 0); >> + WREG32(D2GRPH_INTERRUPT_CONTROL, >> + atomic_read(&rdev->irq.pflip[1]) ? DxGRPH_PFLIP_INT_MASK : >> 0); >> WREG32(GRBM_INT_CNTL, grbm_int_cntl); >> if (ASIC_IS_DCE3(rdev)) { >> WREG32(DC_HPD1_INT_CONTROL, hpd1); >> @@ -3920,11 +3922,13 @@ restart_ih: >> break; >> case 9: /* D1 pflip */ >> DRM_DEBUG("IH: D1 flip\n"); >> - radeon_crtc_handle_flip(rdev, 0); >> + if (atomic_read(&rdev->irq.pflip[0])) >> + radeon_crtc_handle_flip(rdev, 0); >> break; >> case 11: /* D2 pflip */ >> DRM_DEBUG("IH: D2 flip\n"); >> - radeon_crtc_handle_flip(rdev, 1); >> + if (atomic_read(&rdev->irq.pflip[1])) >> + radeon_crtc_handle_flip(rdev, 1); >> break; >> case 19: /* HPD/DAC hotplug */ >> switch (src_data) { >> diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c >> index c128bde..5d37ea5 100644 >> --- a/drivers/gpu/drm/radeon/si.c >> +++ b/drivers/gpu/drm/radeon/si.c >> @@ -5919,21 +5919,21 @@ int si_irq_set(struct radeon_device *rdev) >> if (rdev->num_crtc >= 2) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 4) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (rdev->num_crtc >= 6) { >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); >> WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, >> - GRPH_PFLIP_INT_MASK); >> + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); >> } >> if (!ASIC_IS_NODCE(rdev)) { >> @@ -6303,8 +6303,10 @@ restart_ih: >> 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); >> + src_id = (src_id - 8) >> 1; >> + DRM_DEBUG("IH: D%d flip\n", src_id + 1); >> + if (atomic_read(&rdev->irq.pflip[src_id])) >> + radeon_crtc_handle_flip(rdev, src_id); >> break; >> case 42: /* HPD hotplug */ >> switch (src_data) { > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 27.06.2014 09:53, Dieter Nützel wrote: > Am 26.06.2014 12:39, schrieb Christian König: >> 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? > > With #2 alone I get this during boot up (before plymouth): [...] > [ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR* > radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) That's the original patch I sent to you along with two others for testing. The patch I submitted in this series has these messages downgraded to debugging messages, as they just show the patch preventing bad stuff from happening as designed. The question is, can you reproduce the panic or the 'impossible msc' lines in the Xorg log with only patch #2? > But with Michel's #1+2 and 3 I got this in Xorg.0.log: > (See Xorg.0.log.old.xz) > > (EE) [mi] EQ overflowing. Additional events will be discarded until > existing events are processed. That may not be directly related to the page flipping issues.
Am 27.06.2014 03:03, schrieb Michel Dänzer: > On 27.06.2014 09:53, Dieter Nützel wrote: >> Am 26.06.2014 12:39, schrieb Christian König: >>> 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? >> >> With #2 alone I get this during boot up (before plymouth): > > [...] > >> [ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR* >> radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) > > That's the original patch I sent to you along with two others for > testing. The patch I submitted in this series has these messages > downgraded to debugging messages, as they just show the patch > preventing > bad stuff from happening as designed. > > The question is, can you reproduce the panic or the 'impossible msc' > lines in the Xorg log with only patch #2? Oh, shit sorry. I'll redo in a minute before I went to bed! -Dieter >> But with Michel's #1+2 and 3 I got this in Xorg.0.log: >> (See Xorg.0.log.old.xz) >> >> (EE) [mi] EQ overflowing. Additional events will be discarded until >> existing events are processed. > > That may not be directly related to the page flipping issues.
Am 27.06.2014 03:03, schrieb Michel Dänzer: > On 27.06.2014 09:53, Dieter Nützel wrote: >> Am 26.06.2014 12:39, schrieb Christian König: >>> 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? >> >> With #2 alone I get this during boot up (before plymouth): > > [...] > >> [ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR* >> radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) > > That's the original patch I sent to you along with two others for > testing. The patch I submitted in this series has these messages > downgraded to debugging messages, as they just show the patch > preventing > bad stuff from happening as designed. > > The question is, can you reproduce the panic or the 'impossible msc' > lines in the Xorg log with only patch #2? Here we go (the 'new' #2 alone): For the panic I need more time. But the first 'impossible msc' line arise during X start: [ 80.271] (II) RADEON(0): Modeline "1680x1050"x0.0 119.00 1680 1728 1760 1840 1050 1053 1059 1080 +hsync -vsync (64.7 kHz e) [ 80.271] (II) RADEON(0): Modeline "1920x1080"x60.0 172.80 1920 2040 2248 2576 1080 1081 1084 1118 -hsync +vsync (67.1 kHz e) [ 100.246] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 5302 < target_msc 5303 Some arise with KWin cube effects. [ 1059.124] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 62948 < target_msc 62949 [ 1067.223] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 63435 < target_msc 63436 [ 1081.458] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 64290 < target_msc 64291 [ 1233.165] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73416 < target_msc 73417 [ 1233.515] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73437 < target_msc 73438 [ 1234.330] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73486 < target_msc 73487 [ 1235.079] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73531 < target_msc 73532 Sleep well! -Dieter >> But with Michel's #1+2 and 3 I got this in Xorg.0.log: >> (See Xorg.0.log.old.xz) >> >> (EE) [mi] EQ overflowing. Additional events will be discarded until >> existing events are processed. > > That may not be directly related to the page flipping issues.
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 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. The diagnostic messages Dieter was getting with only patch #2 show that the pflip interrupt often triggers unnecessarily, potentially wasting power by waking up the CPU from a power saving state pointlessly.
On 27.06.2014 11:06, Dieter Nützel wrote: > Am 27.06.2014 03:03, schrieb Michel Dänzer: >> >> The question is, can you reproduce the panic or the 'impossible msc' >> lines in the Xorg log with only patch #2? > > Here we go (the 'new' #2 alone): > > For the panic I need more time. > But the first 'impossible msc' line arise during X start: > > [ 80.271] (II) RADEON(0): Modeline "1680x1050"x0.0 119.00 1680 1728 > 1760 1840 1050 1053 1059 1080 +hsync -vsync (64.7 kHz e) > [ 80.271] (II) RADEON(0): Modeline "1920x1080"x60.0 172.80 1920 > 2040 2248 2576 1080 1081 1084 1118 -hsync +vsync (67.1 kHz e) > [ 100.246] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip > completion event has impossible msc 5302 < target_msc 5303 > > Some arise with KWin cube effects. Once you've confirmed patch #2 alone avoids the panic, can you try if adding patch #1 again avoids the 'impossible msc' lines as well?
On 27.06.2014 12:03, Michel Dänzer wrote: > On 27.06.2014 11:06, Dieter Nützel wrote: >> Am 27.06.2014 03:03, schrieb Michel Dänzer: >>> >>> The question is, can you reproduce the panic or the 'impossible msc' >>> lines in the Xorg log with only patch #2? >> >> Here we go (the 'new' #2 alone): >> >> For the panic I need more time. >> But the first 'impossible msc' line arise during X start: >> >> [ 80.271] (II) RADEON(0): Modeline "1680x1050"x0.0 119.00 1680 1728 >> 1760 1840 1050 1053 1059 1080 +hsync -vsync (64.7 kHz e) >> [ 80.271] (II) RADEON(0): Modeline "1920x1080"x60.0 172.80 1920 >> 2040 2248 2576 1080 1081 1084 1118 -hsync +vsync (67.1 kHz e) >> [ 100.246] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip >> completion event has impossible msc 5302 < target_msc 5303 >> >> Some arise with KWin cube effects. > > Once you've confirmed patch #2 alone avoids the panic, can you try if > adding patch #1 again avoids the 'impossible msc' lines as well? Never mind, I was able to reproduce them with both patches.
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 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, this has already lead to quite some additional overhead in the fence waiting code. > > The diagnostic messages Dieter was getting with only patch #2 show that > the pflip interrupt often triggers unnecessarily, potentially wasting > power by waking up the CPU from a power saving state pointlessly. That's a really good point, but my question would rather be why does the pflip interrupt fires if there isn't any pflip? Turning the vblank and other interrupts off makes sense because they fire anyway, but pflip should only fire if we really made a request for it to do so. Regards, Christian.
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 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. > this has already lead to quite some additional overhead in the fence > waiting code. radeon_irq_set() should probably be split up to reduce the overhead. >> The diagnostic messages Dieter was getting with only patch #2 show >> that the pflip interrupt often triggers unnecessarily, potentially >> wasting power by waking up the CPU from a power saving state >> pointlessly. > That's a really good point, but my question would rather be why does > the pflip interrupt fires if there isn't any pflip? There is a page flip, but it already completes in the vertical blank interrupt handler in a lot of (most?) cases. Which brings me back to the question: Do we really need the pflip interrupt yet? [0] Since flips are no longer programmed to the hardware in the vertical blank handler but in a work queue, is there actually still any problem with handling the flip completion in the vertical blank interrupt handler? FWIW, by disabling the radeon_crtc_handle_flip() call from the pflip interrupt handler, I no longer seem to be able to reproduce the 'impossible msc' lines in the Xorg log file. [0] Of course the pflip interrupt will be needed for asynchronous flips, but that doesn't mean we have to use it for all flips?
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 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() . > >> this has already lead to quite some additional overhead in the fence >> waiting code. > radeon_irq_set() should probably be split up to reduce the overhead. Yeah, agree totally but didn't had time for this yet (it has a rather low priority). Might be a good task for someone to get his hands dirty on the kernel code for the first time as well. I think turning interrupts on and off at each fence wait is the reason why inlining r100_mm_rreg and r100_mm_wreg again turned out to improve performance for some people quite a bit. >>> The diagnostic messages Dieter was getting with only patch #2 show >>> that the pflip interrupt often triggers unnecessarily, potentially >>> wasting power by waking up the CPU from a power saving state >>> pointlessly. >> That's a really good point, but my question would rather be why does >> the pflip interrupt fires if there isn't any pflip? > There is a page flip, but it already completes in the vertical blank > interrupt handler in a lot of (most?) cases. The issue is still that as far as I understand it when the vblank interrupt fires the flip is actually not completed yet. So we only don't see the pending bit high any more because of the minimal time between vblank fires and we check the bit. 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. > Which brings me back to the question: Do we really need the pflip > interrupt yet? [0] Since flips are no longer programmed to the hardware > in the vertical blank handler but in a work queue, is there actually > still any problem with handling the flip completion in the vertical > blank interrupt handler? Good question, essentially we need to test with a TV capable of the 24Hz modes. I don't have the necessary hardware here and last time was only able to test this by torturing my poor HP monitor with a 24Hz modeline it actually couldn't handle. > FWIW, by disabling the radeon_crtc_handle_flip() call from the pflip > interrupt handler, I no longer seem to be able to reproduce the > 'impossible msc' lines in the Xorg log file. Good to know, that narrows down the scenario how this problem is triggered. > [0] Of course the pflip interrupt will be needed for asynchronous flips, > but that doesn't mean we have to use it for all flips? It seemed to be the right thing for the job, but at this time I planned to use it exclusively and stop bothering with the vblank interrupt at all. But then Alex noted that the pflip interrupt might be unreliable on older system and so I wanted to use the exiting vblank handling as backup. Now I'm not sure any more what's the best approach here. Regards, Christian.
diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c index 0f4b38f..7f522a4 100644 --- a/drivers/gpu/drm/radeon/cik.c +++ b/drivers/gpu/drm/radeon/cik.c @@ -7145,21 +7145,21 @@ int cik_irq_set(struct radeon_device *rdev) if (rdev->num_crtc >= 2) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); } if (rdev->num_crtc >= 4) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); } if (rdev->num_crtc >= 6) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); } WREG32(DC_HPD1_INT_CONTROL, hpd1); @@ -7611,8 +7611,10 @@ restart_ih: 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); + src_id = (src_id - 8) >> 1; + DRM_DEBUG("IH: D%d flip\n", src_id + 1); + if (atomic_read(&rdev->irq.pflip[src_id])) + radeon_crtc_handle_flip(rdev, src_id); break; case 42: /* HPD hotplug */ switch (src_data) { diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index 0443183..fa6e320 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -4542,20 +4542,20 @@ int evergreen_irq_set(struct radeon_device *rdev) } WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); if (rdev->num_crtc >= 4) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); } if (rdev->num_crtc >= 6) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); } WREG32(DC_HPD1_INT_CONTROL, hpd1); @@ -4950,8 +4950,10 @@ restart_ih: 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); + src_id = (src_id - 8) >> 1; + DRM_DEBUG("IH: D%d flip\n", src_id + 1); + if (atomic_read(&rdev->irq.pflip[src_id])) + radeon_crtc_handle_flip(rdev, src_id); break; case 42: /* HPD hotplug */ switch (src_data) { diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index ae54f76..2f9a2af 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -3614,8 +3614,10 @@ int r600_irq_set(struct radeon_device *rdev) WREG32(CP_INT_CNTL, cp_int_cntl); WREG32(DMA_CNTL, dma_cntl); WREG32(DxMODE_INT_MASK, mode_int); - WREG32(D1GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); - WREG32(D2GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK); + WREG32(D1GRPH_INTERRUPT_CONTROL, + atomic_read(&rdev->irq.pflip[0]) ? DxGRPH_PFLIP_INT_MASK : 0); + WREG32(D2GRPH_INTERRUPT_CONTROL, + atomic_read(&rdev->irq.pflip[1]) ? DxGRPH_PFLIP_INT_MASK : 0); WREG32(GRBM_INT_CNTL, grbm_int_cntl); if (ASIC_IS_DCE3(rdev)) { WREG32(DC_HPD1_INT_CONTROL, hpd1); @@ -3920,11 +3922,13 @@ restart_ih: break; case 9: /* D1 pflip */ DRM_DEBUG("IH: D1 flip\n"); - radeon_crtc_handle_flip(rdev, 0); + if (atomic_read(&rdev->irq.pflip[0])) + radeon_crtc_handle_flip(rdev, 0); break; case 11: /* D2 pflip */ DRM_DEBUG("IH: D2 flip\n"); - radeon_crtc_handle_flip(rdev, 1); + if (atomic_read(&rdev->irq.pflip[1])) + radeon_crtc_handle_flip(rdev, 1); break; case 19: /* HPD/DAC hotplug */ switch (src_data) { diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index c128bde..5d37ea5 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -5919,21 +5919,21 @@ int si_irq_set(struct radeon_device *rdev) if (rdev->num_crtc >= 2) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0); } if (rdev->num_crtc >= 4) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0); } if (rdev->num_crtc >= 6) { WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0); WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET, - GRPH_PFLIP_INT_MASK); + atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0); } if (!ASIC_IS_NODCE(rdev)) { @@ -6303,8 +6303,10 @@ restart_ih: 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); + src_id = (src_id - 8) >> 1; + DRM_DEBUG("IH: D%d flip\n", src_id + 1); + if (atomic_read(&rdev->irq.pflip[src_id])) + radeon_crtc_handle_flip(rdev, src_id); break; case 42: /* HPD hotplug */ switch (src_data) {