Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Thu, 27 Jul 2023 at 01:04, Rob Clark <robdclark@xxxxxxxxx> wrote:
> >
> > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On 26/07/2023 23:11, Rob Clark wrote:
> > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > >>
> > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <robdclark@xxxxxxxxx> wrote:
> > > >>>
> > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <quic_akhilpo@xxxxxxxxxxx> wrote:
> > > >>>>
> > > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > >>>>>
> > > >>>>> On 07/07/2023 00:10, Rob Clark wrote:
> > > >>>>>> From: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > >>>>>>
> > > >>>>>> There are cases where there are differences due to SoC integration.
> > > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to
> > > >>>>>> speedbin mappings.
> > > >>>>>
> > > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd
> > > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > >>>>> using of_device_id::data and then of_device_get_match_data().
> > > >>>>>
> > > >>>> Just thinking, then how about a unique compatible string which we match
> > > >>>> to identify gpu->info and drop chip-id check completely here?
> > > >>>
> > > >>> Ok, I think we could do this, so something like:
> > > >>>
> > > >>>    compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> > > >>>
> > > >>> ?
> > > >>>
> > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > > >>> or sm6375, so I suppose we could get away with this change
> > > >>
> > > >> I think we can even skip the 619.0 part in the SoC compat string.
> > > >> So it will be:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > > >>
> > > >> In future we can drop the chipid part completely and handle that as a
> > > >> part of SoC data:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > > >>
> > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > > >>
> > > >
> > > > I don't think we can do that, there are cases where the same SoC had
> > > > multiple revisions of adreno.
> > >
> > > Is that the case for the production versions of the SoC? In other
> > > subsystems what we usually do is that we add support only for the latest
> > > SoC revision (which would probably mean the latest GPU patch revision).
> > > Previous GPU revisions can be added in the following way (pure example):
> > >
> > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > > sample
> > > qcom,sm4350-v1-adreno -> 6,1,9,0
> > >
> >
> > My recollection was that nexus4 shipped with an early version of 8064
> > which needed userspace workarounds that later 8064 did not.  Not sure
> > if that is the only such example, but it is one that userspace needed
> > to be aware of.
>
> Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
>
> And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

I no longer have a n4 that boots.. but if I did both it and the later
ones should work properly if they expose the appropriate chip id

I do still prefer parsing the chip-id out of the compatible.  It
avoids needing separate table entries just to have a different
chip-id.  Maybe the scheme that is used elsewhere makes sense when it
is only the kernel that needs to be aware of the device-id.  And maybe
we could just done matching based on compat-id in userspace as well,
but (a) msm and freedreno pre-date dt, and (b) that ship has already
sailed.

BR,
-R

> >
> > Anyways, future things, it sounds like we'll be able to read the id
> > from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> > so I don't want to change any of the existing compat strings.
>
> I think so too. Current compat strings should stay.
>
> >
> > BR,
> > -R
> >
> > > >  We could possibly do that with future
> > > > things where we can read the chip-id from fw.
> > > >
> > > > What we _could_ do at the expense of making the compatible parsing a
> > > > tiny bit more complex is something like:
> > > >
> > > >     compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > > >
> > > > BR,
> > > > -R
> > > >
> > > >>>
> > > >>> BR,
> > > >>> -R
> > > >>>
> > > >>>> -Akhil
> > > >>>>
> > > >>>>>>
> > > >>>>>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > >>>>>> ---
> > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
> > > >>>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
> > > >>>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> index 3c531da417b9..e62bc895a31f 100644
> > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > >>>>>>              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>>              .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > >>>>>>              .init = a6xx_gpu_init,
> > > >>>>>> +   }, {
> > > >>>>>> +           .machine = "qcom,sm4350",
> > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>> +           .revn = 619,
> > > >>>>>> +           .fw = {
> > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > >>>>>> +           },
> > > >>>>>> +           .gmem = SZ_512K,
> > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>> +           .init = a6xx_gpu_init,
> > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > >>>>>> +           .hwcg = a615_hwcg,
> > > >>>>>> +   }, {
> > > >>>>>> +           .machine = "qcom,sm6375",
> > > >>>>>> +           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>> +           .revn = 619,
> > > >>>>>> +           .fw = {
> > > >>>>>> +                   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > >>>>>> +                   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > >>>>>> +           },
> > > >>>>>> +           .gmem = SZ_512K,
> > > >>>>>> +           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > >>>>>> +           .init = a6xx_gpu_init,
> > > >>>>>> +           .zapfw = "a615_zap.mdt",
> > > >>>>>> +           .hwcg = a615_hwcg,
> > > >>>>>>      }, {
> > > >>>>>>              .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > >>>>>>              .revn = 619,
> > > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
> > > >>>>>>      /* identify gpu: */
> > > >>>>>>      for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > >>>>>>              const struct adreno_info *info = &gpulist[i];
> > > >>>>>> +           if (info->machine && !of_machine_is_compatible(info->machine))
> > > >>>>>> +                   continue;
> > > >>>>>>              if (adreno_cmp_rev(info->rev, rev))
> > > >>>>>>                      return info;
> > > >>>>>>      }
> > > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >>>>>>              config.rev.minor, config.rev.patchid);
> > > >>>>>>      priv->is_a2xx = config.rev.core == 2;
> > > >>>>>> +   priv->has_cached_coherent =
> > > >>>>>> +           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > >>>>>>      gpu = info->init(drm);
> > > >>>>>>      if (IS_ERR(gpu)) {
> > > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >>>>>>      if (ret)
> > > >>>>>>              return ret;
> > > >>>>>> -   priv->has_cached_coherent =
> > > >>>>>> -           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > >>>>>> -           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > >>>>>> -
> > > >>>>>>      return 0;
> > > >>>>>>    }
> > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> index e08d41337169..d5335b99c64c 100644
> > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> > > >>>>>>    extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > > >>>>>>    struct adreno_info {
> > > >>>>>> +   const char *machine;
> > > >>>>>>      struct adreno_rev rev;
> > > >>>>>>      uint32_t revn;
> > > >>>>>>      const char *fw[ADRENO_FW_MAX];
> > > >>>>>
> > > >>>>> --
> > > >>>>> With best wishes
> > > >>>>> Dmitry
> > > >>>>>
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> With best wishes
> > > >> Dmitry
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >
>
>
>
> --
> With best wishes
> Dmitry




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux