[PATCH 02/11] drm/amdgpu: send IVs to the KFD only after processing them v2

[Christian König]

Am 30.11.18 um 21:55 schrieb Zeng, Oak:
> See comment [Oak]
>
> Thanks,
> Oak
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Christian König
> Sent: Friday, November 30, 2018 7:36 AM
> To: amd-gfx at lists.freedesktop.org
> Subject: [PATCH 02/11] drm/amdgpu: send IVs to the KFD only after processing them v2
>
> This allows us to filter out VM faults in the GMC code.
>
> v2: don't filter out all faults
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 29 +++++++++++++++----------
>   1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 6b6524f04ce0..6db4c58ddc13 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -149,9 +149,6 @@ static void amdgpu_irq_callback(struct amdgpu_device *adev,
>   	if (!amdgpu_ih_prescreen_iv(adev))
>   		return;
>   
> -	/* Before dispatching irq to IP blocks, send it to amdkfd */
> -	amdgpu_amdkfd_interrupt(adev, (const void *) &ih->ring[ring_index]);
> -
>   	entry.iv_entry = (const uint32_t *)&ih->ring[ring_index];
>   	amdgpu_ih_decode_iv(adev, &entry);
>   
> @@ -371,29 +368,31 @@ void amdgpu_irq_dispatch(struct amdgpu_device *adev,
>   	unsigned client_id = entry->client_id;
>   	unsigned src_id = entry->src_id;
>   	struct amdgpu_irq_src *src;
> +	bool handled = false;
>   	int r;
>   
>   	trace_amdgpu_iv(entry);
>   
>   	if (client_id >= AMDGPU_IRQ_CLIENTID_MAX) {
> -		DRM_DEBUG("Invalid client_id in IV: %d\n", client_id);
> +		DRM_ERROR("Invalid client_id in IV: %d\n", client_id);
>   		return;
>   	}
>   
>   	if (src_id >= AMDGPU_MAX_IRQ_SRC_ID) {
> -		DRM_DEBUG("Invalid src_id in IV: %d\n", src_id);
> +		DRM_ERROR("Invalid src_id in IV: %d\n", src_id);
>   		return;
>   	}
>   
>   	if (adev->irq.virq[src_id]) {
>   		generic_handle_irq(irq_find_mapping(adev->irq.domain, src_id));
> -	} else {
> -		if (!adev->irq.client[client_id].sources) {
> -			DRM_DEBUG("Unregistered interrupt client_id: %d src_id: %d\n",
> -				  client_id, src_id);
> -			return;
> -		}
> +		return;
> +	}
>   
> +	if (!adev->irq.client[client_id].sources) {
> +		DRM_DEBUG("Unregistered interrupt client_id: %d src_id: %d\n",
> +			  client_id, src_id);
> +		return;
> +	} else {
> [Oak]: you probably don't need a else because the return in above if.
Ah, crap! Thanks for pointing this out, it is a copy&paste error.
>   
>   		src = adev->irq.client[client_id].sources[src_id];
>   		if (!src) {
>   			DRM_DEBUG("Unhandled interrupt src_id: %d\n", src_id); @@ -401,9 +400,15 @@ void amdgpu_irq_dispatch(struct amdgpu_device *adev,
>   		}
>   
>   		r = src->funcs->process(adev, src, entry);
> -		if (r)
> +		if (r < 0)
>   			DRM_ERROR("error processing interrupt (%d)\n", r);
> +		else if (r)
> +			handled = true;
>   	}
> +
> +	/* Send it to amdkfd as well if it isn't already handled */
> +	if (!handled)
> [Oak]: Kfd cares about followed interrupt source/client. How can we guarantee that those are not handled by registered interrupt handler at amdgpu level?
> 		source_id == SOC15_INTSRC_CP_END_OF_PIPE ||
> 		source_id == SOC15_INTSRC_SDMA_TRAP ||
> 		source_id == SOC15_INTSRC_SQ_INTERRUPT_MSG ||
> 		source_id == SOC15_INTSRC_CP_BAD_OPCODE ||
> 		client_id == SOC15_IH_CLIENTID_VMC ||
> 		client_id == SOC15_IH_CLIENTID_UTCL2;

Well the whole idea is that we filter out some of the IVs so that KFD 
does not see them any more.

>
> To completely avoid this, we should remove the amdgpu_amdkfd_interrupt function. Instead, kfd interrupt handling functions should be splitted and registered to adev->irq.client[].sources[]

Yeah, completely agree. But I wasn't sure which events the KFD actually 
needs, so the filtering I added for now was made to avoid problems for now.

In the long term we should indeed remove the dispatching from the kfd 
interrupt handling and move everything into the amdgpu per IP handlers.

Regards,
Christian.

>
> +		amdgpu_amdkfd_interrupt(adev, entry->iv_entry);
>   }
>   
>   /**
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx