On 7/13/2024 2:49 AM, Dmitry Baryshkov wrote:
On Sat, 13 Jul 2024 at 03:25, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
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.
The problem of the NULL checks is that it's too tempting to perform a
NULL check after to_dpu_hw_ctl conversion. However it's not safe to
pass NULL pointer to such functions: there is no guarantee that
conversion will return NULL if it gets passed the NULL pointer.
Yes, thats why these checks are there before calling to_dpu_hw_ctl() to
make sure we dont pass NULL there.
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;
Yeah, why not.
Generally, I think we should stop storing the state-related data in
the non-state structures. Hopefully I'll have time for that at some
point later on.
But this will need the NULL init back.
It doesn't, as you have the comparison.
Ack, yes thats true. Lets do it this way then. I am fine with that.
DPU_ERROR_ENC(dpu_enc,
"no ctl block assigned at idx: %d\n", i);
return;
