* [PATCH] drm/doc: Document that modifiers are always required for fb
@ 2020-09-02 10:24 Daniel Vetter
2020-09-02 11:02 ` Pekka Paalanen
2020-09-02 12:40 ` Simon Ser
0 siblings, 2 replies; 19+ messages in thread
From: Daniel Vetter @ 2020-09-02 10:24 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Stone, Marek Olšák, Daniel Vetter, Juston Li,
Daniel Vetter
Even for legacy userspace, since otherwise GETFB2 is broken and if you
switch between modifier-less and modifier-aware compositors, smooth
transitions break.
Also it's just best practice to make sure modifiers are invariant for
a given drm_fb, and that a modifier-aware kms drivers only has one
place to store them, ignoring any old implicit bo flags or whatever
else might float around.
Motivated by some irc discussion with Bas about amdgpu modifier
support.
Fixes: 455e00f1412f ("drm: Add getfb2 ioctl")
Cc: Daniel Stone <daniels@collabora.com>
Cc: Juston Li <juston.li@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Cc: Marek Olšák <maraeo@gmail.com>
Cc: "Wentland, Harry" <harry.wentland@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
include/drm/drm_mode_config.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index a18f73eb3cf6..5ffbb4ed5b35 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -58,6 +58,12 @@ struct drm_mode_config_funcs {
* actual modifier used if the request doesn't have it specified,
* ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
*
+ * IMPORTANT: These implied modifiers for legacy userspace must be
+ * stored in struct &drm_framebuffer, including all relevant metadata
+ * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the
+ * modifier enables additional planes beyond the fourcc pixel format
+ * code. This is required by the GETFB2 ioctl.
+ *
* If the parameters are deemed valid and the backing storage objects in
* the underlying memory manager all exist, then the driver allocates
* a new &drm_framebuffer structure, subclassed to contain
@@ -915,6 +921,13 @@ struct drm_mode_config {
* @allow_fb_modifiers:
*
* Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
+ *
+ * IMPORTANT:
+ *
+ * If this is set the driver must fill out the full implicit modifier
+ * information in their &drm_mode_config_funcs.fb_create hook for legacy
+ * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
+ * broken for modifier aware userspace.
*/
bool allow_fb_modifiers;
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 10:24 [PATCH] drm/doc: Document that modifiers are always required for fb Daniel Vetter
@ 2020-09-02 11:02 ` Pekka Paalanen
2020-09-02 11:15 ` Daniel Vetter
2020-09-02 12:40 ` Simon Ser
1 sibling, 1 reply; 19+ messages in thread
From: Pekka Paalanen @ 2020-09-02 11:02 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
[-- Attachment #1.1: Type: text/plain, Size: 2957 bytes --]
On Wed, 2 Sep 2020 12:24:40 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Even for legacy userspace, since otherwise GETFB2 is broken and if you
> switch between modifier-less and modifier-aware compositors, smooth
> transitions break.
>
> Also it's just best practice to make sure modifiers are invariant for
> a given drm_fb, and that a modifier-aware kms drivers only has one
> place to store them, ignoring any old implicit bo flags or whatever
> else might float around.
>
> Motivated by some irc discussion with Bas about amdgpu modifier
> support.
>
> Fixes: 455e00f1412f ("drm: Add getfb2 ioctl")
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Juston Li <juston.li@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Cc: Marek Olšák <maraeo@gmail.com>
> Cc: "Wentland, Harry" <harry.wentland@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> include/drm/drm_mode_config.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index a18f73eb3cf6..5ffbb4ed5b35 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -58,6 +58,12 @@ struct drm_mode_config_funcs {
> * actual modifier used if the request doesn't have it specified,
> * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
> *
> + * IMPORTANT: These implied modifiers for legacy userspace must be
> + * stored in struct &drm_framebuffer, including all relevant metadata
> + * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the
> + * modifier enables additional planes beyond the fourcc pixel format
> + * code. This is required by the GETFB2 ioctl.
> + *
> * If the parameters are deemed valid and the backing storage objects in
> * the underlying memory manager all exist, then the driver allocates
> * a new &drm_framebuffer structure, subclassed to contain
> @@ -915,6 +921,13 @@ struct drm_mode_config {
> * @allow_fb_modifiers:
> *
> * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> + *
> + * IMPORTANT:
> + *
> + * If this is set the driver must fill out the full implicit modifier
> + * information in their &drm_mode_config_funcs.fb_create hook for legacy
> + * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
> + * broken for modifier aware userspace.
> */
> bool allow_fb_modifiers;
>
Hi,
are there any drivers that would infer this information at
modeset/pageflip/atomic ioctl time instead of AddFB/AddFB2 time?
Userspace may be creating the FB once per buffer and keep re-using
that over several render/display cycles. If a driver was changing the
"effective modifiers" dynamically, userspace could break.
Thanks,
pq
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 11:02 ` Pekka Paalanen
@ 2020-09-02 11:15 ` Daniel Vetter
2020-09-02 11:34 ` Pekka Paalanen
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2020-09-02 11:15 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
On Wed, Sep 2, 2020 at 1:02 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Wed, 2 Sep 2020 12:24:40 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > Even for legacy userspace, since otherwise GETFB2 is broken and if you
> > switch between modifier-less and modifier-aware compositors, smooth
> > transitions break.
> >
> > Also it's just best practice to make sure modifiers are invariant for
> > a given drm_fb, and that a modifier-aware kms drivers only has one
> > place to store them, ignoring any old implicit bo flags or whatever
> > else might float around.
> >
> > Motivated by some irc discussion with Bas about amdgpu modifier
> > support.
> >
> > Fixes: 455e00f1412f ("drm: Add getfb2 ioctl")
> > Cc: Daniel Stone <daniels@collabora.com>
> > Cc: Juston Li <juston.li@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Cc: Marek Olšák <maraeo@gmail.com>
> > Cc: "Wentland, Harry" <harry.wentland@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > include/drm/drm_mode_config.h | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index a18f73eb3cf6..5ffbb4ed5b35 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -58,6 +58,12 @@ struct drm_mode_config_funcs {
> > * actual modifier used if the request doesn't have it specified,
> > * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
> > *
> > + * IMPORTANT: These implied modifiers for legacy userspace must be
> > + * stored in struct &drm_framebuffer, including all relevant metadata
> > + * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the
> > + * modifier enables additional planes beyond the fourcc pixel format
> > + * code. This is required by the GETFB2 ioctl.
> > + *
> > * If the parameters are deemed valid and the backing storage objects in
> > * the underlying memory manager all exist, then the driver allocates
> > * a new &drm_framebuffer structure, subclassed to contain
> > @@ -915,6 +921,13 @@ struct drm_mode_config {
> > * @allow_fb_modifiers:
> > *
> > * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> > + *
> > + * IMPORTANT:
> > + *
> > + * If this is set the driver must fill out the full implicit modifier
> > + * information in their &drm_mode_config_funcs.fb_create hook for legacy
> > + * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
> > + * broken for modifier aware userspace.
> > */
> > bool allow_fb_modifiers;
> >
>
> Hi,
>
> are there any drivers that would infer this information at
> modeset/pageflip/atomic ioctl time instead of AddFB/AddFB2 time?
Currently no, the only driver that infers anything for legacy is i915.
Proposed amdgpu modifier patches don't work like that, but I think Bas
is working on adding the modifier inference at addfb time for legacy
userspace.
> Userspace may be creating the FB once per buffer and keep re-using
> that over several render/display cycles. If a driver was changing the
> "effective modifiers" dynamically, userspace could break.
Yeah this is why I want to lock this all down, since effective
modifiers that change for a drm_fb object which is supposed to have
all invariant metadata just isn't great uapi.
-Daniel
>
>
> Thanks,
> pq
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 11:15 ` Daniel Vetter
@ 2020-09-02 11:34 ` Pekka Paalanen
0 siblings, 0 replies; 19+ messages in thread
From: Pekka Paalanen @ 2020-09-02 11:34 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
[-- Attachment #1.1: Type: text/plain, Size: 3998 bytes --]
On Wed, 2 Sep 2020 13:15:49 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Wed, Sep 2, 2020 at 1:02 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Wed, 2 Sep 2020 12:24:40 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > > Even for legacy userspace, since otherwise GETFB2 is broken and if you
> > > switch between modifier-less and modifier-aware compositors, smooth
> > > transitions break.
> > >
> > > Also it's just best practice to make sure modifiers are invariant for
> > > a given drm_fb, and that a modifier-aware kms drivers only has one
> > > place to store them, ignoring any old implicit bo flags or whatever
> > > else might float around.
> > >
> > > Motivated by some irc discussion with Bas about amdgpu modifier
> > > support.
> > >
> > > Fixes: 455e00f1412f ("drm: Add getfb2 ioctl")
> > > Cc: Daniel Stone <daniels@collabora.com>
> > > Cc: Juston Li <juston.li@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > Cc: Marek Olšák <maraeo@gmail.com>
> > > Cc: "Wentland, Harry" <harry.wentland@amd.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > > include/drm/drm_mode_config.h | 13 +++++++++++++
> > > 1 file changed, 13 insertions(+)
> > >
> > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > index a18f73eb3cf6..5ffbb4ed5b35 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -58,6 +58,12 @@ struct drm_mode_config_funcs {
> > > * actual modifier used if the request doesn't have it specified,
> > > * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
> > > *
> > > + * IMPORTANT: These implied modifiers for legacy userspace must be
> > > + * stored in struct &drm_framebuffer, including all relevant metadata
> > > + * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the
> > > + * modifier enables additional planes beyond the fourcc pixel format
> > > + * code. This is required by the GETFB2 ioctl.
> > > + *
> > > * If the parameters are deemed valid and the backing storage objects in
> > > * the underlying memory manager all exist, then the driver allocates
> > > * a new &drm_framebuffer structure, subclassed to contain
> > > @@ -915,6 +921,13 @@ struct drm_mode_config {
> > > * @allow_fb_modifiers:
> > > *
> > > * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> > > + *
> > > + * IMPORTANT:
> > > + *
> > > + * If this is set the driver must fill out the full implicit modifier
> > > + * information in their &drm_mode_config_funcs.fb_create hook for legacy
> > > + * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
> > > + * broken for modifier aware userspace.
> > > */
> > > bool allow_fb_modifiers;
> > >
> >
> > Hi,
> >
> > are there any drivers that would infer this information at
> > modeset/pageflip/atomic ioctl time instead of AddFB/AddFB2 time?
>
> Currently no, the only driver that infers anything for legacy is i915.
> Proposed amdgpu modifier patches don't work like that, but I think Bas
> is working on adding the modifier inference at addfb time for legacy
> userspace.
>
> > Userspace may be creating the FB once per buffer and keep re-using
> > that over several render/display cycles. If a driver was changing the
> > "effective modifiers" dynamically, userspace could break.
>
> Yeah this is why I want to lock this all down, since effective
> modifiers that change for a drm_fb object which is supposed to have
> all invariant metadata just isn't great uapi.
Sounds good to me!
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Thanks,
pq
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 10:24 [PATCH] drm/doc: Document that modifiers are always required for fb Daniel Vetter
2020-09-02 11:02 ` Pekka Paalanen
@ 2020-09-02 12:40 ` Simon Ser
2020-09-02 12:44 ` Daniel Vetter
1 sibling, 1 reply; 19+ messages in thread
From: Simon Ser @ 2020-09-02 12:40 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
Looks fine to me!
Acked-by: Simon Ser <contact@emersion.fr>
I suppose something similar happens in user-space: gbm_bo_create
without modifiers needs to properly set the implicit modifier, ie.
gbm_bo_get_modifier needs to return the effective modifier. Is this
something already documented?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 12:40 ` Simon Ser
@ 2020-09-02 12:44 ` Daniel Vetter
2020-09-02 12:49 ` Simon Ser
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2020-09-02 12:44 UTC (permalink / raw)
To: Simon Ser
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
On Wed, Sep 2, 2020 at 2:40 PM Simon Ser <contact@emersion.fr> wrote:
>
> Looks fine to me!
>
> Acked-by: Simon Ser <contact@emersion.fr>
>
> I suppose something similar happens in user-space: gbm_bo_create
> without modifiers needs to properly set the implicit modifier, ie.
> gbm_bo_get_modifier needs to return the effective modifier. Is this
> something already documented?
I don't think that happens, but it has come up in discussions. It's
kinda different scenario though: getfb2 is for cross-compositor stuff,
enabling smooth transitions at boot-up and when switching. So you have
a legit reason for mixing modifier-aware userspace with
non-modifier-aware userspace. And the modifier-aware userspace would
like that everything works with modifiers consistently, including
getfb2. But gbm is just within a single process, and that should
either run all with modifiers, or not at all, since these worlds just
dont mix well. Hence I'm not seeing much use for that, -modesetting
being a confused mess nonwithstanding :-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 12:44 ` Daniel Vetter
@ 2020-09-02 12:49 ` Simon Ser
2020-09-02 12:56 ` Daniel Stone
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Simon Ser @ 2020-09-02 12:49 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > I suppose something similar happens in user-space: gbm_bo_create
> > without modifiers needs to properly set the implicit modifier, ie.
> > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > something already documented?
>
> I don't think that happens, but it has come up in discussions. It's
> kinda different scenario though: getfb2 is for cross-compositor stuff,
> enabling smooth transitions at boot-up and when switching. So you have
> a legit reason for mixing modifier-aware userspace with
> non-modifier-aware userspace. And the modifier-aware userspace would
> like that everything works with modifiers consistently, including
> getfb2. But gbm is just within a single process, and that should
> either run all with modifiers, or not at all, since these worlds just
> dont mix well. Hence I'm not seeing much use for that, -modesetting
> being a confused mess nonwithstanding :-)
Well… There's also the case where some legacy Wayland client talks to a
modifier-aware compositor. gbm_bo_import would be called without a
modifier, but the compositor expects gbm_bo_get_modifier to work.
Also, wlroots will call gbm_bo_create without a modifier to only let
the driver pick "safe" modifiers in case passing the full list of
modifiers results in a black screen. Later on wlroots will call
gbm_bo_get_modifier to figure out what modifier the driver picked.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 12:49 ` Simon Ser
@ 2020-09-02 12:56 ` Daniel Stone
2020-09-02 14:29 ` Bas Nieuwenhuizen
2020-09-02 14:29 ` Daniel Vetter
2 siblings, 0 replies; 19+ messages in thread
From: Daniel Stone @ 2020-09-02 12:56 UTC (permalink / raw)
To: Simon Ser
Cc: Daniel Stone, Marek Olšák, Daniel Vetter,
DRI Development, Daniel Vetter, Juston Li
On Wed, 2 Sep 2020 at 13:49, Simon Ser <contact@emersion.fr> wrote:
> On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > I don't think that happens, but it has come up in discussions. It's
> > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > enabling smooth transitions at boot-up and when switching. So you have
> > a legit reason for mixing modifier-aware userspace with
> > non-modifier-aware userspace. And the modifier-aware userspace would
> > like that everything works with modifiers consistently, including
> > getfb2. But gbm is just within a single process, and that should
> > either run all with modifiers, or not at all, since these worlds just
> > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > being a confused mess nonwithstanding :-)
>
> Well… There's also the case where some legacy Wayland client talks to a
> modifier-aware compositor. gbm_bo_import would be called without a
> modifier, but the compositor expects gbm_bo_get_modifier to work.
>
> Also, wlroots will call gbm_bo_create without a modifier to only let
> the driver pick "safe" modifiers in case passing the full list of
> modifiers results in a black screen. Later on wlroots will call
> gbm_bo_get_modifier to figure out what modifier the driver picked.
I think those are reasonable expectations to have, even though
arguably for consistency we should always have a fixed INVALID for
when a modifier isn't used.
Anyway, patch is:
Acked-by: Daniel Stone <daniels@collabora.com>
Thanks for typing this up!
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 12:49 ` Simon Ser
2020-09-02 12:56 ` Daniel Stone
@ 2020-09-02 14:29 ` Bas Nieuwenhuizen
2020-09-02 14:29 ` Daniel Vetter
2 siblings, 0 replies; 19+ messages in thread
From: Bas Nieuwenhuizen @ 2020-09-02 14:29 UTC (permalink / raw)
To: Simon Ser
Cc: Daniel Stone, Marek Olšák, Daniel Vetter,
DRI Development, Daniel Vetter, Juston Li
On Wed, Sep 2, 2020 at 2:49 PM Simon Ser <contact@emersion.fr> wrote:
>
> On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > > I suppose something similar happens in user-space: gbm_bo_create
> > > without modifiers needs to properly set the implicit modifier, ie.
> > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > something already documented?
> >
> > I don't think that happens, but it has come up in discussions. It's
> > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > enabling smooth transitions at boot-up and when switching. So you have
> > a legit reason for mixing modifier-aware userspace with
> > non-modifier-aware userspace. And the modifier-aware userspace would
> > like that everything works with modifiers consistently, including
> > getfb2. But gbm is just within a single process, and that should
> > either run all with modifiers, or not at all, since these worlds just
> > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > being a confused mess nonwithstanding :-)
>
> Well… There's also the case where some legacy Wayland client talks to a
> modifier-aware compositor. gbm_bo_import would be called without a
> modifier, but the compositor expects gbm_bo_get_modifier to work.
>
> Also, wlroots will call gbm_bo_create without a modifier to only let
> the driver pick "safe" modifiers in case passing the full list of
> modifiers results in a black screen. Later on wlroots will call
> gbm_bo_get_modifier to figure out what modifier the driver picked.
I don't think we can do that generally, as pre-modifier clients are
typically also not capable of handling memory planes > format planes,
which will happen with compression (but used out of band communication
for the second plane on AMD in the non-modifier case).
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 12:49 ` Simon Ser
2020-09-02 12:56 ` Daniel Stone
2020-09-02 14:29 ` Bas Nieuwenhuizen
@ 2020-09-02 14:29 ` Daniel Vetter
2020-09-02 14:59 ` Simon Ser
2 siblings, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2020-09-02 14:29 UTC (permalink / raw)
To: Simon Ser
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
On Wed, Sep 2, 2020 at 2:49 PM Simon Ser <contact@emersion.fr> wrote:
>
> On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > > I suppose something similar happens in user-space: gbm_bo_create
> > > without modifiers needs to properly set the implicit modifier, ie.
> > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > something already documented?
> >
> > I don't think that happens, but it has come up in discussions. It's
> > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > enabling smooth transitions at boot-up and when switching. So you have
> > a legit reason for mixing modifier-aware userspace with
> > non-modifier-aware userspace. And the modifier-aware userspace would
> > like that everything works with modifiers consistently, including
> > getfb2. But gbm is just within a single process, and that should
> > either run all with modifiers, or not at all, since these worlds just
> > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > being a confused mess nonwithstanding :-)
>
> Well… There's also the case where some legacy Wayland client talks to a
> modifier-aware compositor. gbm_bo_import would be called without a
> modifier, but the compositor expects gbm_bo_get_modifier to work.
>
> Also, wlroots will call gbm_bo_create without a modifier to only let
> the driver pick "safe" modifiers in case passing the full list of
> modifiers results in a black screen. Later on wlroots will call
> gbm_bo_get_modifier to figure out what modifier the driver picked.
gbm_bo_import is a different thing from gbm_bo_create. Former I agree
should figure out the right modifiers (and I think it does that, at
least on intel mesa). For gbm_bo_create I'm not sure we should/need to
require that.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 14:29 ` Daniel Vetter
@ 2020-09-02 14:59 ` Simon Ser
2020-09-07 8:31 ` Daniel Vetter
0 siblings, 1 reply; 19+ messages in thread
From: Simon Ser @ 2020-09-02 14:59 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Marek Olšák, Juston Li, Daniel Stone,
DRI Development
On Wednesday, September 2, 2020 4:29 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Wed, Sep 2, 2020 at 2:49 PM Simon Ser contact@emersion.fr wrote:
>
> > On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> >
> > > > I suppose something similar happens in user-space: gbm_bo_create
> > > > without modifiers needs to properly set the implicit modifier, ie.
> > > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > > something already documented?
> > >
> > > I don't think that happens, but it has come up in discussions. It's
> > > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > > enabling smooth transitions at boot-up and when switching. So you have
> > > a legit reason for mixing modifier-aware userspace with
> > > non-modifier-aware userspace. And the modifier-aware userspace would
> > > like that everything works with modifiers consistently, including
> > > getfb2. But gbm is just within a single process, and that should
> > > either run all with modifiers, or not at all, since these worlds just
> > > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > > being a confused mess nonwithstanding :-)
> >
> > Well… There's also the case where some legacy Wayland client talks to a
> > modifier-aware compositor. gbm_bo_import would be called without a
> > modifier, but the compositor expects gbm_bo_get_modifier to work.
> > Also, wlroots will call gbm_bo_create without a modifier to only let
> > the driver pick "safe" modifiers in case passing the full list of
> > modifiers results in a black screen. Later on wlroots will call
> > gbm_bo_get_modifier to figure out what modifier the driver picked.
>
> gbm_bo_import is a different thing from gbm_bo_create. Former I agree
> should figure out the right modifiers (and I think it does that, at
> least on intel mesa). For gbm_bo_create I'm not sure we should/need to
> require that.
I guess the compositor will just forward the value returned by
gbm_bo_get_modifier in any case, so returning INVALID would be fine
too (to mean "implicit modifier").
In both the create and import cases, other metadata like pitches and
offsets should be correctly set I think?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-02 14:59 ` Simon Ser
@ 2020-09-07 8:31 ` Daniel Vetter
2020-09-07 8:37 ` Simon Ser
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2020-09-07 8:31 UTC (permalink / raw)
To: Simon Ser
Cc: Daniel Stone, Marek Olšák, Daniel Vetter,
DRI Development, Daniel Vetter, Juston Li
On Wed, Sep 02, 2020 at 02:59:49PM +0000, Simon Ser wrote:
> On Wednesday, September 2, 2020 4:29 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > On Wed, Sep 2, 2020 at 2:49 PM Simon Ser contact@emersion.fr wrote:
> >
> > > On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > >
> > > > > I suppose something similar happens in user-space: gbm_bo_create
> > > > > without modifiers needs to properly set the implicit modifier, ie.
> > > > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > > > something already documented?
> > > >
> > > > I don't think that happens, but it has come up in discussions. It's
> > > > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > > > enabling smooth transitions at boot-up and when switching. So you have
> > > > a legit reason for mixing modifier-aware userspace with
> > > > non-modifier-aware userspace. And the modifier-aware userspace would
> > > > like that everything works with modifiers consistently, including
> > > > getfb2. But gbm is just within a single process, and that should
> > > > either run all with modifiers, or not at all, since these worlds just
> > > > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > > > being a confused mess nonwithstanding :-)
> > >
> > > Well… There's also the case where some legacy Wayland client talks to a
> > > modifier-aware compositor. gbm_bo_import would be called without a
> > > modifier, but the compositor expects gbm_bo_get_modifier to work.
> > > Also, wlroots will call gbm_bo_create without a modifier to only let
> > > the driver pick "safe" modifiers in case passing the full list of
> > > modifiers results in a black screen. Later on wlroots will call
> > > gbm_bo_get_modifier to figure out what modifier the driver picked.
> >
> > gbm_bo_import is a different thing from gbm_bo_create. Former I agree
> > should figure out the right modifiers (and I think it does that, at
> > least on intel mesa). For gbm_bo_create I'm not sure we should/need to
> > require that.
>
> I guess the compositor will just forward the value returned by
> gbm_bo_get_modifier in any case, so returning INVALID would be fine
> too (to mean "implicit modifier").
>
> In both the create and import cases, other metadata like pitches and
> offsets should be correctly set I think?
Well if you have a modifier format underneath, the non-modifiered offsets
and pitches might be pure fiction. Also, they might not be sufficient, if
the modifier adds more planes.
So I'm not sure how we can let the "implicit modifier" go through once a
stack is converted to support modifiers. In a way modifiers are one-way
compatible only: implicit modifiers -> explicit modifiers should be
well-defined, the other way just looses information and doesn't work.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-07 8:31 ` Daniel Vetter
@ 2020-09-07 8:37 ` Simon Ser
2020-09-07 8:41 ` Daniel Vetter
0 siblings, 1 reply; 19+ messages in thread
From: Simon Ser @ 2020-09-07 8:37 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Stone, Marek Olšák, Daniel Vetter,
DRI Development, Daniel Vetter, Juston Li
On Monday, September 7, 2020 10:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 02, 2020 at 02:59:49PM +0000, Simon Ser wrote:
>
> > On Wednesday, September 2, 2020 4:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> >
> > > On Wed, Sep 2, 2020 at 2:49 PM Simon Ser contact@emersion.fr wrote:
> > >
> > > > On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > > >
> > > > > > I suppose something similar happens in user-space: gbm_bo_create
> > > > > > without modifiers needs to properly set the implicit modifier, ie.
> > > > > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > > > > something already documented?
> > > > >
> > > > > I don't think that happens, but it has come up in discussions. It's
> > > > > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > > > > enabling smooth transitions at boot-up and when switching. So you have
> > > > > a legit reason for mixing modifier-aware userspace with
> > > > > non-modifier-aware userspace. And the modifier-aware userspace would
> > > > > like that everything works with modifiers consistently, including
> > > > > getfb2. But gbm is just within a single process, and that should
> > > > > either run all with modifiers, or not at all, since these worlds just
> > > > > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > > > > being a confused mess nonwithstanding :-)
> > > >
> > > > Well… There's also the case where some legacy Wayland client talks to a
> > > > modifier-aware compositor. gbm_bo_import would be called without a
> > > > modifier, but the compositor expects gbm_bo_get_modifier to work.
> > > > Also, wlroots will call gbm_bo_create without a modifier to only let
> > > > the driver pick "safe" modifiers in case passing the full list of
> > > > modifiers results in a black screen. Later on wlroots will call
> > > > gbm_bo_get_modifier to figure out what modifier the driver picked.
> > >
> > > gbm_bo_import is a different thing from gbm_bo_create. Former I agree
> > > should figure out the right modifiers (and I think it does that, at
> > > least on intel mesa). For gbm_bo_create I'm not sure we should/need to
> > > require that.
> >
> > I guess the compositor will just forward the value returned by
> > gbm_bo_get_modifier in any case, so returning INVALID would be fine
> > too (to mean "implicit modifier").
> > In both the create and import cases, other metadata like pitches and
> > offsets should be correctly set I think?
>
> Well if you have a modifier format underneath, the non-modifiered offsets
> and pitches might be pure fiction. Also, they might not be sufficient, if
> the modifier adds more planes.
In this case (gbm_bo_create without modifiers), we're discussing
whether we require gbm_bo_get_modifier to return a valid modifier, or
if INVALID is fine.
> So I'm not sure how we can let the "implicit modifier" go through once a
> stack is converted to support modifiers. In a way modifiers are one-way
> compatible only: implicit modifiers -> explicit modifiers should be
> well-defined, the other way just looses information and doesn't work.
That makes sense to me, and still works fine with the two use-cases
outlined above.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-07 8:37 ` Simon Ser
@ 2020-09-07 8:41 ` Daniel Vetter
2020-09-07 9:07 ` Pekka Paalanen
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2020-09-07 8:41 UTC (permalink / raw)
To: Simon Ser
Cc: Daniel Stone, Marek Olšák, Daniel Vetter,
DRI Development, Daniel Vetter, Juston Li
On Mon, Sep 07, 2020 at 08:37:31AM +0000, Simon Ser wrote:
> On Monday, September 7, 2020 10:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Wed, Sep 02, 2020 at 02:59:49PM +0000, Simon Ser wrote:
> >
> > > On Wednesday, September 2, 2020 4:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > >
> > > > On Wed, Sep 2, 2020 at 2:49 PM Simon Ser contact@emersion.fr wrote:
> > > >
> > > > > On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > > > >
> > > > > > > I suppose something similar happens in user-space: gbm_bo_create
> > > > > > > without modifiers needs to properly set the implicit modifier, ie.
> > > > > > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > > > > > something already documented?
> > > > > >
> > > > > > I don't think that happens, but it has come up in discussions. It's
> > > > > > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > > > > > enabling smooth transitions at boot-up and when switching. So you have
> > > > > > a legit reason for mixing modifier-aware userspace with
> > > > > > non-modifier-aware userspace. And the modifier-aware userspace would
> > > > > > like that everything works with modifiers consistently, including
> > > > > > getfb2. But gbm is just within a single process, and that should
> > > > > > either run all with modifiers, or not at all, since these worlds just
> > > > > > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > > > > > being a confused mess nonwithstanding :-)
> > > > >
> > > > > Well… There's also the case where some legacy Wayland client talks to a
> > > > > modifier-aware compositor. gbm_bo_import would be called without a
> > > > > modifier, but the compositor expects gbm_bo_get_modifier to work.
> > > > > Also, wlroots will call gbm_bo_create without a modifier to only let
> > > > > the driver pick "safe" modifiers in case passing the full list of
> > > > > modifiers results in a black screen. Later on wlroots will call
> > > > > gbm_bo_get_modifier to figure out what modifier the driver picked.
> > > >
> > > > gbm_bo_import is a different thing from gbm_bo_create. Former I agree
> > > > should figure out the right modifiers (and I think it does that, at
> > > > least on intel mesa). For gbm_bo_create I'm not sure we should/need to
> > > > require that.
> > >
> > > I guess the compositor will just forward the value returned by
> > > gbm_bo_get_modifier in any case, so returning INVALID would be fine
> > > too (to mean "implicit modifier").
> > > In both the create and import cases, other metadata like pitches and
> > > offsets should be correctly set I think?
> >
> > Well if you have a modifier format underneath, the non-modifiered offsets
> > and pitches might be pure fiction. Also, they might not be sufficient, if
> > the modifier adds more planes.
>
> In this case (gbm_bo_create without modifiers), we're discussing
> whether we require gbm_bo_get_modifier to return a valid modifier, or
> if INVALID is fine.
Hm then I missed the use-case for a gbm_bo_create without modifiers, where
afterwards userspace wants the modifiers. That sounds like a bug (and yes
-modesetting is buggy that way).
-Daniel
> > So I'm not sure how we can let the "implicit modifier" go through once a
> > stack is converted to support modifiers. In a way modifiers are one-way
> > compatible only: implicit modifiers -> explicit modifiers should be
> > well-defined, the other way just looses information and doesn't work.
>
> That makes sense to me, and still works fine with the two use-cases
> outlined above.
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-07 8:41 ` Daniel Vetter
@ 2020-09-07 9:07 ` Pekka Paalanen
2020-09-07 13:51 ` Daniel Vetter
0 siblings, 1 reply; 19+ messages in thread
From: Pekka Paalanen @ 2020-09-07 9:07 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Stone, Marek Olšák, DRI Development,
Daniel Vetter, Daniel Vetter, Juston Li
[-- Attachment #1.1: Type: text/plain, Size: 4136 bytes --]
On Mon, 7 Sep 2020 10:41:37 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 07, 2020 at 08:37:31AM +0000, Simon Ser wrote:
> > On Monday, September 7, 2020 10:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Wed, Sep 02, 2020 at 02:59:49PM +0000, Simon Ser wrote:
> > >
> > > > On Wednesday, September 2, 2020 4:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > > >
> > > > > On Wed, Sep 2, 2020 at 2:49 PM Simon Ser contact@emersion.fr wrote:
> > > > >
> > > > > > On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > > > > >
> > > > > > > > I suppose something similar happens in user-space: gbm_bo_create
> > > > > > > > without modifiers needs to properly set the implicit modifier, ie.
> > > > > > > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > > > > > > something already documented?
> > > > > > >
> > > > > > > I don't think that happens, but it has come up in discussions. It's
> > > > > > > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > > > > > > enabling smooth transitions at boot-up and when switching. So you have
> > > > > > > a legit reason for mixing modifier-aware userspace with
> > > > > > > non-modifier-aware userspace. And the modifier-aware userspace would
> > > > > > > like that everything works with modifiers consistently, including
> > > > > > > getfb2. But gbm is just within a single process, and that should
> > > > > > > either run all with modifiers, or not at all, since these worlds just
> > > > > > > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > > > > > > being a confused mess nonwithstanding :-)
> > > > > >
> > > > > > Well… There's also the case where some legacy Wayland client talks to a
> > > > > > modifier-aware compositor. gbm_bo_import would be called without a
> > > > > > modifier, but the compositor expects gbm_bo_get_modifier to work.
> > > > > > Also, wlroots will call gbm_bo_create without a modifier to only let
> > > > > > the driver pick "safe" modifiers in case passing the full list of
> > > > > > modifiers results in a black screen. Later on wlroots will call
> > > > > > gbm_bo_get_modifier to figure out what modifier the driver picked.
> > > > >
> > > > > gbm_bo_import is a different thing from gbm_bo_create. Former I agree
> > > > > should figure out the right modifiers (and I think it does that, at
> > > > > least on intel mesa). For gbm_bo_create I'm not sure we should/need to
> > > > > require that.
> > > >
> > > > I guess the compositor will just forward the value returned by
> > > > gbm_bo_get_modifier in any case, so returning INVALID would be fine
> > > > too (to mean "implicit modifier").
> > > > In both the create and import cases, other metadata like pitches and
> > > > offsets should be correctly set I think?
> > >
> > > Well if you have a modifier format underneath, the non-modifiered offsets
> > > and pitches might be pure fiction. Also, they might not be sufficient, if
> > > the modifier adds more planes.
> >
> > In this case (gbm_bo_create without modifiers), we're discussing
> > whether we require gbm_bo_get_modifier to return a valid modifier, or
> > if INVALID is fine.
>
> Hm then I missed the use-case for a gbm_bo_create without modifiers, where
> afterwards userspace wants the modifiers. That sounds like a bug (and yes
> -modesetting is buggy that way).
I'm guessing that use case might be related to
https://gitlab.freedesktop.org/wayland/weston/-/issues/429
The weston issue is about
gbm_surface_create/gbm_surface_create_with_modifiers, but that's not
too different from gbm_bo_create/gbm_bo_create_with_modifiers?
Weston happens to have this code:
https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0.0/libweston/backend-drm/drm-gbm.c#L209-230
and then it unconditionally calls gbm_bo_get_modifier(). However,
DRM_FORMAT_MOD_INVALID is handled specially:
https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0.0/libweston/backend-drm/fb.c#L80-97
Thanks,
pq
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-07 9:07 ` Pekka Paalanen
@ 2020-09-07 13:51 ` Daniel Vetter
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2020-09-07 13:51 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Daniel Stone, Marek Olšák, DRI Development,
Daniel Vetter, Juston Li
On Mon, Sep 7, 2020 at 11:07 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Mon, 7 Sep 2020 10:41:37 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Mon, Sep 07, 2020 at 08:37:31AM +0000, Simon Ser wrote:
> > > On Monday, September 7, 2020 10:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > On Wed, Sep 02, 2020 at 02:59:49PM +0000, Simon Ser wrote:
> > > >
> > > > > On Wednesday, September 2, 2020 4:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > > > >
> > > > > > On Wed, Sep 2, 2020 at 2:49 PM Simon Ser contact@emersion.fr wrote:
> > > > > >
> > > > > > > On Wednesday, September 2, 2020 2:44 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
> > > > > > >
> > > > > > > > > I suppose something similar happens in user-space: gbm_bo_create
> > > > > > > > > without modifiers needs to properly set the implicit modifier, ie.
> > > > > > > > > gbm_bo_get_modifier needs to return the effective modifier. Is this
> > > > > > > > > something already documented?
> > > > > > > >
> > > > > > > > I don't think that happens, but it has come up in discussions. It's
> > > > > > > > kinda different scenario though: getfb2 is for cross-compositor stuff,
> > > > > > > > enabling smooth transitions at boot-up and when switching. So you have
> > > > > > > > a legit reason for mixing modifier-aware userspace with
> > > > > > > > non-modifier-aware userspace. And the modifier-aware userspace would
> > > > > > > > like that everything works with modifiers consistently, including
> > > > > > > > getfb2. But gbm is just within a single process, and that should
> > > > > > > > either run all with modifiers, or not at all, since these worlds just
> > > > > > > > dont mix well. Hence I'm not seeing much use for that, -modesetting
> > > > > > > > being a confused mess nonwithstanding :-)
> > > > > > >
> > > > > > > Well… There's also the case where some legacy Wayland client talks to a
> > > > > > > modifier-aware compositor. gbm_bo_import would be called without a
> > > > > > > modifier, but the compositor expects gbm_bo_get_modifier to work.
> > > > > > > Also, wlroots will call gbm_bo_create without a modifier to only let
> > > > > > > the driver pick "safe" modifiers in case passing the full list of
> > > > > > > modifiers results in a black screen. Later on wlroots will call
> > > > > > > gbm_bo_get_modifier to figure out what modifier the driver picked.
> > > > > >
> > > > > > gbm_bo_import is a different thing from gbm_bo_create. Former I agree
> > > > > > should figure out the right modifiers (and I think it does that, at
> > > > > > least on intel mesa). For gbm_bo_create I'm not sure we should/need to
> > > > > > require that.
> > > > >
> > > > > I guess the compositor will just forward the value returned by
> > > > > gbm_bo_get_modifier in any case, so returning INVALID would be fine
> > > > > too (to mean "implicit modifier").
> > > > > In both the create and import cases, other metadata like pitches and
> > > > > offsets should be correctly set I think?
> > > >
> > > > Well if you have a modifier format underneath, the non-modifiered offsets
> > > > and pitches might be pure fiction. Also, they might not be sufficient, if
> > > > the modifier adds more planes.
> > >
> > > In this case (gbm_bo_create without modifiers), we're discussing
> > > whether we require gbm_bo_get_modifier to return a valid modifier, or
> > > if INVALID is fine.
> >
> > Hm then I missed the use-case for a gbm_bo_create without modifiers, where
> > afterwards userspace wants the modifiers. That sounds like a bug (and yes
> > -modesetting is buggy that way).
>
> I'm guessing that use case might be related to
> https://gitlab.freedesktop.org/wayland/weston/-/issues/429
>
> The weston issue is about
> gbm_surface_create/gbm_surface_create_with_modifiers, but that's not
> too different from gbm_bo_create/gbm_bo_create_with_modifiers?
>
> Weston happens to have this code:
> https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0.0/libweston/backend-drm/drm-gbm.c#L209-230
> and then it unconditionally calls gbm_bo_get_modifier(). However,
> DRM_FORMAT_MOD_INVALID is handled specially:
> https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0.0/libweston/backend-drm/fb.c#L80-97
Hm yeah that feels a bit like an interim hack instead of more
modifiers fallback logic. I guess shouldn't be too tricky for mesa to
support that, since internally modifier aware drivers work only with
modifiers anyway (or at least should, that's what we're requiring on
the kms side with this patch at least).
Up to mesa people I'd say.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] drm/doc: Document that modifiers are always required for fb
@ 2020-09-17 16:47 Daniel Vetter
2020-09-17 17:24 ` Bas Nieuwenhuizen
0 siblings, 1 reply; 19+ messages in thread
From: Daniel Vetter @ 2020-09-17 16:47 UTC (permalink / raw)
To: DRI Development
Cc: Pekka Paalanen, Daniel Stone, Marek Olšák,
Daniel Vetter, Juston Li, Daniel Vetter
Even for legacy userspace, since otherwise GETFB2 is broken and if you
switch between modifier-less and modifier-aware compositors, smooth
transitions break.
Also it's just best practice to make sure modifiers are invariant for
a given drm_fb, and that a modifier-aware kms drivers only has one
place to store them, ignoring any old implicit bo flags or whatever
else might float around.
Motivated by some irc discussion with Bas about amdgpu modifier
support.
Acked-by: Simon Ser <contact@emersion.fr>
Acked-by: Daniel Stone <daniels@collabora.com>
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Fixes: 455e00f1412f ("drm: Add getfb2 ioctl")
Cc: Daniel Stone <daniels@collabora.com>
Cc: Juston Li <juston.li@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Cc: Marek Olšák <maraeo@gmail.com>
Cc: "Wentland, Harry" <harry.wentland@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
Sending this out again to double-check there's no objections or concerns.
From what I understand looking at the code the latest modifier patches for
amdgpu follow this now.
Cheers, Daniel
---
include/drm/drm_mode_config.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index a18f73eb3cf6..5ffbb4ed5b35 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -58,6 +58,12 @@ struct drm_mode_config_funcs {
* actual modifier used if the request doesn't have it specified,
* ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
*
+ * IMPORTANT: These implied modifiers for legacy userspace must be
+ * stored in struct &drm_framebuffer, including all relevant metadata
+ * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the
+ * modifier enables additional planes beyond the fourcc pixel format
+ * code. This is required by the GETFB2 ioctl.
+ *
* If the parameters are deemed valid and the backing storage objects in
* the underlying memory manager all exist, then the driver allocates
* a new &drm_framebuffer structure, subclassed to contain
@@ -915,6 +921,13 @@ struct drm_mode_config {
* @allow_fb_modifiers:
*
* Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
+ *
+ * IMPORTANT:
+ *
+ * If this is set the driver must fill out the full implicit modifier
+ * information in their &drm_mode_config_funcs.fb_create hook for legacy
+ * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
+ * broken for modifier aware userspace.
*/
bool allow_fb_modifiers;
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-17 16:47 Daniel Vetter
@ 2020-09-17 17:24 ` Bas Nieuwenhuizen
2020-09-23 15:53 ` Daniel Vetter
0 siblings, 1 reply; 19+ messages in thread
From: Bas Nieuwenhuizen @ 2020-09-17 17:24 UTC (permalink / raw)
To: Daniel Vetter
Cc: Pekka Paalanen, Daniel Stone, Marek Olšák,
DRI Development, Juston Li, Daniel Vetter
Acked-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
On Thu, Sep 17, 2020 at 6:48 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> Even for legacy userspace, since otherwise GETFB2 is broken and if you
> switch between modifier-less and modifier-aware compositors, smooth
> transitions break.
>
> Also it's just best practice to make sure modifiers are invariant for
> a given drm_fb, and that a modifier-aware kms drivers only has one
> place to store them, ignoring any old implicit bo flags or whatever
> else might float around.
>
> Motivated by some irc discussion with Bas about amdgpu modifier
> support.
>
> Acked-by: Simon Ser <contact@emersion.fr>
> Acked-by: Daniel Stone <daniels@collabora.com>
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Fixes: 455e00f1412f ("drm: Add getfb2 ioctl")
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Juston Li <juston.li@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Cc: Marek Olšák <maraeo@gmail.com>
> Cc: "Wentland, Harry" <harry.wentland@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> Sending this out again to double-check there's no objections or concerns.
> From what I understand looking at the code the latest modifier patches for
> amdgpu follow this now.
>
> Cheers, Daniel
> ---
> include/drm/drm_mode_config.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index a18f73eb3cf6..5ffbb4ed5b35 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -58,6 +58,12 @@ struct drm_mode_config_funcs {
> * actual modifier used if the request doesn't have it specified,
> * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
> *
> + * IMPORTANT: These implied modifiers for legacy userspace must be
> + * stored in struct &drm_framebuffer, including all relevant metadata
> + * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the
> + * modifier enables additional planes beyond the fourcc pixel format
> + * code. This is required by the GETFB2 ioctl.
> + *
> * If the parameters are deemed valid and the backing storage objects in
> * the underlying memory manager all exist, then the driver allocates
> * a new &drm_framebuffer structure, subclassed to contain
> @@ -915,6 +921,13 @@ struct drm_mode_config {
> * @allow_fb_modifiers:
> *
> * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> + *
> + * IMPORTANT:
> + *
> + * If this is set the driver must fill out the full implicit modifier
> + * information in their &drm_mode_config_funcs.fb_create hook for legacy
> + * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
> + * broken for modifier aware userspace.
> */
> bool allow_fb_modifiers;
>
> --
> 2.28.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] drm/doc: Document that modifiers are always required for fb
2020-09-17 17:24 ` Bas Nieuwenhuizen
@ 2020-09-23 15:53 ` Daniel Vetter
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2020-09-23 15:53 UTC (permalink / raw)
To: Bas Nieuwenhuizen
Cc: Pekka Paalanen, Daniel Stone, Marek Olšák,
Daniel Vetter, DRI Development, Daniel Vetter, Juston Li
On Thu, Sep 17, 2020 at 07:24:58PM +0200, Bas Nieuwenhuizen wrote:
> Acked-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>
> On Thu, Sep 17, 2020 at 6:48 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > Even for legacy userspace, since otherwise GETFB2 is broken and if you
> > switch between modifier-less and modifier-aware compositors, smooth
> > transitions break.
> >
> > Also it's just best practice to make sure modifiers are invariant for
> > a given drm_fb, and that a modifier-aware kms drivers only has one
> > place to store them, ignoring any old implicit bo flags or whatever
> > else might float around.
> >
> > Motivated by some irc discussion with Bas about amdgpu modifier
> > support.
> >
> > Acked-by: Simon Ser <contact@emersion.fr>
> > Acked-by: Daniel Stone <daniels@collabora.com>
> > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Fixes: 455e00f1412f ("drm: Add getfb2 ioctl")
> > Cc: Daniel Stone <daniels@collabora.com>
> > Cc: Juston Li <juston.li@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Cc: Marek Olšák <maraeo@gmail.com>
> > Cc: "Wentland, Harry" <harry.wentland@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Pushed to drm-misc-next, thanks for all the feedback.
-Daniel
> > ---
> > Sending this out again to double-check there's no objections or concerns.
> > From what I understand looking at the code the latest modifier patches for
> > amdgpu follow this now.
> >
> > Cheers, Daniel
> > ---
> > include/drm/drm_mode_config.h | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index a18f73eb3cf6..5ffbb4ed5b35 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -58,6 +58,12 @@ struct drm_mode_config_funcs {
> > * actual modifier used if the request doesn't have it specified,
> > * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
> > *
> > + * IMPORTANT: These implied modifiers for legacy userspace must be
> > + * stored in struct &drm_framebuffer, including all relevant metadata
> > + * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the
> > + * modifier enables additional planes beyond the fourcc pixel format
> > + * code. This is required by the GETFB2 ioctl.
> > + *
> > * If the parameters are deemed valid and the backing storage objects in
> > * the underlying memory manager all exist, then the driver allocates
> > * a new &drm_framebuffer structure, subclassed to contain
> > @@ -915,6 +921,13 @@ struct drm_mode_config {
> > * @allow_fb_modifiers:
> > *
> > * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> > + *
> > + * IMPORTANT:
> > + *
> > + * If this is set the driver must fill out the full implicit modifier
> > + * information in their &drm_mode_config_funcs.fb_create hook for legacy
> > + * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
> > + * broken for modifier aware userspace.
> > */
> > bool allow_fb_modifiers;
> >
> > --
> > 2.28.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-09-23 15:54 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-02 10:24 [PATCH] drm/doc: Document that modifiers are always required for fb Daniel Vetter
2020-09-02 11:02 ` Pekka Paalanen
2020-09-02 11:15 ` Daniel Vetter
2020-09-02 11:34 ` Pekka Paalanen
2020-09-02 12:40 ` Simon Ser
2020-09-02 12:44 ` Daniel Vetter
2020-09-02 12:49 ` Simon Ser
2020-09-02 12:56 ` Daniel Stone
2020-09-02 14:29 ` Bas Nieuwenhuizen
2020-09-02 14:29 ` Daniel Vetter
2020-09-02 14:59 ` Simon Ser
2020-09-07 8:31 ` Daniel Vetter
2020-09-07 8:37 ` Simon Ser
2020-09-07 8:41 ` Daniel Vetter
2020-09-07 9:07 ` Pekka Paalanen
2020-09-07 13:51 ` Daniel Vetter
2020-09-17 16:47 Daniel Vetter
2020-09-17 17:24 ` Bas Nieuwenhuizen
2020-09-23 15:53 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).