| Message ID | 20230404130622.509628-2-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: rework HW catalog | expand |
On 05/04/2023 01:12, Abhinav Kumar wrote: > > > On 4/4/2023 6:05 AM, Dmitry Baryshkov wrote: >> On sm8450 platform the CTL_0 doesn't differ from the rest of CTL blocks, >> so switch it to CTL_SC7280_MASK too. >> >> Some background: original commit 100d7ef6995d ("drm/msm/dpu: add support >> for SM8450") had all (relevant at that time) bit spelled individually. >> Then commit 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog"), >> despite being a mismerge, correctly changed all other CTL entries to use >> CTL_SC7280_MASK, except CTL_0. >> > > I think having it spelled individually is better. If we start using one > chipset's mask for another, we are again going down the same path of > this becoming one confused file. > > So, even though I agree that 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to > hw catalog") corrected the mask to re-use sc7280, with the individual > catalog file, its better to have it separate and spelled individually. > > This change is not heading in the direction of the rest of the series. I didn't create duplicates of all the defines. This is done well in the style of patch37. I'm not going to add all per-SoC feature masks. > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >> index 6840b22a4159..83f8f83e2b29 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >> @@ -975,7 +975,7 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = { >> { >> .name = "ctl_0", .id = CTL_0, >> .base = 0x15000, .len = 0x204, >> - .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_SPLIT_DISPLAY) >> | BIT(DPU_CTL_FETCH_ACTIVE), >> + .features = BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK, >> .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9), >> }, >> {
On 4/4/2023 5:33 PM, Dmitry Baryshkov wrote: > On 05/04/2023 01:12, Abhinav Kumar wrote: >> >> >> On 4/4/2023 6:05 AM, Dmitry Baryshkov wrote: >>> On sm8450 platform the CTL_0 doesn't differ from the rest of CTL blocks, >>> so switch it to CTL_SC7280_MASK too. >>> >>> Some background: original commit 100d7ef6995d ("drm/msm/dpu: add support >>> for SM8450") had all (relevant at that time) bit spelled individually. >>> Then commit 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog"), >>> despite being a mismerge, correctly changed all other CTL entries to use >>> CTL_SC7280_MASK, except CTL_0. >>> >> >> I think having it spelled individually is better. If we start using >> one chipset's mask for another, we are again going down the same path >> of this becoming one confused file. >> >> So, even though I agree that 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to >> hw catalog") corrected the mask to re-use sc7280, with the individual >> catalog file, its better to have it separate and spelled individually. >> >> This change is not heading in the direction of the rest of the series. > > I didn't create duplicates of all the defines. This is done well in the > style of patch37. I'm not going to add all per-SoC feature masks. > Yes, I was actually going to comment even on patch 37. We are again trying to generalize a CTL's caps based on DPU version, the same mistake which led us down this path. So today you have CTL_DPU_0_MASK , CTL_DPU_5_MASK , CTL_DPU_7_MASK and CTL_DPU_9_MASK and this builds on an assumption that you can get 5 by ORing ACTIVE_CFG with 0. +#define CTL_DPU_5_MASK (CTL_DPU_0_MASK | \ + BIT(DPU_CTL_ACTIVE_CFG)) + This is again moving towards that problematic pattern. Why dont we stick to CTL features individually spelling it then work towards generalizing as we discussed. >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>> index 6840b22a4159..83f8f83e2b29 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>> @@ -975,7 +975,7 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = { >>> { >>> .name = "ctl_0", .id = CTL_0, >>> .base = 0x15000, .len = 0x204, >>> - .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_SPLIT_DISPLAY) >>> | BIT(DPU_CTL_FETCH_ACTIVE), >>> + .features = BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK, >>> .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9), >>> }, >>> { >
On 05/04/2023 03:39, Abhinav Kumar wrote: > > > On 4/4/2023 5:33 PM, Dmitry Baryshkov wrote: >> On 05/04/2023 01:12, Abhinav Kumar wrote: >>> >>> >>> On 4/4/2023 6:05 AM, Dmitry Baryshkov wrote: >>>> On sm8450 platform the CTL_0 doesn't differ from the rest of CTL >>>> blocks, >>>> so switch it to CTL_SC7280_MASK too. >>>> >>>> Some background: original commit 100d7ef6995d ("drm/msm/dpu: add >>>> support >>>> for SM8450") had all (relevant at that time) bit spelled individually. >>>> Then commit 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog"), >>>> despite being a mismerge, correctly changed all other CTL entries to >>>> use >>>> CTL_SC7280_MASK, except CTL_0. >>>> >>> >>> I think having it spelled individually is better. If we start using >>> one chipset's mask for another, we are again going down the same path >>> of this becoming one confused file. >>> >>> So, even though I agree that 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 >>> to hw catalog") corrected the mask to re-use sc7280, with the >>> individual catalog file, its better to have it separate and spelled >>> individually. >>> >>> This change is not heading in the direction of the rest of the series. >> >> I didn't create duplicates of all the defines. This is done well in >> the style of patch37. I'm not going to add all per-SoC feature masks. >> > > Yes, I was actually going to comment even on patch 37. > > We are again trying to generalize a CTL's caps based on DPU version, the > same mistake which led us down this path. > > So today you have CTL_DPU_0_MASK , CTL_DPU_5_MASK , CTL_DPU_7_MASK and > CTL_DPU_9_MASK and this builds on an assumption that you can get 5 by > ORing ACTIVE_CFG with 0. > > +#define CTL_DPU_5_MASK (CTL_DPU_0_MASK | \ > + BIT(DPU_CTL_ACTIVE_CFG)) > + > > This is again moving towards that problematic pattern. > > Why dont we stick to CTL features individually spelling it then work > towards generalizing as we discussed. Because adding a feature would become a nightmare of touching all the platforms? We discussed not merging on major+LM. Glad, I agreed there. But I don't think that we should remove existing defines without good reason. We know that they work in the majority of cases. >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>> index 6840b22a4159..83f8f83e2b29 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>> @@ -975,7 +975,7 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = { >>>> { >>>> .name = "ctl_0", .id = CTL_0, >>>> .base = 0x15000, .len = 0x204, >>>> - .features = BIT(DPU_CTL_ACTIVE_CFG) | >>>> BIT(DPU_CTL_SPLIT_DISPLAY) | BIT(DPU_CTL_FETCH_ACTIVE), >>>> + .features = BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK, >>>> .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9), >>>> }, >>>> { >>
On 4/4/2023 5:43 PM, Dmitry Baryshkov wrote: > On 05/04/2023 03:39, Abhinav Kumar wrote: >> >> >> On 4/4/2023 5:33 PM, Dmitry Baryshkov wrote: >>> On 05/04/2023 01:12, Abhinav Kumar wrote: >>>> >>>> >>>> On 4/4/2023 6:05 AM, Dmitry Baryshkov wrote: >>>>> On sm8450 platform the CTL_0 doesn't differ from the rest of CTL >>>>> blocks, >>>>> so switch it to CTL_SC7280_MASK too. >>>>> >>>>> Some background: original commit 100d7ef6995d ("drm/msm/dpu: add >>>>> support >>>>> for SM8450") had all (relevant at that time) bit spelled individually. >>>>> Then commit 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog"), >>>>> despite being a mismerge, correctly changed all other CTL entries >>>>> to use >>>>> CTL_SC7280_MASK, except CTL_0. >>>>> >>>> >>>> I think having it spelled individually is better. If we start using >>>> one chipset's mask for another, we are again going down the same >>>> path of this becoming one confused file. >>>> >>>> So, even though I agree that 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 >>>> to hw catalog") corrected the mask to re-use sc7280, with the >>>> individual catalog file, its better to have it separate and spelled >>>> individually. >>>> >>>> This change is not heading in the direction of the rest of the series. >>> >>> I didn't create duplicates of all the defines. This is done well in >>> the style of patch37. I'm not going to add all per-SoC feature masks. >>> >> >> Yes, I was actually going to comment even on patch 37. >> >> We are again trying to generalize a CTL's caps based on DPU version, >> the same mistake which led us down this path. >> >> So today you have CTL_DPU_0_MASK , CTL_DPU_5_MASK , CTL_DPU_7_MASK >> and CTL_DPU_9_MASK and this builds on an assumption that you can get 5 >> by ORing ACTIVE_CFG with 0. >> >> +#define CTL_DPU_5_MASK (CTL_DPU_0_MASK | \ >> + BIT(DPU_CTL_ACTIVE_CFG)) >> + >> >> This is again moving towards that problematic pattern. >> >> Why dont we stick to CTL features individually spelling it then work >> towards generalizing as we discussed. > > Because adding a feature would become a nightmare of touching all the > platforms? > On the contrary, this would help us to enable the feature where it was verified and not generalize it that when it works on one chipset it will work on the other. There is another point of view here which we have already seen. If we go with generalizing, then when we find one case which is different, we end up decoupling the generalization and thats more painful and led us to the rework in the first place. > We discussed not merging on major+LM. Glad, I agreed there. But I don't > think that we should remove existing defines without good reason. We > know that they work in the majority of cases. > Ofcourse it will work today because we have covered only supported chipsets but its just the start of a design which we know led us to this rework. >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> index 6840b22a4159..83f8f83e2b29 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> @@ -975,7 +975,7 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = { >>>>> { >>>>> .name = "ctl_0", .id = CTL_0, >>>>> .base = 0x15000, .len = 0x204, >>>>> - .features = BIT(DPU_CTL_ACTIVE_CFG) | >>>>> BIT(DPU_CTL_SPLIT_DISPLAY) | BIT(DPU_CTL_FETCH_ACTIVE), >>>>> + .features = BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK, >>>>> .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9), >>>>> }, >>>>> { >>> >
On 05/04/2023 04:00, Abhinav Kumar wrote: > > > On 4/4/2023 5:43 PM, Dmitry Baryshkov wrote: >> On 05/04/2023 03:39, Abhinav Kumar wrote: >>> >>> >>> On 4/4/2023 5:33 PM, Dmitry Baryshkov wrote: >>>> On 05/04/2023 01:12, Abhinav Kumar wrote: >>>>> >>>>> >>>>> On 4/4/2023 6:05 AM, Dmitry Baryshkov wrote: >>>>>> On sm8450 platform the CTL_0 doesn't differ from the rest of CTL >>>>>> blocks, >>>>>> so switch it to CTL_SC7280_MASK too. >>>>>> >>>>>> Some background: original commit 100d7ef6995d ("drm/msm/dpu: add >>>>>> support >>>>>> for SM8450") had all (relevant at that time) bit spelled >>>>>> individually. >>>>>> Then commit 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog"), >>>>>> despite being a mismerge, correctly changed all other CTL entries >>>>>> to use >>>>>> CTL_SC7280_MASK, except CTL_0. >>>>>> >>>>> >>>>> I think having it spelled individually is better. If we start using >>>>> one chipset's mask for another, we are again going down the same >>>>> path of this becoming one confused file. >>>>> >>>>> So, even though I agree that 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 >>>>> to hw catalog") corrected the mask to re-use sc7280, with the >>>>> individual catalog file, its better to have it separate and spelled >>>>> individually. >>>>> >>>>> This change is not heading in the direction of the rest of the series. >>>> >>>> I didn't create duplicates of all the defines. This is done well in >>>> the style of patch37. I'm not going to add all per-SoC feature masks. >>>> >>> >>> Yes, I was actually going to comment even on patch 37. >>> >>> We are again trying to generalize a CTL's caps based on DPU version, >>> the same mistake which led us down this path. >>> >>> So today you have CTL_DPU_0_MASK , CTL_DPU_5_MASK , CTL_DPU_7_MASK >>> and CTL_DPU_9_MASK and this builds on an assumption that you can get >>> 5 by ORing ACTIVE_CFG with 0. >>> >>> +#define CTL_DPU_5_MASK (CTL_DPU_0_MASK | \ >>> + BIT(DPU_CTL_ACTIVE_CFG)) >>> + >>> >>> This is again moving towards that problematic pattern. >>> >>> Why dont we stick to CTL features individually spelling it then work >>> towards generalizing as we discussed. >> >> Because adding a feature would become a nightmare of touching all the >> platforms? >> > > On the contrary, this would help us to enable the feature where it was > verified and not generalize it that when it works on one chipset it will > work on the other. We have been discussing this while I was working on wide planes. I agreed there, because it was the topic which can impact a lot. In the same way the virtual planes even have a modparam knob as to limit the impact. However I still have the understanding that this is not how the things should be working. This is not how we are doing development/cleanups in other areas. The usual practice is perform the change, verify it as much as possible, collect the fallouts. Doing it other way around would mostly stop the development. > > There is another point of view here which we have already seen. > > If we go with generalizing, then when we find one case which is > different, we end up decoupling the generalization and thats more > painful and led us to the rework in the first place. No, squashing everything together led us to the rework. I'd prefer to see whole picture before reworking this part. I'm not going to do everything at once. So far the masks have been working in the boundaries of generations. The only one which didn't is the IRQ mask. It gets inlined. If you want it other way, currently we have three defines which do not fall into the DPUn standard. I'd prefer to get more date before making a decision there. > > >> We discussed not merging on major+LM. Glad, I agreed there. But I >> don't think that we should remove existing defines without good >> reason. We know that they work in the majority of cases. >> > > Ofcourse it will work today because we have covered only supported > chipsets but its just the start of a design which we know led us to this > rework. Again, getting samples led us to the understanding and then the rework. Doing rework without understanding the issues is like premature optimization. > > >>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>> --- >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>>> index 6840b22a4159..83f8f83e2b29 100644 >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>>> @@ -975,7 +975,7 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = { >>>>>> { >>>>>> .name = "ctl_0", .id = CTL_0, >>>>>> .base = 0x15000, .len = 0x204, >>>>>> - .features = BIT(DPU_CTL_ACTIVE_CFG) | >>>>>> BIT(DPU_CTL_SPLIT_DISPLAY) | BIT(DPU_CTL_FETCH_ACTIVE), >>>>>> + .features = BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK, >>>>>> .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9), >>>>>> }, >>>>>> { >>>> >>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c index 6840b22a4159..83f8f83e2b29 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -975,7 +975,7 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = { { .name = "ctl_0", .id = CTL_0, .base = 0x15000, .len = 0x204, - .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_SPLIT_DISPLAY) | BIT(DPU_CTL_FETCH_ACTIVE), + .features = BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK, .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9), }, {
On sm8450 platform the CTL_0 doesn't differ from the rest of CTL blocks, so switch it to CTL_SC7280_MASK too. Some background: original commit 100d7ef6995d ("drm/msm/dpu: add support for SM8450") had all (relevant at that time) bit spelled individually. Then commit 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog"), despite being a mismerge, correctly changed all other CTL entries to use CTL_SC7280_MASK, except CTL_0. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)