| Message ID | 20240402232813.2670131-6-zack.rusin@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vmwgfx: vblank and crc generation support | expand |
On Tue, 2 Apr 2024 19:28:13 -0400 Zack Rusin <zack.rusin@broadcom.com> wrote: > The table of primary plane formats wasn't sorted at all, leading to > applications picking our least desirable formats by defaults. > > Sort the primary plane formats according to our order of preference. This is good. > Fixes IGT's kms_atomic plane-invalid-params which assumes that the > preferred format is a 32bpp format. That sounds strange, why would IGT depend on preferred format being 32bpp? That must be an oversight. IGT cannot dictate the format that hardware must prefer. XRGB8888 is strongly suggested to be supported in general, but why also preferred? Thanks, pq > Signed-off-by: Zack Rusin <zack.rusin@broadcom.com> > Fixes: 36cc79bc9077 ("drm/vmwgfx: Add universal plane support") > Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> > Cc: dri-devel@lists.freedesktop.org > Cc: <stable@vger.kernel.org> # v4.12+ > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h > index bf9931e3a728..bf24f2f0dcfc 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h > @@ -233,10 +233,10 @@ struct vmw_framebuffer_bo { > > > static const uint32_t __maybe_unused vmw_primary_plane_formats[] = { > - DRM_FORMAT_XRGB1555, > - DRM_FORMAT_RGB565, > DRM_FORMAT_XRGB8888, > DRM_FORMAT_ARGB8888, > + DRM_FORMAT_RGB565, > + DRM_FORMAT_XRGB1555, > }; > > static const uint32_t __maybe_unused vmw_cursor_plane_formats[] = {
On Wed, Apr 3, 2024 at 3:43 AM Pekka Paalanen <pekka.paalanen@collabora.com> wrote: > > On Tue, 2 Apr 2024 19:28:13 -0400 > Zack Rusin <zack.rusin@broadcom.com> wrote: > > > The table of primary plane formats wasn't sorted at all, leading to > > applications picking our least desirable formats by defaults. > > > > Sort the primary plane formats according to our order of preference. > > This is good. > > > Fixes IGT's kms_atomic plane-invalid-params which assumes that the > > preferred format is a 32bpp format. > > That sounds strange, why would IGT depend on preferred format being > 32bpp? > > That must be an oversight. IGT cannot dictate the format that hardware > must prefer. XRGB8888 is strongly suggested to be supported in general, > but why also preferred? I think it's just a side-effect of the pixman's assert that's failing: https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/lib/igt_fb.c#n4190 i.e. pixman assumes everything is 4 byte aligned. I should have rephrased the message as "IGT assumes that the preferred fb format is 4 byte aligned because our 16bpp formats are packed and pixman can't convert them". z
On Wed, 3 Apr 2024 07:44:54 -0400 Zack Rusin <zack.rusin@broadcom.com> wrote: > On Wed, Apr 3, 2024 at 3:43 AM Pekka Paalanen > <pekka.paalanen@collabora.com> wrote: > > > > On Tue, 2 Apr 2024 19:28:13 -0400 > > Zack Rusin <zack.rusin@broadcom.com> wrote: > > > > > The table of primary plane formats wasn't sorted at all, leading to > > > applications picking our least desirable formats by defaults. > > > > > > Sort the primary plane formats according to our order of preference. > > > > This is good. > > > > > Fixes IGT's kms_atomic plane-invalid-params which assumes that the > > > preferred format is a 32bpp format. > > > > That sounds strange, why would IGT depend on preferred format being > > 32bpp? > > > > That must be an oversight. IGT cannot dictate the format that hardware > > must prefer. XRGB8888 is strongly suggested to be supported in general, > > but why also preferred? > > I think it's just a side-effect of the pixman's assert that's failing: > https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/lib/igt_fb.c#n4190 > i.e. pixman assumes everything is 4 byte aligned. > I should have rephrased the message as "IGT assumes that the preferred > fb format is 4 byte aligned because our 16bpp formats are packed and > pixman can't convert them". Ah, yes. IIRC Pixman indeed assumes 4-byte alignment for stride and row start. It should work for 16bpp formats if the FB had even width + padding in pixels. I think this is just an indication that Pixman is ill-suited for IGT. IGT should be able to generate and analyse images in any format any kernel driver might support. I've noticed IGT also using Cairo, which limits the possible pixel formats so severely I'm actually puzzled how it can even be used there. Anyway, this is not at all about your patch, which looks good and well to me. Just the comment about adapting to IGT seemed odd. Maybe phrase that more like a happy accident rather than another justification for this patch? This patch: Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> Thanks, pq
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h index bf9931e3a728..bf24f2f0dcfc 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h @@ -233,10 +233,10 @@ struct vmw_framebuffer_bo { static const uint32_t __maybe_unused vmw_primary_plane_formats[] = { - DRM_FORMAT_XRGB1555, - DRM_FORMAT_RGB565, DRM_FORMAT_XRGB8888, DRM_FORMAT_ARGB8888, + DRM_FORMAT_RGB565, + DRM_FORMAT_XRGB1555, }; static const uint32_t __maybe_unused vmw_cursor_plane_formats[] = {
The table of primary plane formats wasn't sorted at all, leading to applications picking our least desirable formats by defaults. Sort the primary plane formats according to our order of preference. Fixes IGT's kms_atomic plane-invalid-params which assumes that the preferred format is a 32bpp format. Signed-off-by: Zack Rusin <zack.rusin@broadcom.com> Fixes: 36cc79bc9077 ("drm/vmwgfx: Add universal plane support") Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> Cc: dri-devel@lists.freedesktop.org Cc: <stable@vger.kernel.org> # v4.12+ --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)