Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: jcrouse=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 2239EC433C6
Date:   Tue, 3 Nov 2020 10:29:52 -0700
From:   Jordan Crouse <jcrouse@codeaurora.org>
To:     Marijn Suijten <marijn.suijten@somainline.org>
Cc:     robdclark@gmail.com, konrad.dybcio@somainline.org,
        martin.botka@somainline.org, phone-devel@vger.kernel.org,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@somainline.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Konrad Dybcio <konradybcio@gmail.com>,
        AngeloGioacchino Del Regno <kholk11@gmail.com>,
        Eric Anholt <eric@anholt.net>, linux-arm-msm@vger.kernel.org,
        dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/msm: a5xx: Make preemption reset case reentrant
Message-ID: <20201103172951.GB5934@jcrouse1-lnx.qualcomm.com>
Mail-Followup-To: Marijn Suijten <marijn.suijten@somainline.org>,
        robdclark@gmail.com, konrad.dybcio@somainline.org,
        martin.botka@somainline.org, phone-devel@vger.kernel.org,
        AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Konrad Dybcio <konradybcio@gmail.com>,
        AngeloGioacchino Del Regno <kholk11@gmail.com>,
        Eric Anholt <eric@anholt.net>, linux-arm-msm@vger.kernel.org,
        dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20201102200227.8876-1-marijn.suijten@somainline.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201102200227.8876-1-marijn.suijten@somainline.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Mon, Nov 02, 2020 at 09:02:25PM +0100, Marijn Suijten wrote:
> nr_rings is reset to 1, but when this function is called for a second
> (and third!) time nr_rings > 1 is false, thus the else case is entered
> to set up a buffer for the RPTR shadow and consequently written to
> RB_RPTR_ADDR, hanging platforms without WHERE_AM_I firmware support.
> 
> Restructure the condition in such a way that shadow buffer setup only
> ever happens when has_whereami is true; otherwise preemption is only
> finalized when the number of ring buffers has not been reset to 1 yet.
> 
> Fixes: 8907afb476ac ("drm/msm: Allow a5xx to mark the RPTR shadow as privileged")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

Way better. Thanks for doing this.

Reviewed-by: Jordan Crouse <jcrouse@codeaurora.org>

> ---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index d6804a802355..9a202a7da131 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -755,12 +755,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  	gpu_write(gpu, REG_A5XX_CP_RB_CNTL,
>  		MSM_GPU_RB_CNTL_DEFAULT | AXXX_CP_RB_CNTL_NO_UPDATE);
>  
> -	/* Disable preemption if WHERE_AM_I isn't available */
> -	if (!a5xx_gpu->has_whereami && gpu->nr_rings > 1) {
> -		a5xx_preempt_fini(gpu);
> -		gpu->nr_rings = 1;
> -	} else {
> -		/* Create a privileged buffer for the RPTR shadow */
> +	/* Create a privileged buffer for the RPTR shadow */
> +	if (a5xx_gpu->has_whereami) {
>  		if (!a5xx_gpu->shadow_bo) {
>  			a5xx_gpu->shadow = msm_gem_kernel_new(gpu->dev,
>  				sizeof(u32) * gpu->nr_rings,
> @@ -774,6 +770,10 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
>  
>  		gpu_write64(gpu, REG_A5XX_CP_RB_RPTR_ADDR,
>  			REG_A5XX_CP_RB_RPTR_ADDR_HI, shadowptr(a5xx_gpu, gpu->rb[0]));
> +	} else if (gpu->nr_rings > 1) {
> +		/* Disable preemption if WHERE_AM_I isn't available */
> +		a5xx_preempt_fini(gpu);
> +		gpu->nr_rings = 1;
>  	}
>  
>  	a5xx_preempt_hw_init(gpu);
> -- 
> 2.29.2
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project