[1/2] drm/radeon: Only enable and handle pageflip interrupts when needed

Commit Message

Michel Dänzer June 26, 2014, 9:29 a.m. UTC

From: Michel Dänzer <[e-mail]>

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 <[e-mail]>
Signed-off-by: Michel Dänzer <[e-mail]>
---
 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(-)

Comments

Christian König June 26, 2014, 10:39 a.m. UTC | #1

Am 26.06.2014 11:29, schrieb Michel Dänzer:
> From: Michel Dänzer <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Michel Dänzer <[e-mail]>

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) {

Dieter Nützel June 26, 2014, 11:51 a.m. UTC | #2

Am 26.06.2014 12:39, schrieb Christian König:
> Am 26.06.2014 11:29, schrieb Michel Dänzer:
>> From: Michel Dänzer <[e-mail]>
>> 
>> 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 <[e-mail]>
>> Signed-off-by: Michel Dänzer <[e-mail]>
> 
> 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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Dieter Nützel June 27, 2014, 12:53 a.m. UTC | #3

Am 26.06.2014 12:39, schrieb Christian König:
> Am 26.06.2014 11:29, schrieb Michel Dänzer:
>> From: Michel Dänzer <[e-mail]>
>> 
>> 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 <[e-mail]>
>> Signed-off-by: Michel Dänzer <[e-mail]>
> 
> 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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Michel Dänzer June 27, 2014, 1:03 a.m. UTC | #4

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Michel Dänzer <[e-mail]>
>>
>> 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.

Dieter Nützel June 27, 2014, 1:08 a.m. UTC | #5

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 <[e-mail]>
>>>> 
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Michel Dänzer <[e-mail]>
>>> 
>>> 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.

Dieter Nützel June 27, 2014, 2:06 a.m. UTC | #6

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 <[e-mail]>
>>>> 
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Michel Dänzer <[e-mail]>
>>> 
>>> 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.

Michel Dänzer June 27, 2014, 2:58 a.m. UTC | #7

On 26.06.2014 19:39, Christian König wrote:
> Am 26.06.2014 11:29, schrieb Michel Dänzer:
>> From: Michel Dänzer <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Michel Dänzer <[e-mail]>
> 
> 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.

Michel Dänzer June 27, 2014, 3:03 a.m. UTC | #8

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?

Michel Dänzer June 27, 2014, 3:17 a.m. UTC | #9

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.

Christian König June 27, 2014, 8:18 a.m. UTC | #10

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Michel Dänzer <[e-mail]>
>> 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.

Michel Dänzer June 27, 2014, 9:44 a.m. UTC | #11

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 <[e-mail]>
>>>> 
>>>> 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 <[e-mail]> Signed-off-by:
>>>> Michel Dänzer <[e-mail]>
>>> 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?

Christian König June 27, 2014, 10:47 a.m. UTC | #12

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 <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]> Signed-off-by:
>>>>> Michel Dänzer <[e-mail]>
>>>> 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.

Patch

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) {