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

Christian König
2017-05-17T08:33:00Z

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