| Message ID | 20230628-topic-a635-v2-1-5494c881b8be@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] drm/msm/adreno: Assign revn to A635 | expand |
On Sat, 1 Jul 2023 at 02:12, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > Recently, a WARN_ON() was introduced to ensure that revn is filled before > adreno_is_aXYZ is called. This however doesn't work very well when revn is > 0 by design (such as for A635). Fill it in as a stopgap solution for > -fixes. > > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> As the v1: Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > Changes in v2: > - add fixes > - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org > --- > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index cb94cfd137a8..8ea7eae9fc52 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { > .address_space_size = SZ_16G, > }, { > .rev = ADRENO_REV(6, 3, 5, ANY_ID), > + .revn = 635, > .fw = { > [ADRENO_FW_SQE] = "a660_sqe.fw", > [ADRENO_FW_GMU] = "a660_gmu.bin", > > --- > base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 > change-id: 20230628-topic-a635-1b3c2c987417 > > Best regards, > -- > Konrad Dybcio <konrad.dybcio@linaro.org> >
On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > Recently, a WARN_ON() was introduced to ensure that revn is filled before > adreno_is_aXYZ is called. This however doesn't work very well when revn is > 0 by design (such as for A635). Fill it in as a stopgap solution for > -fixes. > > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Changes in v2: > - add fixes > - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org > --- > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index cb94cfd137a8..8ea7eae9fc52 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { > .address_space_size = SZ_16G, > }, { > .rev = ADRENO_REV(6, 3, 5, ANY_ID), > + .revn = 635, > .fw = { > [ADRENO_FW_SQE] = "a660_sqe.fw", > [ADRENO_FW_GMU] = "a660_gmu.bin", > hmm, I realized a problem with this, it would change what MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return.. The former should be "harmless", although it isn't a good idea for uabi changes to be a side effect of a fix. The latter is more problematic. I think I'm leaning more towards reverting commit cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") for -fixes. I'm still thinking about options for a longer term fix. BR, -R > --- > base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 > change-id: 20230628-topic-a635-1b3c2c987417 > > Best regards, > -- > Konrad Dybcio <konrad.dybcio@linaro.org> >
On Sat, Jul 1, 2023 at 8:49 AM Rob Clark <robdclark@gmail.com> wrote: > > On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > Recently, a WARN_ON() was introduced to ensure that revn is filled before > > adreno_is_aXYZ is called. This however doesn't work very well when revn is > > 0 by design (such as for A635). Fill it in as a stopgap solution for > > -fixes. > > > > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > --- > > Changes in v2: > > - add fixes > > - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org > > --- > > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > > index cb94cfd137a8..8ea7eae9fc52 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > > @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { > > .address_space_size = SZ_16G, > > }, { > > .rev = ADRENO_REV(6, 3, 5, ANY_ID), > > + .revn = 635, > > .fw = { > > [ADRENO_FW_SQE] = "a660_sqe.fw", > > [ADRENO_FW_GMU] = "a660_gmu.bin", > > > > hmm, I realized a problem with this, it would change what > MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return.. The > former should be "harmless", although it isn't a good idea for uabi > changes to be a side effect of a fix. The latter is more problematic. > > I think I'm leaning more towards reverting commit cc943f43ece7 > ("drm/msm/adreno: warn if chip revn is verified before being set") for > -fixes. I'm still thinking about options for a longer term fix. This is what I came up with as far as better way to solve this: https://patchwork.freedesktop.org/series/120108/ BR, -R > BR, > -R > > > > --- > > base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 > > change-id: 20230628-topic-a635-1b3c2c987417 > > > > Best regards, > > -- > > Konrad Dybcio <konrad.dybcio@linaro.org> > >
On Sat, 1 Jul 2023 at 18:50, Rob Clark <robdclark@gmail.com> wrote: > > On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > Recently, a WARN_ON() was introduced to ensure that revn is filled before > > adreno_is_aXYZ is called. This however doesn't work very well when revn is > > 0 by design (such as for A635). Fill it in as a stopgap solution for > > -fixes. > > > > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > --- > > Changes in v2: > > - add fixes > > - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org > > --- > > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > > index cb94cfd137a8..8ea7eae9fc52 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > > @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { > > .address_space_size = SZ_16G, > > }, { > > .rev = ADRENO_REV(6, 3, 5, ANY_ID), > > + .revn = 635, > > .fw = { > > [ADRENO_FW_SQE] = "a660_sqe.fw", > > [ADRENO_FW_GMU] = "a660_gmu.bin", > > > > hmm, I realized a problem with this, it would change what > MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return.. The > former should be "harmless", although it isn't a good idea for uabi > changes to be a side effect of a fix. The latter is more problematic. I'd say MSM_PARAM_GPU_ID is broken for 635 anyway (won't it return 0 in this case)? So the new value should be correct. But more importantly, why are we exporting speedbin in MSM_PARAM_CHIP_ID only if there is no revn? And why are we exporting the speedbin at all as a part of CHIP_ID? > > I think I'm leaning more towards reverting commit cc943f43ece7 > ("drm/msm/adreno: warn if chip revn is verified before being set") for > -fixes. I'm still thinking about options for a longer term fix. > > BR, > -R > > > > --- > > base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 > > change-id: 20230628-topic-a635-1b3c2c987417 > > > > Best regards, > > -- > > Konrad Dybcio <konrad.dybcio@linaro.org> > >
On Sat, Jul 1, 2023 at 5:24 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Sat, 1 Jul 2023 at 18:50, Rob Clark <robdclark@gmail.com> wrote: > > > > On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > > > Recently, a WARN_ON() was introduced to ensure that revn is filled before > > > adreno_is_aXYZ is called. This however doesn't work very well when revn is > > > 0 by design (such as for A635). Fill it in as a stopgap solution for > > > -fixes. > > > > > > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > --- > > > Changes in v2: > > > - add fixes > > > - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org > > > --- > > > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > > > index cb94cfd137a8..8ea7eae9fc52 100644 > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > > > @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { > > > .address_space_size = SZ_16G, > > > }, { > > > .rev = ADRENO_REV(6, 3, 5, ANY_ID), > > > + .revn = 635, > > > .fw = { > > > [ADRENO_FW_SQE] = "a660_sqe.fw", > > > [ADRENO_FW_GMU] = "a660_gmu.bin", > > > > > > > hmm, I realized a problem with this, it would change what > > MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return.. The > > former should be "harmless", although it isn't a good idea for uabi > > changes to be a side effect of a fix. The latter is more problematic. > > I'd say MSM_PARAM_GPU_ID is broken for 635 anyway (won't it return 0 > in this case)? > So the new value should be correct. no, it is very much intentional that GPU_ID returns 0 for newer GPUs, userspace should be matching on CHIP_ID. (Also, we should be moving away from trying to infer generation/etc from CHIP_ID.. userspace is farther ahead of the kernel on this.) > But more importantly, why are we exporting speedbin in > MSM_PARAM_CHIP_ID only if there is no revn? And why are we exporting > the speedbin at all as a part of CHIP_ID? Basically just being paranoid about not changing uabi. It probably would be ok to export the speedbin for all, but I'd have to double check mesa version history. BR, -R > > > > I think I'm leaning more towards reverting commit cc943f43ece7 > > ("drm/msm/adreno: warn if chip revn is verified before being set") for > > -fixes. I'm still thinking about options for a longer term fix. > > > > BR, > > -R > > > > > > > --- > > > base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 > > > change-id: 20230628-topic-a635-1b3c2c987417 > > > > > > Best regards, > > > -- > > > Konrad Dybcio <konrad.dybcio@linaro.org> > > > > > > > -- > With best wishes > Dmitry
On 02/07/2023 17:31, Rob Clark wrote: > On Sat, Jul 1, 2023 at 5:24 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> On Sat, 1 Jul 2023 at 18:50, Rob Clark <robdclark@gmail.com> wrote: >>> >>> On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >>>> >>>> Recently, a WARN_ON() was introduced to ensure that revn is filled before >>>> adreno_is_aXYZ is called. This however doesn't work very well when revn is >>>> 0 by design (such as for A635). Fill it in as a stopgap solution for >>>> -fixes. >>>> >>>> Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >>>> Changes in v2: >>>> - add fixes >>>> - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org >>>> --- >>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> index cb94cfd137a8..8ea7eae9fc52 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c >>>> @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { >>>> .address_space_size = SZ_16G, >>>> }, { >>>> .rev = ADRENO_REV(6, 3, 5, ANY_ID), >>>> + .revn = 635, >>>> .fw = { >>>> [ADRENO_FW_SQE] = "a660_sqe.fw", >>>> [ADRENO_FW_GMU] = "a660_gmu.bin", >>>> >>> >>> hmm, I realized a problem with this, it would change what >>> MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return.. The >>> former should be "harmless", although it isn't a good idea for uabi >>> changes to be a side effect of a fix. The latter is more problematic. >> >> I'd say MSM_PARAM_GPU_ID is broken for 635 anyway (won't it return 0 >> in this case)? >> So the new value should be correct. > > no, it is very much intentional that GPU_ID returns 0 for newer GPUs, > userspace should be matching on CHIP_ID. (Also, we should be moving > away from trying to infer generation/etc from CHIP_ID.. userspace is > farther ahead of the kernel on this.) Thanks for the explanation. So in theory we can change this to always return 0? Or must we keep it to keep UABI / compatibility? I'm trying to understand if we can drop revn at all. > >> But more importantly, why are we exporting speedbin in >> MSM_PARAM_CHIP_ID only if there is no revn? And why are we exporting >> the speedbin at all as a part of CHIP_ID? > > Basically just being paranoid about not changing uabi. It probably > would be ok to export the speedbin for all, but I'd have to double > check mesa version history. Thanks! > > BR, > -R > >>> >>> I think I'm leaning more towards reverting commit cc943f43ece7 >>> ("drm/msm/adreno: warn if chip revn is verified before being set") for >>> -fixes. I'm still thinking about options for a longer term fix. >>> >>> BR, >>> -R >>> >>> >>>> --- >>>> base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 >>>> change-id: 20230628-topic-a635-1b3c2c987417 >>>> >>>> Best regards, >>>> -- >>>> Konrad Dybcio <konrad.dybcio@linaro.org> >>>> >> >> >> >> -- >> With best wishes >> Dmitry
On Sun, Jul 2, 2023 at 7:34 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 02/07/2023 17:31, Rob Clark wrote: > > On Sat, Jul 1, 2023 at 5:24 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > >> > >> On Sat, 1 Jul 2023 at 18:50, Rob Clark <robdclark@gmail.com> wrote: > >>> > >>> On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >>>> > >>>> Recently, a WARN_ON() was introduced to ensure that revn is filled before > >>>> adreno_is_aXYZ is called. This however doesn't work very well when revn is > >>>> 0 by design (such as for A635). Fill it in as a stopgap solution for > >>>> -fixes. > >>>> > >>>> Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") > >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>>> --- > >>>> Changes in v2: > >>>> - add fixes > >>>> - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org > >>>> --- > >>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > >>>> index cb94cfd137a8..8ea7eae9fc52 100644 > >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > >>>> @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { > >>>> .address_space_size = SZ_16G, > >>>> }, { > >>>> .rev = ADRENO_REV(6, 3, 5, ANY_ID), > >>>> + .revn = 635, > >>>> .fw = { > >>>> [ADRENO_FW_SQE] = "a660_sqe.fw", > >>>> [ADRENO_FW_GMU] = "a660_gmu.bin", > >>>> > >>> > >>> hmm, I realized a problem with this, it would change what > >>> MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return.. The > >>> former should be "harmless", although it isn't a good idea for uabi > >>> changes to be a side effect of a fix. The latter is more problematic. > >> > >> I'd say MSM_PARAM_GPU_ID is broken for 635 anyway (won't it return 0 > >> in this case)? > >> So the new value should be correct. > > > > no, it is very much intentional that GPU_ID returns 0 for newer GPUs, > > userspace should be matching on CHIP_ID. (Also, we should be moving > > away from trying to infer generation/etc from CHIP_ID.. userspace is > > farther ahead of the kernel on this.) > > Thanks for the explanation. So in theory we can change this to always > return 0? Or must we keep it to keep UABI / compatibility? > > I'm trying to understand if we can drop revn at all. I'm not sure how likely it is that someone would try a very old mesa with a new kernel, but that combo would break if we dropped revn completely and returned 0 for GPU_ID. But I would like to move away from using revn internally. (And probably move away from the patch-id wildcard matching, and instead just explicitly list all known patch-id values so we can start treating CHIP_ID as just an opaque identifier.) BR, -R > > > > >> But more importantly, why are we exporting speedbin in > >> MSM_PARAM_CHIP_ID only if there is no revn? And why are we exporting > >> the speedbin at all as a part of CHIP_ID? > > > > Basically just being paranoid about not changing uabi. It probably > > would be ok to export the speedbin for all, but I'd have to double > > check mesa version history. > > Thanks! > > > > > BR, > > -R > > > >>> > >>> I think I'm leaning more towards reverting commit cc943f43ece7 > >>> ("drm/msm/adreno: warn if chip revn is verified before being set") for > >>> -fixes. I'm still thinking about options for a longer term fix. > >>> > >>> BR, > >>> -R > >>> > >>> > >>>> --- > >>>> base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 > >>>> change-id: 20230628-topic-a635-1b3c2c987417 > >>>> > >>>> Best regards, > >>>> -- > >>>> Konrad Dybcio <konrad.dybcio@linaro.org> > >>>> > >> > >> > >> > >> -- > >> With best wishes > >> Dmitry > > -- > With best wishes > Dmitry >
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index cb94cfd137a8..8ea7eae9fc52 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { .address_space_size = SZ_16G, }, { .rev = ADRENO_REV(6, 3, 5, ANY_ID), + .revn = 635, .fw = { [ADRENO_FW_SQE] = "a660_sqe.fw", [ADRENO_FW_GMU] = "a660_gmu.bin",
Recently, a WARN_ON() was introduced to ensure that revn is filled before adreno_is_aXYZ is called. This however doesn't work very well when revn is 0 by design (such as for A635). Fill it in as a stopgap solution for -fixes. Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Changes in v2: - add fixes - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@linaro.org --- drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + 1 file changed, 1 insertion(+) --- base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 change-id: 20230628-topic-a635-1b3c2c987417 Best regards,