mbox series

[0/3] drm/msm/dpu: Initialize SSPP scaler version (from register read)

Message ID 20230215-sspp-scaler-version-v1-0-416b1500b85b@somainline.org
Headers
Series drm/msm/dpu: Initialize SSPP scaler version (from register read) |

Message

Marijn Suijten Feb. 15, 2023, 11:02 p.m. UTC 
  Random inspection of the SSPP code surfaced that the version field of
dpu_scaler_blk was never assigned in the catalog, resulting in wrong
codepaths to be taken within dpu_hw_setup_scaler3 based on a 0 version.
Rectify this by reading an accurate value from a register (that is not
equal to the values represented by DPU_SSPP_SCALER_QSEEDx enum
variants) and deleting dead code around QSEED versioning.

Future changes should likely get rid of the distinction between QSEED3
and up, as these are now purely determined from the register value.
Furthermore implementations could look at the scaler subblk .id field
rather than the SSPP feature bits, which currently hold redundant
information.

---
Marijn Suijten (3):
      drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw
      drm/msm/dpu: Drop unused get_scaler_ver callback from SSPP
      drm/msm/dpu: Drop unused qseed_type from catalog dpu_caps

 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 12 ------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  4 ----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c    | 12 ++++++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h    |  9 +++------
 4 files changed, 11 insertions(+), 26 deletions(-)
---
base-commit: 9d9019bcea1aac7eed64a1a4966282b6b7b141c8
change-id: 20230215-sspp-scaler-version-19f221585c5e

Best regards,

Comments

Dmitry Baryshkov Feb. 16, 2023, 3:02 a.m. UTC | #1 
On Thu, 16 Feb 2023 at 01:02, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Random inspection of the SSPP code surfaced that the version field of
> dpu_scaler_blk was never assigned in the catalog, resulting in wrong
> codepaths to be taken within dpu_hw_setup_scaler3 based on a 0 version.
> Rectify this by reading an accurate value from a register (that is not
> equal to the values represented by DPU_SSPP_SCALER_QSEEDx enum
> variants) and deleting dead code around QSEED versioning.
>
> Future changes should likely get rid of the distinction between QSEED3
> and up, as these are now purely determined from the register value.
> Furthermore implementations could look at the scaler subblk .id field
> rather than the SSPP feature bits, which currently hold redundant
> information.
>
> ---
> Marijn Suijten (3):
>       drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw
>       drm/msm/dpu: Drop unused get_scaler_ver callback from SSPP
>       drm/msm/dpu: Drop unused qseed_type from catalog dpu_caps

The cleanup looks good. However as you are on it, maybe you can also
add patch 4, dropping DPU_SSPP_SCALER_QSEED3LITE and
DPU_SSPP_SCALER_QSEED4 in favour of using QSEED3 for all these
scalers? As we are going to use scaler_version to distinguish between
them, it would be logical not to duplicate that bit of information
(not to mention all the possible troubles if scaler_version disagrees
with the sblk->scaler_blk.id).

>
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 12 ------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  4 ----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c    | 12 ++++++++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h    |  9 +++------
>  4 files changed, 11 insertions(+), 26 deletions(-)
> ---
> base-commit: 9d9019bcea1aac7eed64a1a4966282b6b7b141c8
> change-id: 20230215-sspp-scaler-version-19f221585c5e
>
> Best regards,
> --
> Marijn Suijten <marijn.suijten@somainline.org>
>
Marijn Suijten Feb. 16, 2023, 9:04 a.m. UTC | #2 
On 2023-02-16 05:02:32, Dmitry Baryshkov wrote:
> On Thu, 16 Feb 2023 at 01:02, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Random inspection of the SSPP code surfaced that the version field of
> > dpu_scaler_blk was never assigned in the catalog, resulting in wrong
> > codepaths to be taken within dpu_hw_setup_scaler3 based on a 0 version.
> > Rectify this by reading an accurate value from a register (that is not
> > equal to the values represented by DPU_SSPP_SCALER_QSEEDx enum
> > variants) and deleting dead code around QSEED versioning.
> >
> > Future changes should likely get rid of the distinction between QSEED3
> > and up, as these are now purely determined from the register value.
> > Furthermore implementations could look at the scaler subblk .id field
> > rather than the SSPP feature bits, which currently hold redundant
> > information.
> >
> > ---
> > Marijn Suijten (3):
> >       drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw
> >       drm/msm/dpu: Drop unused get_scaler_ver callback from SSPP
> >       drm/msm/dpu: Drop unused qseed_type from catalog dpu_caps
> 
> The cleanup looks good. However as you are on it, maybe you can also
> add patch 4, dropping DPU_SSPP_SCALER_QSEED3LITE and
> DPU_SSPP_SCALER_QSEED4 in favour of using QSEED3 for all these
> scalers?

I surely can!  Do you mind if I rename it to QSEED3_AND_UP for clarity?
How about the second question, dropping this redundant information from
the SSPP feature flags and only looking at the scaler_blk.id?

> As we are going to use scaler_version to distinguish between
> them, it would be logical not to duplicate that bit of information
> (not to mention all the possible troubles if scaler_version disagrees
> with the sblk->scaler_blk.id).

Note that we had a similar discussion for UBWC HW decoder version and it
was decided to go the opposite route [1].  That may have been for
technical reasons (unclocked register access), but it's an inconsistent
approach to say the least.

[1]: https://lore.kernel.org/linux-arm-msm/71f96910-e7b1-92f9-ae15-79bd1da40a0d@quicinc.com/

- Marijn