linux-kernel - Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACu1E7HDmQXDNtEQCXpHXsOKPCOgrWgo+_kcgizo9Mp1ntjDbA@mail.gmail.com>
Date: Thu, 17 Apr 2025 11:32:26 -0400
From: Connor Abbott <cwabbott0@...il.com>
To: Akhil P Oommen <quic_akhilpo@...cinc.com>
Cc: Konrad Dybcio <konradybcio@...nel.org>, Bjorn Andersson <andersson@...nel.org>, 
	Kees Cook <kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>, Rob Clark <robdclark@...il.com>, 
	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@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, linux-hardening@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, 
	Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

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.

Connor

> So it is better to
> make this opt in, for newer chipsets or those which somebody can verify.
> We can invert this logic to something like this:
>
> if (!gpu->ubwc_config.highest_bank_bit)
>     gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb();
>
> >  }
> >
> >  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
> > @@ -2467,6 +2476,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
> >       bool is_a7xx;
> >       int ret;
> >
> > +     /* We need data from SMEM to retrieve HBB in calc_ubwc_config() */
> > +     if (!qcom_smem_is_available())
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +
>
> We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present
> in all Qcom SoC devicetrees? I wonder if there is a scenario where there
> might be an infinite EPROBE_DEFER here.
>
> -Akhil.
>
> >       a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL);
> >       if (!a6xx_gpu)
> >               return ERR_PTR(-ENOMEM);
> >
>