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

[Thomas Zimmermann]

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.

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

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature