Message-ID: <CAF6AEGut3VQpj=v1euA+re55cLYot+BpjWVBs7orHURQ=Xdhdg@mail.gmail.com>
Date: Wed, 23 Apr 2025 07:56:11 -0700
From: Rob Clark <robdclark@...il.com>
To: Akhil P Oommen <quic_akhilpo@...cinc.com>
Cc: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, Connor Abbott <cwabbott0@...il.com>, 
	Konrad Dybcio <konradybcio@...nel.org>, Bjorn Andersson <andersson@...nel.org>, 
	Kees Cook <kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>, Sean Paul <sean@...rly.run>, 
	Abhinav Kumar <quic_abhinavk@...cinc.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Dmitry Baryshkov <lumag@...nel.org>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, linux-kernel <linux-kernel@...r.kernel.org>, 
	linux-arm-msm <linux-arm-msm@...r.kernel.org>, linux-hardening@...r.kernel.org, 
	dri-devel <dri-devel@...ts.freedesktop.org>, freedreno@...ts.freedesktop.org
Subject: Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

On Tue, Apr 22, 2025 at 11:55 PM Akhil P Oommen
<quic_akhilpo@...cinc.com> wrote:
>
> On 4/23/2025 5:27 AM, Konrad Dybcio wrote:
> > On 4/21/25 10:13 PM, Rob Clark wrote:
> >> On Fri, Apr 18, 2025 at 9:00 AM Akhil P Oommen <quic_akhilpo@...cinc.com> wrote:
> >>>
> >>> On 4/18/2025 6:40 AM, Connor Abbott wrote:
> >>>> On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen <quic_akhilpo@...cinc.com> wrote:
> >>>>>
> >>>>> On 4/17/2025 9:02 PM, Connor Abbott wrote:
> >>>>>> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@...cinc.com> wrote:
> >>>>>>>
> >>>>>>> On 4/10/2025 11:13 PM, Konrad Dybcio wrote:
> >>>>>>>> From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> >>>>>>>>
> >>>>>>>> The Highest Bank address Bit value can change based on memory type used.
> >>>>>>>>
> >>>>>>>> Attempt to retrieve it dynamically, and fall back to a reasonable
> >>>>>>>> default (the one used prior to this change) on error.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 15 ++++++++++++++-
> >>>>>>>>  1 file changed, 14 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>>>>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 100644
> >>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>>>>>> @@ -13,6 +13,7 @@
> >>>>>>>>  #include <linux/firmware/qcom/qcom_scm.h>
> >>>>>>>>  #include <linux/pm_domain.h>
> >>>>>>>>  #include <linux/soc/qcom/llcc-qcom.h>
> >>>>>>>> +#include <linux/soc/qcom/smem.h>
> >>>>>>>>
> >>>>>>>>  #define GPU_PAS_ID 13
> >>>>>>>>
> >>>>>>>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
> >>>>>>>>
> >>>>>>>>  static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
> >>>>>>>>  {
> >>>>>>>> +     int hbb;
> >>>>>>>> +
> >>>>>>>>       gpu->ubwc_config.rgb565_predicator = 0;
> >>>>>>>>       gpu->ubwc_config.uavflagprd_inv = 0;
> >>>>>>>>       gpu->ubwc_config.min_acc_len = 0;
> >>>>>>>> @@ -635,7 +638,6 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
> >>>>>>>>           adreno_is_a690(gpu) ||
> >>>>>>>>           adreno_is_a730(gpu) ||
> >>>>>>>>           adreno_is_a740_family(gpu)) {
> >>>>>>>> -             /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
> >>>>>>>>               gpu->ubwc_config.highest_bank_bit = 16;
> >>>>>>>>               gpu->ubwc_config.amsbc = 1;
> >>>>>>>>               gpu->ubwc_config.rgb565_predicator = 1;
> >>>>>>>> @@ -664,6 +666,13 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
> >>>>>>>>               gpu->ubwc_config.highest_bank_bit = 14;
> >>>>>>>>               gpu->ubwc_config.min_acc_len = 1;
> >>>>>>>>       }
> >>>>>>>> +
> >>>>>>>> +     /* Attempt to retrieve the data from SMEM, keep the above defaults in case of error */
> >>>>>>>> +     hbb = qcom_smem_dram_get_hbb();
> >>>>>>>> +     if (hbb < 0)
> >>>>>>>> +             return;
> >>>>>>>> +
> >>>>>>>> +     gpu->ubwc_config.highest_bank_bit = hbb;
> >>>>>>>
> >>>>>>> I am worried about blindly relying on SMEM data directly for HBB for
> >>>>>>> legacy chipsets. There is no guarantee it is accurate on every chipset
> >>>>>>> and every version of firmware. Also, until recently, this value was
> >>>>>>> hardcoded in Mesa which matched the value in KMD.
> >>>>>>
> >>>>>> To be clear about this, from the moment we introduced host image
> >>>>>> copies in Mesa we added support for querying the HBB from the kernel,
> >>>>>> explicitly so that we could do what this series does without Mesa ever
> >>>>>> breaking. Mesa will never assume the HBB unless the kernel is too old
> >>>>>> to support querying it. So don't let Mesa be the thing that stops us
> >>>>>> here.
> >>>>>
> >>>>> Thanks for clarifying about Mesa. I still don't trust a data source that
> >>>>> is unused in production.
> >>>>
> >>>> Fair enough, I'm not going to argue with that part. Just wanted to
> >>>> clear up any confusion about Mesa.
> >>>>
> >>>> Although, IIRC kgsl did set different values for a650 depending on
> >>>> memory type... do you know what source that used?
> >>>
> >>> KGSL relies on an undocumented devicetree node populated by bootloader
> >>> to detect ddrtype and calculates the HBB value based on that.
> >>
> >> Would it be reasonable to use the smem value, but if we find the
> >> undocumented dt property, WARN_ON() if it's value disagrees with smem?
> >>
> >> That would at least give some confidence, or justified un-confidence
> >> about the smem values
> >
> > The aforementioned value is populated based on the data that this
> > driver reads out, and only on the same range of platforms that this
> > driver happens to cater to
>
> Like I suggested privately, can we centralize all ubwc configuration so
> that it is consistent across all drivers. With that, we will need to
> maintain a table of ubwc config for each chipset and HBB can be
> calculated based on the DDR configuration identified from SMEM. Once we
> migrate the downstream drivers to the new API, we can hopefully move to
> the HBB value from SMEM. This will ensure that the SMEM data for HBB is
> accurate in all future chipsets.
>

yes please

BR,
-R

Powered by blists - more mailing lists