| Message ID | 20211005201637.58563-6-igormtorrente@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor the vkms to accept new formats | expand |
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@gmail.com> > --- > 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. Best regards Thomas > + > + if (!format_supported) { > DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", > &fb->format->format); > return -EINVAL; >
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@gmail.com> >> --- >> 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... > > Best regards > Thomas > >> + >> + if (!format_supported) { >> DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >> &fb->format->format); >> return -EINVAL; >> >
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@gmail.com> >>> --- >>> 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. Best regards Thomas > >> >> Best regards >> Thomas >> >>> + >>> + if (!format_supported) { >>> DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >>> &fb->format->format); >>> return -EINVAL; >>> >>
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@gmail.com> >>>> --- >>>> 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? > > 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
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@gmail.com> >>>>> --- >>>>> 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
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@gmail.com> >>>>>> --- >>>>>> 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 >
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; + } + } + + if (!format_supported) { DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", &fb->format->format); return -EINVAL;
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@gmail.com> --- drivers/gpu/drm/vkms/vkms_writeback.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)