Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init
On 2023-07-30 03:16:59, Dmitry Baryshkov wrote:
> >>> + 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
Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init
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
---
.../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 = _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(_enc->pending_vblank_cnt, 0);
init_waitqueue_head(_enc->pending_vblank_wq);
--
2.39.2
--
With best wishes
Dmitry
Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init
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
> > ---
> > .../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?
- 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 = _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(_enc->pending_vblank_cnt, 0);
> > init_waitqueue_head(_enc->pending_vblank_wq);
> >
> > --
> > 2.39.2
> >
Re: [PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init
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
> ---
> .../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?
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 = _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(_enc->pending_vblank_cnt, 0);
> init_waitqueue_head(_enc->pending_vblank_wq);
>
> --
> 2.39.2
>
[PATCH 7/7] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init
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
---
.../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)) {
+ 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 = _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) {
+ DPU_ERROR_CMDENC(cmd_enc, "tearcheck not supported\n");
+
+ return ERR_PTR(-EINVAL);
+ }
+
atomic_set(_enc->pending_vblank_cnt, 0);
init_waitqueue_head(_enc->pending_vblank_wq);
--
2.39.2
