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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ab5a070-25ab-1e08-429a-9dcd9218a1af@amd.com>
Date:   Wed, 17 May 2017 10:32:38 +0200
From:   Christian König <christian.koenig@....com>
To:     Lyude <lyude@...hat.com>, <amd-gfx@...ts.freedesktop.org>
CC:     Alex Deucher <alexander.deucher@....com>,
        David Airlie <airlied@...ux.ie>,
        <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/3] Cleanup evergreen/si IRQ handling code

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