Re: [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder

[Abhinav Kumar]

On 12/11/2023 10:40 PM, Dmitry Baryshkov wrote:
> On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > In preparation for adding more formats to dpu writeback add
> > format validation to it to fail any unsupported formats.
> >
> > changes in v3:
> >          - rebase on top of msm-next
> >          - replace drm_atomic_helper_check_wb_encoder_state() with
> >            drm_atomic_helper_check_wb_connector_state() due to the
> >            rebase
> >
> > changes in v2:
> >          - correct some grammar in the commit text
> >
> > Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback")
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > index bb94909caa25..425415d45ec1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > @@ -272,6 +272,7 @@ static int dpu_encoder_phys_wb_atomic_check(
> >   {
> >          struct drm_framebuffer *fb;
> >          const struct drm_display_mode *mode = &crtc_state->mode;
> > +       int ret;
> >
> >          DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> >                          phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
> > @@ -308,6 +309,12 @@ static int dpu_encoder_phys_wb_atomic_check(
> >                  return -EINVAL;
> >          }
> >
> > +       ret = drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> > +       if (ret < 0) {
> > +               DPU_ERROR("invalid pixel format %p4cc\n", &fb->format->format);
> > +               return ret;
> > +       }
>
> There is no guarantee that there will be no other checks added to this
> helper. So, I think this message is incorrect. If you wish, you can
> promote the level of the message in the helper itself.
> On the other hand, we rarely print such messages by default. Most of
> the checks use drm_dbg.

hmm...actually drm_atomic_helper_check_wb_connector_state() already has 
a debug message to indicate invalid pixel formats.

You are right, i should perhaps just say that "atomic_check failed" or 
something.

I can make this a DPU_DEBUG. Actually I didnt know that we are not 
supposed to print out atomic_check() errors. Is it perhaps because its 
okay for check to fail?

But then we would not know why it failed.

> > +
> >          return 0;
> >   }
> >
> > --
> > 2.40.1