Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <001d7571-5e9f-4f60-f6d0-35806a3e51c5@linaro.org>
Date:   Thu, 15 Jun 2023 12:34:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.2
Subject: Re: [PATCH v8 07/18] drm/msm/a6xx: Add a helper for
 software-resetting the GPU
Content-Language: en-US
To:     Akhil P Oommen <quic_akhilpo@quicinc.com>
Cc:     Rob Clark <robdclark@gmail.com>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Conor Dooley <conor+dt@kernel.org>,
        linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
        freedreno@lists.freedesktop.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Rob Clark <robdclark@chromium.org>,
        Marijn Suijten <marijn.suijten@somainline.org>
References: <20230223-topic-gmuwrapper-v8-0-69c68206609e@linaro.org>
 <20230223-topic-gmuwrapper-v8-7-69c68206609e@linaro.org>
 <jplt5g5xuphbnci73pdtaxd63fguxtgtp4c37kc7ehavzrjbau@kamshezxnvgy>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <jplt5g5xuphbnci73pdtaxd63fguxtgtp4c37kc7ehavzrjbau@kamshezxnvgy>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6.06.2023 19:18, Akhil P Oommen wrote:
> On Mon, May 29, 2023 at 03:52:26PM +0200, Konrad Dybcio wrote:
>>
>> Introduce a6xx_gpu_sw_reset() in preparation for adding GMU wrapper
>> GPUs and reuse it in a6xx_gmu_force_off().
>>
>> This helper, contrary to the original usage in GMU code paths, adds
>> a write memory barrier which together with the necessary delay should
>> ensure that the reset is never deasserted too quickly due to e.g. OoO
>> execution going crazy.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c |  3 +--
>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++++++++
>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  1 +
>>  3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> index b86be123ecd0..5ba8cba69383 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> @@ -899,8 +899,7 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)
>>  	a6xx_bus_clear_pending_transactions(adreno_gpu, true);
>>  
>>  	/* Reset GPU core blocks */
>> -	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, 1);
>> -	udelay(100);
>> +	a6xx_gpu_sw_reset(gpu, true);
>>  }
>>  
>>  static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index e3ac3f045665..083ccb5bcb4e 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -1634,6 +1634,17 @@ void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_
>>  	gpu_write(gpu, REG_A6XX_GBIF_HALT, 0x0);
>>  }
>>  
>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert)
>> +{
>> +	gpu_write(gpu, REG_A6XX_RBBM_SW_RESET_CMD, assert);
>> +	/* Add a barrier to avoid bad surprises */
> Can you please make this comment a bit more clear? Highlight that we
> should ensure the register is posted at hw before polling.
> 
> I think this barrier is required only during assert.
Generally it should not be strictly required at all, but I'm thinking
that it'd be good to keep it in both cases, so that:

if (assert)
	we don't keep writing things to the GPU if it's in reset
else
	we don't start writing things to the GPU becomes it comes
	out of reset

Also, if you squint hard enough at the commit message, you'll notice
I intended for this so only be a wmb, but for some reason generalized
it.. Perhaps that's another thing I should fix!
for v9..

Konrad
> 
> -Akhil.
>> +	mb();
>> +
>> +	/* The reset line needs to be asserted for at least 100 us */
>> +	if (assert)
>> +		udelay(100);
>> +}
>> +
>>  static int a6xx_pm_resume(struct msm_gpu *gpu)
>>  {
>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> index 9580def06d45..aa70390ee1c6 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
>> @@ -89,5 +89,6 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
>>  int a6xx_gpu_state_put(struct msm_gpu_state *state);
>>  
>>  void a6xx_bus_clear_pending_transactions(struct adreno_gpu *adreno_gpu, bool gx_off);
>> +void a6xx_gpu_sw_reset(struct msm_gpu *gpu, bool assert);
>>  
>>  #endif /* __A6XX_GPU_H__ */
>>
>> -- 
>> 2.40.1
>>