Re: [PATCH 0/3] Cleanup evergreen/si IRQ handling code

[Christian KÃnig]

Am 16.05.2017 um 23:11 schrieb Lyude:
> This is the first part of me going through and cleaning up the IRQ handling
> code for radeon, since after taking a look at it the other day while trying to
> debug something I realized basically all of the code was copy pasted
> everywhere, and quite difficult to actually read through.
>
> Will come up with something for r600 and cik once I've got the chipsets on hand
> to test with.

Oh, yes please. This always annoyed me as well, but never had the time 
to clean it up globally.

Just two general comments:
1. Don't modify the register headers.

They are more or less from a database and we don't want manual code like 
this in there:
> +#define DISP_INTERRUPT_STATUS(i) \
> +	((i < 4) ? 0x60f4 + (0x4 * i) : 0x614c + (0x4 * (i - 4)))

Instead use a static constant array, e.g. like this:
static const uint32_t disp_interrupt_status[4] {
    DISP_INTERRUPT_STATUS,
    DISP_INTERRUPT_STATUS_CONTINUE,
...
};

2. Keep the order in which stuff is written to the regs exactly the same.

In other words, don't replace this:

> -	rdev->irq.stat_regs.evergreen.afmt_status1 = RREG32(AFMT_STATUS + EVERGREEN_CRTC0_REGISTER_OFFSET);
> -	rdev->irq.stat_regs.evergreen.afmt_status2 = RREG32(AFMT_STATUS + EVERGREEN_CRTC1_REGISTER_OFFSET);
> -	rdev->irq.stat_regs.evergreen.afmt_status3 = RREG32(AFMT_STATUS + EVERGREEN_CRTC2_REGISTER_OFFSET);
> -	rdev->irq.stat_regs.evergreen.afmt_status4 = RREG32(AFMT_STATUS + EVERGREEN_CRTC3_REGISTER_OFFSET);
> -	rdev->irq.stat_regs.evergreen.afmt_status5 = RREG32(AFMT_STATUS + EVERGREEN_CRTC4_REGISTER_OFFSET);
> -	rdev->irq.stat_regs.evergreen.afmt_status6 = RREG32(AFMT_STATUS + EVERGREEN_CRTC5_REGISTER_OFFSET);

With:

>   	for (i = 0; i < EVERGREEN_MAX_DISP_REGISTERS; i++) {
>   		disp_int[i] = RREG32(DISP_INTERRUPT_STATUS(i));
> +		afmt_status[i] = RREG32(AFMT_STATUS + crtc_offsets[i]);
>   
>   		if (disp_int[i] & DC_HPDx_INTERRUPT)
>   			WREG32_OR(DC_HPDx_INT_CONTROL(i), DC_HPDx_INT_ACK);
>   		if (disp_int[i] & DC_HPDx_RX_INTERRUPT)
>   			WREG32_OR(DC_HPDx_INT_CONTROL(i), DC_HPDx_RX_INT_ACK);
> +		if (afmt_status[i] & AFMT_AZ_FORMAT_WTRIG)
> +			WREG32_OR(AFMT_AUDIO_PACKET_CONTROL + crtc_offsets[i],
> +				  AFMT_AZ_FORMAT_WTRIG_ACK);

Regards,
Christian.

> Lyude (3):
>    drm/radeon: Cleanup display interrupt handling for evergreen, si
>    drm/radeon: Cleanup HDMI audio interrupt handling for evergreen
>    drm/radeon: Cleanup pageflipping IRQ handling for evergreen, si
>
>   drivers/gpu/drm/radeon/evergreen.c      | 915 +++++---------------------------
>   drivers/gpu/drm/radeon/evergreend.h     |  74 +--
>   drivers/gpu/drm/radeon/radeon.h         |  27 +-
>   drivers/gpu/drm/radeon/radeon_irq_kms.c |  35 ++
>   drivers/gpu/drm/radeon/si.c             | 627 ++++------------------
>   drivers/gpu/drm/radeon/sid.h            |  72 +--
>   6 files changed, 328 insertions(+), 1422 deletions(-)