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

[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 at 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
>>> erros if it
>>> + * 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;
>>> +    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.
> 
>>
>> 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.

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);
>>> +        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!
> 
>> 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;
>>>         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