[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 at 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 at 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
>>
> 
>