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

  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@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 7 +++++++
 1 file changed, 7 insertions(+)
  

On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
> ---
>  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.

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

On 12/11/2023 10:40 PM, Dmitry Baryshkov wrote:
> On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
>> ---
>>   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
>>
> 
>
  

On Tue, 12 Dec 2023 at 19:17, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/11/2023 10:40 PM, Dmitry Baryshkov wrote:
> > On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
> >> ---
> >>   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?

There are no messages by default there, because otherwise it is so
easy for the user to overspam the dmesg and thus syslog / journal. DoS
on the plate.

>
> But then we would not know why it failed.

Think about the user of X11. They don't see the console. And by
default in the contemporary distros they won't be able to check dmesg.
So if a commit fails, they have to deduce anyway, why did it fail.

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