| Message ID | 20230727162104.1497483-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dpu: drop DPU_INTF_TE and DPU_PINGPONG_TE | expand |
On 27/07/2023 23:10, Marijn Suijten wrote: > On 2023-07-27 19:21:00, Dmitry Baryshkov wrote: >> Inline the _setup_intf_ops() function, it makes it easier to handle >> different conditions involving INTF configuration. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------ >> 1 file changed, 21 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >> index 8ec6505d9e78..7ca772791a73 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx, >> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2); >> } >> >> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, >> - unsigned long cap, const struct dpu_mdss_version *mdss_rev) >> -{ >> - ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine; >> - ops->setup_prg_fetch = dpu_hw_intf_setup_prg_fetch; >> - ops->get_status = dpu_hw_intf_get_status; >> - ops->enable_timing = dpu_hw_intf_enable_timing_engine; >> - ops->get_line_count = dpu_hw_intf_get_line_count; >> - if (cap & BIT(DPU_INTF_INPUT_CTRL)) >> - ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk; >> - ops->setup_misr = dpu_hw_intf_setup_misr; >> - ops->collect_misr = dpu_hw_intf_collect_misr; >> - >> - if (cap & BIT(DPU_INTF_TE)) { >> - ops->enable_tearcheck = dpu_hw_intf_enable_te; >> - ops->disable_tearcheck = dpu_hw_intf_disable_te; >> - ops->connect_external_te = dpu_hw_intf_connect_external_te; >> - ops->vsync_sel = dpu_hw_intf_vsync_sel; >> - ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh; >> - } >> - >> - if (mdss_rev->core_major_ver >= 7) >> - ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg; >> -} >> - >> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, >> void __iomem *addr, const struct dpu_mdss_version *mdss_rev) >> { >> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, >> */ >> c->idx = cfg->id; >> c->cap = cfg; >> - _setup_intf_ops(&c->ops, c->cap->features, mdss_rev); >> + >> + c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine; >> + c->ops.setup_prg_fetch = dpu_hw_intf_setup_prg_fetch; >> + c->ops.get_status = dpu_hw_intf_get_status; >> + c->ops.enable_timing = dpu_hw_intf_enable_timing_engine; >> + c->ops.get_line_count = dpu_hw_intf_get_line_count; >> + if (cfg->features & BIT(DPU_INTF_INPUT_CTRL)) >> + c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk; > > While at it, could we sort these down with the other conditional > callbacks? What kind of sorting do you have in mind? >> + c->ops.setup_misr = dpu_hw_intf_setup_misr; >> + c->ops.collect_misr = dpu_hw_intf_collect_misr; >> + >> + if (cfg->features & BIT(DPU_INTF_TE)) { > > Any clue why we're not using test_bit()? Feels a bit inconsistent. Yes, some files use test_bit(), others just check the bit directly. Maybe after moving some/most of conditionals to core_major_ver we can clean that too. > >> + c->ops.enable_tearcheck = dpu_hw_intf_enable_te; >> + c->ops.disable_tearcheck = dpu_hw_intf_disable_te; >> + c->ops.connect_external_te = dpu_hw_intf_connect_external_te; >> + c->ops.vsync_sel = dpu_hw_intf_vsync_sel; >> + c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh; >> + } >> + >> + if (mdss_rev->core_major_ver >= 7) >> + c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg; >> >> return c; >> } >> -- >> 2.39.2 >>
On 27/07/2023 23:05, Marijn Suijten wrote: > On 2023-07-27 19:20:58, Dmitry Baryshkov wrote: >> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0. >> Rather than checking for the flag, check for the presense of the >> corresponding interrupt line. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >> index 9298c166b213..912a3bdf8ad4 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg, >> c->idx = cfg->id; >> c->caps = cfg; > > In hindsight, maybe there's one patch missing from this series. You > inlined _setup_intf_ops() later, but there's no patch inlining > _setup_pingpong_ops() which looks to be required for applying this > patch. Yes, I missed it somehow. > > - Marijn > >> >> - if (test_bit(DPU_PINGPONG_TE, &cfg->features)) { >> + if (cfg->intr_rdptr) { >> c->ops.enable_tearcheck = dpu_hw_pp_enable_te; >> c->ops.disable_tearcheck = dpu_hw_pp_disable_te; >> c->ops.connect_external_te = dpu_hw_pp_connect_external_te; >> -- >> 2.39.2 >>
On 27/07/2023 23:03, Marijn Suijten wrote: > On 2023-07-27 19:20:58, Dmitry Baryshkov wrote: >> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0. >> Rather than checking for the flag, check for the presense of the >> corresponding interrupt line. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > That's a smart use of the interrupt field. I both like it, and I do > not. While we didn't do any validation for consistency previously, this > means we now have multiple ways of controlling available "features": > > - Feature flags on hardware blocks; > - Presence of certain IRQs; > - DPU core revision. I hesitated here too. For INTF it is clear that there is no other good way to check for the TE feature. For PP we can just check for the DPU revision. > > Maybe that is more confusing to follow? Regardless of that I'm > convinced that this patch does what it's supposed to and gets rid of > some ambiguity. Maybe a comment above the IF explaining the "PP TE" > feature could alleviate the above concerns thoo. Hence: > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >> index 9298c166b213..912a3bdf8ad4 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg, >> c->idx = cfg->id; >> c->caps = cfg; >> >> - if (test_bit(DPU_PINGPONG_TE, &cfg->features)) { >> + if (cfg->intr_rdptr) { >> c->ops.enable_tearcheck = dpu_hw_pp_enable_te; >> c->ops.disable_tearcheck = dpu_hw_pp_disable_te; >> c->ops.connect_external_te = dpu_hw_pp_connect_external_te; >> -- >> 2.39.2 >>
On 2023-07-29 02:45:43, Dmitry Baryshkov wrote: > On 27/07/2023 23:10, Marijn Suijten wrote: > > On 2023-07-27 19:21:00, Dmitry Baryshkov wrote: > >> Inline the _setup_intf_ops() function, it makes it easier to handle > >> different conditions involving INTF configuration. > >> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------ > >> 1 file changed, 21 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >> index 8ec6505d9e78..7ca772791a73 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx, > >> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2); > >> } > >> > >> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, > >> - unsigned long cap, const struct dpu_mdss_version *mdss_rev) > >> -{ > >> - ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine; > >> - ops->setup_prg_fetch = dpu_hw_intf_setup_prg_fetch; > >> - ops->get_status = dpu_hw_intf_get_status; > >> - ops->enable_timing = dpu_hw_intf_enable_timing_engine; > >> - ops->get_line_count = dpu_hw_intf_get_line_count; > >> - if (cap & BIT(DPU_INTF_INPUT_CTRL)) > >> - ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk; > >> - ops->setup_misr = dpu_hw_intf_setup_misr; > >> - ops->collect_misr = dpu_hw_intf_collect_misr; > >> - > >> - if (cap & BIT(DPU_INTF_TE)) { > >> - ops->enable_tearcheck = dpu_hw_intf_enable_te; > >> - ops->disable_tearcheck = dpu_hw_intf_disable_te; > >> - ops->connect_external_te = dpu_hw_intf_connect_external_te; > >> - ops->vsync_sel = dpu_hw_intf_vsync_sel; > >> - ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh; > >> - } > >> - > >> - if (mdss_rev->core_major_ver >= 7) > >> - ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg; > >> -} > >> - > >> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, > >> void __iomem *addr, const struct dpu_mdss_version *mdss_rev) > >> { > >> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, > >> */ > >> c->idx = cfg->id; > >> c->cap = cfg; > >> - _setup_intf_ops(&c->ops, c->cap->features, mdss_rev); > >> + > >> + c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine; > >> + c->ops.setup_prg_fetch = dpu_hw_intf_setup_prg_fetch; > >> + c->ops.get_status = dpu_hw_intf_get_status; > >> + c->ops.enable_timing = dpu_hw_intf_enable_timing_engine; > >> + c->ops.get_line_count = dpu_hw_intf_get_line_count; > >> + if (cfg->features & BIT(DPU_INTF_INPUT_CTRL)) > >> + c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk; > > > > While at it, could we sort these down with the other conditional > > callbacks? > > What kind of sorting do you have in mind? Moving this conditional ( if (...) ) down with the other conditional assignment below, instead of being right in the middle of get_line_count and setup_misr, both which are not conditional and make it harder to read, especially considering the lack of newlines and/or curly braces. > >> + c->ops.setup_misr = dpu_hw_intf_setup_misr; > >> + c->ops.collect_misr = dpu_hw_intf_collect_misr; > >> + > >> + if (cfg->features & BIT(DPU_INTF_TE)) { > > > > Any clue why we're not using test_bit()? Feels a bit inconsistent. > > Yes, some files use test_bit(), others just check the bit directly. > Maybe after moving some/most of conditionals to core_major_ver we can > clean that too. Sounds good. - Marijn > >> + c->ops.enable_tearcheck = dpu_hw_intf_enable_te; > >> + c->ops.disable_tearcheck = dpu_hw_intf_disable_te; > >> + c->ops.connect_external_te = dpu_hw_intf_connect_external_te; > >> + c->ops.vsync_sel = dpu_hw_intf_vsync_sel; > >> + c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh; > >> + } > >> + > >> + if (mdss_rev->core_major_ver >= 7) > >> + c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg; > >> > >> return c; > >> } > >> -- > >> 2.39.2 > >> > > -- > With best wishes > Dmitry >
On 2023-07-29 02:59:32, Dmitry Baryshkov wrote: > On 27/07/2023 23:03, Marijn Suijten wrote: > > On 2023-07-27 19:20:58, Dmitry Baryshkov wrote: > >> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0. > >> Rather than checking for the flag, check for the presense of the > >> corresponding interrupt line. > >> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > That's a smart use of the interrupt field. I both like it, and I do > > not. While we didn't do any validation for consistency previously, this > > means we now have multiple ways of controlling available "features": > > > > - Feature flags on hardware blocks; > > - Presence of certain IRQs; > > - DPU core revision. > > I hesitated here too. For INTF it is clear that there is no other good > way to check for the TE feature. For PP we can just check for the DPU > revision. For both we could additionally check the DPU revision, and for INTF we could check for TYPE_DSI. Both would aid in extra validation, if we require the IRQ to be present or absent under these conditions. It might also help to document this, either here and on the respective struct fields so that catalog implementers know when they should supply or leave out an IRQ? - Marijn > > Maybe that is more confusing to follow? Regardless of that I'm > > convinced that this patch does what it's supposed to and gets rid of > > some ambiguity. Maybe a comment above the IF explaining the "PP TE" > > feature could alleviate the above concerns thoo. Hence: > > > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c > >> index 9298c166b213..912a3bdf8ad4 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c > >> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg, > >> c->idx = cfg->id; > >> c->caps = cfg; > >> > >> - if (test_bit(DPU_PINGPONG_TE, &cfg->features)) { > >> + if (cfg->intr_rdptr) { > >> c->ops.enable_tearcheck = dpu_hw_pp_enable_te; > >> c->ops.disable_tearcheck = dpu_hw_pp_disable_te; > >> c->ops.connect_external_te = dpu_hw_pp_connect_external_te; > >> -- > >> 2.39.2 > >> > > -- > With best wishes > Dmitry >
On Sat, 29 Jul 2023 at 21:28, Marijn Suijten <marijn.suijten@somainline.org> wrote: > > On 2023-07-29 02:45:43, Dmitry Baryshkov wrote: > > On 27/07/2023 23:10, Marijn Suijten wrote: > > > On 2023-07-27 19:21:00, Dmitry Baryshkov wrote: > > >> Inline the _setup_intf_ops() function, it makes it easier to handle > > >> different conditions involving INTF configuration. > > >> > > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > > >> --- > > >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------ > > >> 1 file changed, 21 insertions(+), 26 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > >> index 8ec6505d9e78..7ca772791a73 100644 > > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > >> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx, > > >> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2); > > >> } > > >> > > >> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, > > >> - unsigned long cap, const struct dpu_mdss_version *mdss_rev) > > >> -{ > > >> - ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine; > > >> - ops->setup_prg_fetch = dpu_hw_intf_setup_prg_fetch; > > >> - ops->get_status = dpu_hw_intf_get_status; > > >> - ops->enable_timing = dpu_hw_intf_enable_timing_engine; > > >> - ops->get_line_count = dpu_hw_intf_get_line_count; > > >> - if (cap & BIT(DPU_INTF_INPUT_CTRL)) > > >> - ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk; > > >> - ops->setup_misr = dpu_hw_intf_setup_misr; > > >> - ops->collect_misr = dpu_hw_intf_collect_misr; > > >> - > > >> - if (cap & BIT(DPU_INTF_TE)) { > > >> - ops->enable_tearcheck = dpu_hw_intf_enable_te; > > >> - ops->disable_tearcheck = dpu_hw_intf_disable_te; > > >> - ops->connect_external_te = dpu_hw_intf_connect_external_te; > > >> - ops->vsync_sel = dpu_hw_intf_vsync_sel; > > >> - ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh; > > >> - } > > >> - > > >> - if (mdss_rev->core_major_ver >= 7) > > >> - ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg; > > >> -} > > >> - > > >> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, > > >> void __iomem *addr, const struct dpu_mdss_version *mdss_rev) > > >> { > > >> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, > > >> */ > > >> c->idx = cfg->id; > > >> c->cap = cfg; > > >> - _setup_intf_ops(&c->ops, c->cap->features, mdss_rev); > > >> + > > >> + c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine; > > >> + c->ops.setup_prg_fetch = dpu_hw_intf_setup_prg_fetch; > > >> + c->ops.get_status = dpu_hw_intf_get_status; > > >> + c->ops.enable_timing = dpu_hw_intf_enable_timing_engine; > > >> + c->ops.get_line_count = dpu_hw_intf_get_line_count; > > >> + if (cfg->features & BIT(DPU_INTF_INPUT_CTRL)) > > >> + c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk; > > > > > > While at it, could we sort these down with the other conditional > > > callbacks? > > > > What kind of sorting do you have in mind? > > Moving this conditional ( if (...) ) down with the other conditional > assignment below, instead of being right in the middle of get_line_count > and setup_misr, both which are not conditional and make it harder to > read, especially considering the lack of newlines and/or curly braces. Ack, sounds good. > > > >> + c->ops.setup_misr = dpu_hw_intf_setup_misr; > > >> + c->ops.collect_misr = dpu_hw_intf_collect_misr; > > >> + > > >> + if (cfg->features & BIT(DPU_INTF_TE)) { > > > > > > Any clue why we're not using test_bit()? Feels a bit inconsistent. > > > > Yes, some files use test_bit(), others just check the bit directly. > > Maybe after moving some/most of conditionals to core_major_ver we can > > clean that too. > > Sounds good. > > - Marijn > > > >> + c->ops.enable_tearcheck = dpu_hw_intf_enable_te; > > >> + c->ops.disable_tearcheck = dpu_hw_intf_disable_te; > > >> + c->ops.connect_external_te = dpu_hw_intf_connect_external_te; > > >> + c->ops.vsync_sel = dpu_hw_intf_vsync_sel; > > >> + c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh; > > >> + } > > >> + > > >> + if (mdss_rev->core_major_ver >= 7) > > >> + c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg; > > >> > > >> return c; > > >> } > > >> -- > > >> 2.39.2 > > >> > > > > -- > > With best wishes > > Dmitry > >
On 29/07/2023 21:31, Marijn Suijten wrote: > On 2023-07-29 02:59:32, Dmitry Baryshkov wrote: >> On 27/07/2023 23:03, Marijn Suijten wrote: >>> On 2023-07-27 19:20:58, Dmitry Baryshkov wrote: >>>> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0. >>>> Rather than checking for the flag, check for the presense of the >>>> corresponding interrupt line. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> >>> That's a smart use of the interrupt field. I both like it, and I do >>> not. While we didn't do any validation for consistency previously, this >>> means we now have multiple ways of controlling available "features": >>> >>> - Feature flags on hardware blocks; >>> - Presence of certain IRQs; >>> - DPU core revision. >> >> I hesitated here too. For INTF it is clear that there is no other good >> way to check for the TE feature. For PP we can just check for the DPU >> revision. > > For both we could additionally check the DPU revision, and for INTF we > could check for TYPE_DSI. Both would aid in extra validation, if we > require the IRQ to be present or absent under these conditions. Yep, maybe that's better. > > It might also help to document this, either here and on the respective > struct fields so that catalog implementers know when they should supply > or leave out an IRQ? Probably a WARN_ON would be enough. > > - Marijn > >>> Maybe that is more confusing to follow? Regardless of that I'm >>> convinced that this patch does what it's supposed to and gets rid of >>> some ambiguity. Maybe a comment above the IF explaining the "PP TE" >>> feature could alleviate the above concerns thoo. Hence: >>> >>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> >>> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >>>> index 9298c166b213..912a3bdf8ad4 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c >>>> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg, >>>> c->idx = cfg->id; >>>> c->caps = cfg; >>>> >>>> - if (test_bit(DPU_PINGPONG_TE, &cfg->features)) { >>>> + if (cfg->intr_rdptr) { >>>> c->ops.enable_tearcheck = dpu_hw_pp_enable_te; >>>> c->ops.disable_tearcheck = dpu_hw_pp_disable_te; >>>> c->ops.connect_external_te = dpu_hw_pp_connect_external_te; >>>> -- >>>> 2.39.2 >>>> >> >> -- >> With best wishes >> Dmitry >>
On 27/07/2023 23:25, Marijn Suijten wrote: > On 2023-07-27 22:22:20, Marijn Suijten wrote: >> On 2023-07-27 19:21:04, Dmitry Baryshkov wrote: >>> As the INTF is fixed at the encoder creation time, we can move the >>> check whether INTF supports tearchck to dpu_encoder_phys_cmd_init(). >>> This function can return an error if INTF doesn't have required feature. >>> Performing this check in dpu_encoder_phys_cmd_tearcheck_config() is less >>> useful, as this function returns void. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 37 +++++++++++-------- >>> 1 file changed, 21 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>> index 04a1106101a7..e1dd0e1b4793 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>> @@ -325,24 +325,17 @@ static void dpu_encoder_phys_cmd_tearcheck_config( >>> unsigned long vsync_hz; >>> struct dpu_kms *dpu_kms; >>> >>> - if (phys_enc->has_intf_te) { >>> - if (!phys_enc->hw_intf || >>> - !phys_enc->hw_intf->ops.enable_tearcheck) { >>> - DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n"); >>> - return; >>> - } >>> - >>> - DPU_DEBUG_CMDENC(cmd_enc, ""); >>> - } else { >>> - if (!phys_enc->hw_pp || >>> - !phys_enc->hw_pp->ops.enable_tearcheck) { >>> - DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n"); >>> - return; >>> - } >>> - >>> - DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0); >>> + if (!phys_enc->has_intf_te && >>> + (!phys_enc->hw_pp || >>> + !phys_enc->hw_pp->ops.enable_tearcheck)) { >> >> when is hw_pp assigned? Can't we also check that somewhere in an init >> phase? > > It would happen right before dpu_encoder_phys_cmd_atomic_mode_set() > where we already happen to check has_intf_te to switch on PP > intr_readptr vs INTF intr_tear_rd_ptr. Might be the perfect place for > the pingpong callback checks? The problem is that mode_set doesn't return an error (by design). I'd put a TODO here, so that if we ever move/change resource allocation, this check can be done next to it (atomic_check isn't a good place, since phys_enc.atomic_check happens before resource reallocation). > > - Marijn > >> >> Also, you won't go over 100 chars (not even 80) by having the (!... || >> !...) on a single line. >> >>> + DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n"); >>> + return; >>> } >>> >>> + DPU_DEBUG_CMDENC(cmd_enc, "intf %d pp %d\n", >>> + phys_enc->hw_intf->idx - INTF_0, >>> + phys_enc->hw_pp->idx - PINGPONG_0); >>> + >>> mode = &phys_enc->cached_mode; >>> >>> dpu_kms = phys_enc->dpu_kms; >>> @@ -768,9 +761,21 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init( >>> phys_enc->intf_mode = INTF_MODE_CMD; >>> cmd_enc->stream_sel = 0; >>> >>> + if (!phys_enc->hw_intf) { >>> + DPU_ERROR_CMDENC(cmd_enc, "no INTF provided\n"); >>> + >>> + return ERR_PTR(-EINVAL); >>> + } >>> + >>> if (phys_enc->dpu_kms->catalog->mdss_ver->core_major_ver >= 5) >>> phys_enc->has_intf_te = true; >>> >>> + if (phys_enc->has_intf_te && !phys_enc->hw_intf->ops.enable_tearcheck) { >> >> Any other callbacks we could check here, and remove the checks >> elsewhere? >> >> As with enable_tearcheck() though, it does make the code less consistent >> with its PP counterpart, which is checked ad-hoc everywhere (but maybe >> that is fixable too). >> >> - Marijn >> >>> + DPU_ERROR_CMDENC(cmd_enc, "tearcheck not supported\n"); >>> + >>> + return ERR_PTR(-EINVAL); >>> + } >>> + >>> atomic_set(&cmd_enc->pending_vblank_cnt, 0); >>> init_waitqueue_head(&cmd_enc->pending_vblank_wq); >>> >>> -- >>> 2.39.2 >>>
On 2023-07-30 02:18:10, Dmitry Baryshkov wrote: > On 29/07/2023 21:31, Marijn Suijten wrote: > > On 2023-07-29 02:59:32, Dmitry Baryshkov wrote: > >> On 27/07/2023 23:03, Marijn Suijten wrote: > >>> On 2023-07-27 19:20:58, Dmitry Baryshkov wrote: > >>>> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0. > >>>> Rather than checking for the flag, check for the presense of the > >>>> corresponding interrupt line. > >>>> > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> > >>> That's a smart use of the interrupt field. I both like it, and I do > >>> not. While we didn't do any validation for consistency previously, this > >>> means we now have multiple ways of controlling available "features": > >>> > >>> - Feature flags on hardware blocks; > >>> - Presence of certain IRQs; > >>> - DPU core revision. > >> > >> I hesitated here too. For INTF it is clear that there is no other good > >> way to check for the TE feature. For PP we can just check for the DPU > >> revision. > > > > For both we could additionally check the DPU revision, and for INTF we > > could check for TYPE_DSI. Both would aid in extra validation, if we > > require the IRQ to be present or absent under these conditions. > > Yep, maybe that's better. > > > > > It might also help to document this, either here and on the respective > > struct fields so that catalog implementers know when they should supply > > or leave out an IRQ? > > Probably a WARN_ON would be enough. SGTM, it is after all only for bring-up as after that (additions have been validated, reviewed and merged) we "trust the kernel" including our catalog, so errors like this should pretty much be unreachable. - Marijn
On 2023-07-30 03:16:59, Dmitry Baryshkov wrote: <snip> > >>> + if (!phys_enc->has_intf_te && > >>> + (!phys_enc->hw_pp || > >>> + !phys_enc->hw_pp->ops.enable_tearcheck)) { > >> > >> when is hw_pp assigned? Can't we also check that somewhere in an init > >> phase? > > > > It would happen right before dpu_encoder_phys_cmd_atomic_mode_set() > > where we already happen to check has_intf_te to switch on PP > > intr_readptr vs INTF intr_tear_rd_ptr. Might be the perfect place for > > the pingpong callback checks? > > The problem is that mode_set doesn't return an error (by design). I'd > put a TODO here, so that if we ever move/change resource allocation, > this check can be done next to it (atomic_check isn't a good place, > since phys_enc.atomic_check happens before resource reallocation). As thought of in another patch, perhaps it could just be a WARN_ON() as these almost always relate directly to constant information provided by the catalog, which we trust to be sound (and these error cases to hardly be reachable) after it has been validated, reviewed and merged. - Marijn
Drop two feature flags, DPU_INTF_TE and DPU_PINGPONG_TE, in favour of performing the MDSS revision checks instead. Dependencies: [1], [2] [1] https://patchwork.freedesktop.org/series/118088/ [2] https://patchwork.freedesktop.org/series/118836/ Dmitry Baryshkov (7): drm/msm/dpu: enable PINGPONG TE operations only when supported by HW drm/msm/dpu: drop the DPU_PINGPONG_TE flag drm/msm/dpu: inline _setup_intf_ops() drm/msm/dpu: enable INTF TE operations only when supported by HW drm/msm/dpu: drop DPU_INTF_TE feature flag drm/msm/dpu: drop useless check from dpu_encoder_phys_cmd_te_rd_ptr_irq() drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 49 +++++++++---------- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 +- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 6 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 ++++++++---------- .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +- 5 files changed, 47 insertions(+), 60 deletions(-) base-commit: be4dacf4eee1c4e2e91586e75e95b2852274dc58 prerequisite-patch-id: be3f3e5df89f9f2cc6b6289a83422d65e29d4900 prerequisite-patch-id: 59cd61ccd3cde7218fe3db6a8c672faafb7167f5 prerequisite-patch-id: d493b9bd85d518c15ca94f22174cb5ab2e2443d9 prerequisite-patch-id: 6a5bf3083c3da70ca110c17d54027e96c0335636 prerequisite-patch-id: 670883101f3b5dca122501f2828d8e45a6843b38 prerequisite-patch-id: 5dacdaf8ba4b369b966ca341db6691b208476fa1 prerequisite-patch-id: 5d8ff96e0fbea3358931d9d1fcb1bf114ae172df prerequisite-patch-id: 9d7a4964337aee22c325fa04424f1e20946e1bb4 prerequisite-patch-id: 7310e0a9f3aa611cb080930a4c8247ced69ed5d5 prerequisite-patch-id: ec7e1b84ef2780c43cf59e2c2bf638d7eff188fd