On Fri, Jan 24, 2025 at 04:08:17PM -0800, Abhinav Kumar wrote:
>
>
> On 1/23/2025 9:19 PM, Dmitry Baryshkov wrote:
> > On Thu, Jan 23, 2025 at 01:41:14PM -0800, Abhinav Kumar wrote:
> > >
> > >
> > > On 1/23/2025 1:32 PM, Abhinav Kumar wrote:
> > > >
> > > >
> > > > On 12/13/2024 2:14 PM, Dmitry Baryshkov wrote:
> > > > > Continue migration to the MDSS-revision based checks and replace
> > > > > DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
> > > > >
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h | 2 --
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h | 1 -
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h | 2 --
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h | 6 ++----
> > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10
> > > > > ++--------
> > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
> > > > > 7 files changed, 5 insertions(+), 20 deletions(-)
> > > > >
> > > >
> > > > <snip>
> > > >
> > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > index 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > @@ -319,7 +319,7 @@ struct dpu_hw_pingpong
> > > > > *dpu_hw_pingpong_init(struct drm_device *dev,
> > > > > c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
> > > > > }
> > > > > - if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
> > > > > + if (mdss_rev->core_major_ver < 7) {
> > > > > c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
> > > > > c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
> > > > > c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
> > > > >
> > > >
> > > > So far in this series, we replaced the feature bits with >= checks of
> > > > core_revisions. That kind of works as usually feature bits get added
> > > > after a specific version.
> > > >
> > > > With this patch and later, whenever we use < checks it gets a bit tricky
> > > > as we might also need an upper bound. Feature bits gave individual
> > > > control of chipsets but generalizing that all chipsets < 7 have PP DSC
> > > > is also not correct. I have to really cross-check but there could be
> > > > some old chipsets which do not have DSC at all.
> > >
> > > This raises another question as well.
> > >
> > > what if some register was introduced >= X version but was then dropped in a
> > > newer chipset.
> > >
> > > Is it not difficult for the user to go back to the files of each of the
> > > sub-blocks and alter these checks rather than just fixing up the catalog.
> >
> > Well, the obvious example we are going to have is the CTL_LAYER_EXT4,
> > but if I understand correctly the whole block is going to be dropped, so
> > maybe it's not that relevant.
> >
> > Another example might be CWB, where we are going to have 5.x-7.x and
> > 8.x+ DPU ranges.
> >
> > Basically, yes, when adding support for a new platform we have to audit
> > HW blocks. But this applied even beforehand, where new platforms could
> > be drooping existing regs (8.x+ dropping a part of the TOP region).
> >
>
> Right, I am wondering what is the trade-off here.
>
> 1) Feature bits allow selective control for every chipset. IOW, the specific
> version is already checked for. I do admit that I have seen errors about
> using the correct feature mask sometimes but even with this change, evey
> developer will need to go and check every sub-block file which they might
> not even know about.
>
> 2) core_revision certainly can generalize but we might still need absolute
> versions for some of the bits anyway. So the checks may not always be >= or
> < but it could also end up with something like
>
> if (major_rev == xxx && minor_rev == yyy)
> ops = ops_a;
> else if (major_rev == aaa &&& minor_rev == bbb)
> ops = ops_b
>
> So the revision check will most likely end up being more complicated than
> simple range checks. With each catalog feature bit atleast we are guaranteed
> that its applied only to that revision.
Yes... However I'd prefer uniformity. In other words, if we started
using core revisions, let's use just it. Having core revision in some
places and feature bits in some other places sounds like an easy way to
confuse developers.
>
> I do see in the cover letter, about incorrect feature bits sometimes used
> but I am trying to assess the trade-offs even with this approach.
Sure :-)
--
With best wishes
Dmitry
