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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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

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


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


--
With best wishes
Dmitry




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux