Re: [PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation

[Igor Torrente <igormtorrente@gmail.com>]

Hi Thomas,

On 11/3/21 12:37 PM, Thomas Zimmermann wrote:
> Hi
> 
> Am 03.11.21 um 16:11 schrieb Leandro Ribeiro:
>> Hi,
>>
>> On 11/3/21 12:03, Igor Torrente wrote:
>>> Hi Leandro,
>>>
>>> On 10/28/21 6:38 PM, Leandro Ribeiro wrote:
>>>> Hi,
>>>>
>>>> On 10/26/21 08:34, Igor Torrente wrote:
>>>>> Add a helper function to validate the connector configuration receive in
>>>>> the encoder atomic_check by the drivers.
>>>>>
>>>>> So the drivers don't need do these common validations themselves.
>>>>>
>>>>> Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
>>>>> ---
>>>>> V2: Move the format verification to a new helper at the
>>>>> drm_atomic_helper.c
>>>>>        (Thomas Zimmermann).
>>>>> ---
>>>>>     drivers/gpu/drm/drm_atomic_helper.c   | 47 +++++++++++++++++++++++++++
>>>>>     drivers/gpu/drm/vkms/vkms_writeback.c |  9 +++--
>>>>>     include/drm/drm_atomic_helper.h       |  3 ++
>>>>>     3 files changed, 54 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> index 2c0c6ec92820..c2653b9824b5 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct
>>>>> drm_device *dev,
>>>>>     }
>>>>>     EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>>>>>     +/**
>>>>> + * drm_atomic_helper_check_wb_connector_state() - Check writeback
>>>>> encoder state
>>>>> + * @encoder: encoder state to check
>>>>> + * @conn_state: connector state to check
>>>>> + *
>>>>> + * Checks if the wriback connector state is valid, and returns a
> 
> 'writeback'
> 
> 'an error'
> 
>>>>> erros if it
> 
> 'error'
> 
>>>>> + * isn't.
>>>>> + *
>>>>> + * RETURNS:
>>>>> + * Zero for success or -errno
>>>>> + */
>>>>> +int
>>>>> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>>>>> +                     struct drm_connector_state *conn_state)
>>>>> +{
>>>>> +    struct drm_writeback_job *wb_job = conn_state->writeback_job;
>>>>> +    struct drm_property_blob *pixel_format_blob;
>>>>> +    bool format_supported = false;
>>>>> +    struct drm_framebuffer *fb;
>>>>> +    int i, n_formats;
> 
> Just 'nformats'.
> 
> Please make both variables 'size_t'.

I Will correct all these minor issues.

> 
> 
>>>>> +    u32 *formats;
>>>>> +
>>>>> +    if (!wb_job || !wb_job->fb)
>>>>> +        return 0;
>>>>
>>>> I think that this should be removed and that this functions should
>>>> assume that (wb_job && wb_job->fb) == true.
>>>
>>> Ok.
> 
> In regular atomic check for planes, there can be planes with no attached
> framebuffer. Helpers handle this situation. [1] I don't know if this is
> possible in writeback code, but for consistency, it would make sense to
> keep this test here. Not sure though.

@Leandro, do you know if it is possible to have a wb_job without a fb
attached?

> 
>>>
>>>>
>>>> Actually, it's weird to have conn_state as argument and only use it to
>>>> get the wb_job. Instead, this function could receive wb_job directly.
>>>
>>> In the Thomas review of v1, he said that maybe other things could be
>>> tested in this helper. I'm not sure what these additional checks could
>>> be, so I tried to design the function signature expecting more things
>>> to be added after his review.
>>>
>>> As you can see, the helper is receiving the `drm_encoder` and doing
>>> nothing with it.
>>>
>>> If we, eventually, don't find anything else that this helper can do, I
>>> will revert to something very similar (if not equal) to your proposal.
>>> I just want to wait for Thomas's review first.
>>>
>>
>> Sure, that makes sense.
> 
> We had many helper functions for atomic modesetting that took various
> arguments for whatever they required. Extending such a function with new
> functionality/arguments required required touching many drivers and made
> the parameter list hard to read. At some point, Maxime went through most
> of the code and unified it all to pass full state > So please keep the connector state. I think it's how we do things ATM.to the helpers.
> 
> So please keep the connector state. I think it's how we do things ATM.

OK, I will keep then.

> 
>>
>> Thanks,
>> Leandro Ribeiro
>>
>>>>
>>>> Of course, its name/description would have to change.
>>>>
>>>>> +
>>>>> +    pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
>>>>> +    n_formats = pixel_format_blob->length / sizeof(u32);
>>>>> +    formats = pixel_format_blob->data;
>>>>> +    fb = wb_job->fb;
>>>>> +
>>>>> +    for (i = 0; i < n_formats; i++) {
>>>>> +        if (fb->format->format == formats[i]) {
>>>>> +            format_supported = true;
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    if (!format_supported) {
>>>>> +        DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
>>>>> +                  &fb->format->format);
> 
> Please use drm_dgb_kms() instead. There's a 100-character-per-line
> limit. The comment probably fits onto a single line.(?)


I will improve that. This code came from the vkms, which follows the 80
chars limit. If I'm not mistaken.

> 
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>
>>>> If you do this, you can get rid of the format_supported flag:
>>>>
>>>>       for(...) {
>>>>           if (fb->format->format == formats[i])
>>>>               return 0;
>>>>       }
>>>>
>>>>
>>>>       DRM_DEBUG_KMS(...);
>>>>       return -EINVAL;
>>>>
>>>
>>> Indeed. Thanks!
> 
> Yes, that looks nicer.
> 
>>>
>>>> Thanks,
>>>> Leandro Ribeiro
>>>>
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
>>>>> +
>>>>>     /**
>>>>>      * drm_atomic_helper_check_plane_state() - Check plane state for
>>>>> validity
>>>>>      * @plane_state: plane state to check
>>>>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
>>>>> b/drivers/gpu/drm/vkms/vkms_writeback.c
>>>>> index 32734cdbf6c2..42f3396c523a 100644
>>>>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
>>>>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
>>>>> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct
>>>>> drm_encoder *encoder,
>>>>>     {
>>>>>         struct drm_framebuffer *fb;
>>>>>         const struct drm_display_mode *mode = &crtc_state->mode;
>>>>> +    int ret;
>>>>>           if (!conn_state->writeback_job ||
>>>>> !conn_state->writeback_job->fb)
>>>>>             return 0;
>>>>> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct
>>>>> drm_encoder *encoder,
>>>>>             return -EINVAL;
>>>>>         }
>>>>>     -    if (fb->format->format != vkms_wb_formats[0]) {
>>>>> -        DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
>>>>> -                  &fb->format->format);
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> +    ret = drm_atomic_helper_check_wb_encoder_state(encoder,
>>>>> conn_state);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
> 
> We usually use just 'if (ret)' for such test. No need for a less-than.

I will change that.

> 
> Best regards
> Thomas
> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L809
> 
>>>>>           return 0;
>>>>>     }
>>>>> diff --git a/include/drm/drm_atomic_helper.h
>>>>> b/include/drm/drm_atomic_helper.h
>>>>> index 4045e2507e11..3fbf695da60f 100644
>>>>> --- a/include/drm/drm_atomic_helper.h
>>>>> +++ b/include/drm/drm_atomic_helper.h
>>>>> @@ -40,6 +40,9 @@ struct drm_private_state;
>>>>>       int drm_atomic_helper_check_modeset(struct drm_device *dev,
>>>>>                     struct drm_atomic_state *state);
>>>>> +int
>>>>> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>>>>> +                     struct drm_connector_state *conn_state);
>>>>>     int drm_atomic_helper_check_plane_state(struct drm_plane_state
>>>>> *plane_state,
>>>>>                         const struct drm_crtc_state *crtc_state,
>>>>>                         int min_scale,
>>>>>
>>>
>>> Thanks,
>>> ---
>>> Igor M. A. Torrente
> 

Thanks,
---
Igor M. A. Torrente