Re: [PATCH v1 2/3] drm/msm/adreno: Add support for X185 GPU

Akhil P Oommen
2024-06-26T08:24:41Z

On Mon, Jun 24, 2024 at 03:53:48PM +0200, Konrad Dybcio wrote:
>
>
> On 6/23/24 13:06, Akhil P Oommen wrote:
> > Add support in drm/msm driver for the Adreno X185 gpu found in
> > Snapdragon X1 Elite chipset.
> >
> > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> > ---
> >
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 19 +++++++++++++++----
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 ++----
> > drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++++++++++++++
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++++
> > 4 files changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index 0e3dfd4c2bc8..168a4bddfaf2 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -830,8 +830,10 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned int state)
> > */
> > gmu_write(gmu, REG_A6XX_GMU_CM3_CFG, 0x4052);
> > + if (adreno_is_x185(adreno_gpu)) {
> > + chipid = 0x7050001;
>
> What's wrong with using the logic below?

patchid is BITS(7, 0), not (15, 8) in the case of x185. Due to the
changes in the chipid scheme within the a7x family, this is a bit
confusing. I will try to improve here in another series.

>
> > /* NOTE: A730 may also fall in this if-condition with a future GMU fw update. */
> > - if (adreno_is_a7xx(adreno_gpu) && !adreno_is_a730(adreno_gpu)) {
> > + } else if (adreno_is_a7xx(adreno_gpu) && !adreno_is_a730(adreno_gpu)) {
> > /* A7xx GPUs have obfuscated chip IDs. Use constant maj = 7 */
> > chipid = FIELD_PREP(GENMASK(31, 24), 0x7);
> > @@ -1329,9 +1331,18 @@ static int a6xx_gmu_rpmh_arc_votes_init(struct device *dev, u32 *votes,
> > if (!pri_count)
> > return -EINVAL;
> > - sec = cmd_db_read_aux_data("mx.lvl", &sec_count);
> > - if (IS_ERR(sec))
> > - return PTR_ERR(sec);
> > + /*
> > + * Some targets have a separate gfx mxc rail. So try to read that first and then fall back
> > + * to regular mx rail if it is missing
> > + */
> > + sec = cmd_db_read_aux_data("gmxc.lvl", &sec_count);
> > + if (PTR_ERR_OR_ZERO(sec) == -EPROBE_DEFER) {
> > + return -EPROBE_DEFER;
> > + } else if (IS_ERR(sec)) {
> > + sec = cmd_db_read_aux_data("mx.lvl", &sec_count);
> > + if (IS_ERR(sec))
> > + return PTR_ERR(sec);
> > + }
>
> I assume GMXC would always be used if present, although please use the
> approach Dmitry suggested

Correct.

-Akhil
>
>
> The rest looks good!
>
> Konrad