Message-ID: <CAF6AEGvyeRLHFBYmxkevgT+hosXGiH_w8Z+UjQmL+LdbNfVZ+w@mail.gmail.com>
Date: Mon, 21 Apr 2025 13:13:53 -0700
From: Rob Clark <robdclark@...il.com>
To: Akhil P Oommen <quic_akhilpo@...cinc.com>
Cc: 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, 
	Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

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

BR,
-R

>
> -Akhil.
>
> >
> >>
> >> I have a related question about HBB. Blob driver doesn't support
> >> host_image_copy, but it still use HBB configuration. I was under the
> >> impression this was required for UMD for compression related
> >> configurations. Is that not true for turnip/freedreno?
> >>
> >> -Akhil.
> >
> > AFAIK the HBB (as well as other UBWC config parameters) doesn't have
> > any impact on layout configuration, so the UMD can ignore it except
> > when it's doing CPU texture uploads/downloads. We certainly do in
> > freedreno/turnip. You'd have to ask that team what they use HBB for,
> > but my best guess is that the GLES driver uses it for CPU texture
> > uploads sometimes. That is, the GLES driver might be using
> > functionality similar to host_image_copy "under the hood". It's
> > something we'd probably want for freedreno too.
> >
> > Connor
> >
> >>
> >>>
> >>> 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);
> >>>>>
> >>>>
> >>
>

Powered by blists - more mailing lists