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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 8112 bytes --]

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


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

>>
>>>
>>> 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 to the helpers.

So please keep the connector state. I think it's how we do things ATM.

> 
> 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.(?)

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

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]