Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <59304cf8-33b7-bf27-f9e2-a15e323c869a@quicinc.com>
Date:   Tue, 12 Dec 2023 09:16:59 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback
 encoder
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     <freedreno@lists.freedesktop.org>, Rob Clark <robdclark@gmail.com>,
        "Sean Paul" <sean@poorly.run>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        "David Airlie" <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        <dri-devel@lists.freedesktop.org>, <seanpaul@chromium.org>,
        <quic_jesszhan@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20231212002245.23715-1-quic_abhinavk@quicinc.com>
 <20231212002245.23715-2-quic_abhinavk@quicinc.com>
 <CAA8EJpqJOh0R1X3i1UGe9hHoezV4uBNDCWPFSdeuXyC6Ju4eHA@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAA8EJpqJOh0R1X3i1UGe9hHoezV4uBNDCWPFSdeuXyC6Ju4eHA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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