Re: [Freedreno] [PATCH] drm/msm/dpu: add DSC range checking during resource reservation

From: Abhinav Kumar
Date: Tue Apr 11 2023 - 21:50:45 EST




On 4/11/2023 6:06 PM, Dmitry Baryshkov wrote:
On 12/04/2023 01:32, Abhinav Kumar wrote:
Hi Marijn

On 4/11/2023 3:24 PM, Marijn Suijten wrote:
Again, don't forget to include previous reviewers in cc, please :)

On 2023-04-11 14:09:40, Kuogee Hsieh wrote:
Perform DSC range checking to make sure correct DSC is requested before
reserve resource for it.

nit: reserving


This isn't performing any range checking for resource reservations /
requests: this is only validating the constants written in our catalog
and seems rather useless.  It isn't fixing any real bug either, so the
Fixes: tag below seems extraneous.

Given prior comments from Abhinav that "the kernel should be trusted",
we should remove this validation for all the other blocks instead.


The purpose of this check is that today all our blocks in RM use the DSC_* enum as the size.

struct dpu_hw_blk *dsc_blks[DSC_MAX - DSC_0];

If the device tree ends up with more DSC blocks than the DSC_* enum, how can we avoid this issue today? Not because its a bug in device tree but how many static number of DSCs are hard-coded in RM.

We don't have these blocks in device tree. And dpu_hw_catalog shouldn't use indices outside of enum dpu_dsc.


ah, my bad, i should have said catalog here. Okay so the expectation is that dpu_hw_catalog.c will program the indices to match the RM limits.

I still stand by the fact that the hardware capabilities coming from catalog should be trusted but this is just the SW index.

Marijn proposed to pass struct dpu_foo_cfg directly to dpu_hw_foo_init(). This will allow us to drop these checks completely.


Ah okay, sure, would like to see that then uniformly get rid of these checks.

For the time being, I think it might be better to add these checks for DSC for the sake of uniformity.


Yes, i think so too.


And like you said, this is not specific to DSC. Such checks are present for other blocks too.

Fixes: c985d7bb64ff ("drm/msm/disp/dpu1: Add DSC support in RM")
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index f4dda88..95e58f1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -1,6 +1,7 @@
  // SPDX-License-Identifier: GPL-2.0-only
  /*
   * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
   */
  #define pr_fmt(fmt)    "[drm:%s] " fmt, __func__
@@ -250,6 +251,11 @@ int dpu_rm_init(struct dpu_rm *rm,
          struct dpu_hw_dsc *hw;
          const struct dpu_dsc_cfg *dsc = &cat->dsc[i];
+        if (dsc->id < DSC_0 || dsc->id >= DSC_MAX) {
+            DPU_ERROR("skip dsc %d with invalid id\n", dsc->id);
+            continue;
+        }
+
          hw = dpu_hw_dsc_init(dsc->id, mmio, cat);
          if (IS_ERR_OR_NULL(hw)) {
              rc = PTR_ERR(hw);
@@ -557,8 +563,10 @@ static int _dpu_rm_make_reservation(
      }
      ret  = _dpu_rm_reserve_dsc(rm, global_state, enc, &reqs->topology);
-    if (ret)
+    if (ret) {
+        DPU_ERROR("unable to find appropriate DSC\n");

This, while a nice addition, should go in a different patch.

I'd agree here, a separate patch.


Thanks!

- Marijn

          return ret;
+    }
      return ret;
  }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project