Re: [PATCH v5 02/16] drm/msm/dpu: fix error condition in dpu_encoder_virt_atomic_mode_set

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

 





On 7/12/2024 4:11 PM, Dmitry Baryshkov wrote:
On Fri, 12 Jul 2024 at 22:41, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote:
The commit b954fa6baaca ("drm/msm/dpu: Refactor rm iterator") removed
zero-init of the hw_ctl array, but didn't change the error condition,
that checked for hw_ctl[i] being NULL. Use indices check instead.

Fixes: b954fa6baaca ("drm/msm/dpu: Refactor rm iterator")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5d205e09cf45..7613005fbfea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1186,7 +1186,7 @@ static void :tag(struct drm_encoder *drm_enc,
                       return;
               }

-             if (!hw_ctl[i]) {
+             if (i >= num_ctl) {

This is not very clear to me.

How will we hit this condition? I dont see i going beyond 1 in this loop
and neither should num_ctl

Why? the driver doesn't support flushing through a single CTL, so
num_ctl = num_intf.


num_ctl will be = num_intf, but what I was trying to understand here is that , previously this condition was making sure that we have a ctl assigned for each physical encoder which is actually a requirement for the display pipeline. If we assigned a hw_ctl for one phys encoder and not the other, its an error.

But on closer look, I think even your check will catch that.



Will it be just easier to bring back the NULL assignment at the top?

struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };

I also see the same issue for other blocks such as hw_dsc, hw_lm

Other blocks loop properly up to the num_resource. I'd prefer to drop
the NULL init from the DSPP init and use num_dspp instead.


Overall, I think the purpose of NULL init was to make sure that before we call to_dpu_hw_***() macros, we have a valid hw_*.

We could use either num_* or the hw_* as both are returned by RM.

One side-note here is with a proper NULL hw_ctl is that the consumers of hw_ctl should also be able to check for NULL correctly.

So for example dpu_encoder_phys layers use if (!phys->hw_ctl) checks but today we do not set phys->hw_ctl to NULL correctly.

Do you think that instead of the return statements, we should do something like

dpu_enc->hw_ctl = i < num_ctl ? 	
	to_dpu_hw_ctl(hw_ctl[i]) : NULL;


But this will need the NULL init back.


                       DPU_ERROR_ENC(dpu_enc,
                               "no ctl block assigned at idx: %d\n", i);
                       return;







[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