Re: [PATCH] drm/amdkfd: Preserve cp_hqd_pq_control on update_mqd

[Philip Yang <yangp@xxxxxxx>]

On 2025-02-18 12:24, David Yat Sin wrote:
> When userspace applications call AMDKFD_IOC_UPDATE_QUEUE. Preserve
> bitfields that do not need to be modified as they contain flags to track
> queue states that are used by CP FW.
>
> Signed-off-by: David Yat Sin <David.YatSin@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c | 4 +++-
>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c | 4 +++-
>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v12.c | 4 +++-
>   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c  | 4 +++-
>   4 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
> index 2eff37aaf827..9aec222251f7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
> @@ -167,7 +167,9 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
>   
>   	m = get_mqd(mqd);
>   
> -	m->cp_hqd_pq_control = 5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT;
> +	m->cp_hqd_pq_control |= (5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT) &
> +			~CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE_MASK;
> +
>   	m->cp_hqd_pq_control |=
>   			ffs(q->queue_size / sizeof(unsigned int)) - 1 - 1;
>   	m->cp_hqd_pq_control |= CP_HQD_PQ_CONTROL__UNORD_DISPATCH_MASK;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> index 68dbc0399c87..c000dd50a4b5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> @@ -221,7 +221,9 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
>   
>   	m = get_mqd(mqd);
>   
> -	m->cp_hqd_pq_control = 5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT;
> +	m->cp_hqd_pq_control |= (5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT) &
> +			~CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE_MASK;
> +
The change makes sense, but seems typo

(5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT) & ~CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE_MASK is always 0,

maybe you want

(5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT) & CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE_MASK

or

m->cp_hqd_pq_control |= (5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT);

Regards,
Philip

>   	m->cp_hqd_pq_control |=
>   			ffs(q->queue_size / sizeof(unsigned int)) - 1 - 1;
>   	m->cp_hqd_pq_control |= CP_HQD_PQ_CONTROL__UNORD_DISPATCH_MASK;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v12.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v12.c
> index 2b72d5b4949b..337dd3fe130d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v12.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v12.c
> @@ -184,7 +184,9 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
>   
>   	m = get_mqd(mqd);
>   
> -	m->cp_hqd_pq_control = 5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT;
> +	m->cp_hqd_pq_control |= (5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT) &
> +			~CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE_MASK;
> +
>   	m->cp_hqd_pq_control |=
>   			ffs(q->queue_size / sizeof(unsigned int)) - 1 - 1;
>   	m->cp_hqd_pq_control |= CP_HQD_PQ_CONTROL__UNORD_DISPATCH_MASK;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> index ff417d5361c4..7d01ad6a686d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> @@ -245,7 +245,9 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
>   
>   	m = get_mqd(mqd);
>   
> -	m->cp_hqd_pq_control = 5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT;
> +	m->cp_hqd_pq_control |= (5 << CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE__SHIFT) &
> +			~CP_HQD_PQ_CONTROL__RPTR_BLOCK_SIZE_MASK;
> +
>   	m->cp_hqd_pq_control |= order_base_2(q->queue_size / 4) - 1;
>   	pr_debug("cp_hqd_pq_control 0x%x\n", m->cp_hqd_pq_control);
>