Re: [PATCH 5/6] drm: vkms: Prepare `vkms_wb_encoder_atomic_check` to accept multiple formats

[Igor Matheus Andrade Torrente]

Hi Thomas,

On 10/19/21 4:17 AM, Thomas Zimmermann wrote:
> Hi
>
> Am 18.10.21 um 21:32 schrieb Igor Matheus Andrade Torrente:
> > Hi Thomas,
> >
> > On 10/18/21 3:06 PM, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 18.10.21 um 19:41 schrieb Igor Matheus Andrade Torrente:
> > > > Hello,
> > > >
> > > > On 10/18/21 7:14 AM, Thomas Zimmermann wrote:
> > > > > Hi
> > > > >
> > > > > Am 05.10.21 um 22:16 schrieb Igor Matheus Andrade Torrente:
> > > > > > Currently, the vkms atomic check only goes through the first
> > > > > > position of
> > > > > > the `vkms_wb_formats` vector.
> > > > > >
> > > > > > This change prepares the atomic_check to check the entire vector.
> > > > > >
> > > > > > Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@xxxxxxxxx>
> > > > > > ---
> > > > > >    drivers/gpu/drm/vkms/vkms_writeback.c | 11 ++++++++++-
> > > > > >    1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > > > b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > > > index 5a3e12f105dc..56978f499203 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > > > @@ -30,6 +30,8 @@ static int vkms_wb_encoder_atomic_check(struct
> > > > > > drm_encoder *encoder,
> > > > > >    {
> > > > > >        struct drm_framebuffer *fb;
> > > > > >        const struct drm_display_mode *mode = &crtc_state->mode;
> > > > > > +    bool format_supported = false;
> > > > > > +    int i;
> > > > > >        if (!conn_state->writeback_job ||
> > > > > > !conn_state->writeback_job->fb)
> > > > > >            return 0;
> > > > > > @@ -41,7 +43,14 @@ static int vkms_wb_encoder_atomic_check(struct
> > > > > > drm_encoder *encoder,
> > > > > >            return -EINVAL;
> > > > > >        }
> > > > > > -    if (fb->format->format != vkms_wb_formats[0]) {
> > > > > > +    for (i = 0; i < ARRAY_SIZE(vkms_wb_formats); i++) {
> > > > > > +        if (fb->format->format == vkms_wb_formats[i]) {
> > > > > > +            format_supported = true;
> > > > > > +            break;
> > > > > > +        }
> > > > > > +    }
> > > > >
> > > > > At a minimum, this loop should be in a helper function. But more
> > > > > generally, I'm surprised that this isn't already covered by the
> > > > > DRM's atomic helpers.
> > > >
> > > > Ok, I can wrap it in a new function.
> > > >
> > > > AFAIK the DRM doesn't cover it. But I may be wrong...
> > >
> > > I couldn't find anything either.
> > >
> > > Other drivers do similar format and frambuffer checks. So I guess a
> > > helper could be implemented. All plane's are supposed to call
> > > drm_atomic_helper_check_plane_state() in their atomic_check() code.
> > > You could add a similar helper, say
> > > drm_atomic_helper_check_writeback_encoder_state(), that tests for the
> > > format and maybe other things as well.
> >
> > Do you think this should be done before or after this patch series?
>
> Just add it as part of this series and use it for vkms. Other drivers
> can adopt it later on. The rcar-du code [1] looks similar to the one in
> vkms. Maybe put the common tests in to the new helper. You can extract
> the list of supported formats from the property blob, I think.

OK, Thanks!

> Best regards
> Thomas
>
> [1]
> https://elixir.bootlin.com/linux/v5.14.13/source/drivers/gpu/drm/rcar-du/rcar_du_writeback.c#L140
>
> > > Best regards
> > > Thomas
> > >
> > > > > Best regards
> > > > > Thomas
> > > > >
> > > > > > +
> > > > > > +    if (!format_supported) {
> > > > > >            DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
> > > > > >                      &fb->format->format);
> > > > > >            return -EINVAL;
> >
> > Thanks,
> > Igor Torrente