* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 11:33 [RFC][PATCH] drm: Nuke modifier[1-3] ville.syrjala
@ 2016-11-16 12:10 ` Eric Engestrom
2016-11-16 12:33 ` Ville Syrjälä
2016-11-16 15:30 ` Ben Widawsky
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Eric Engestrom @ 2016-11-16 12:10 UTC (permalink / raw)
To: ville.syrjala
Cc: Daniel Vetter, dczaplejewicz, Kristian Høgsberg, dri-devel,
Ben Widawsky
On Wednesday, 2016-11-16 13:33:16 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> It has been suggested that having per-plane modifiers is making life
> more difficult for userspace, so let's just retire modifier[1-3] and
> use modifier[0] to apply to the entire framebuffer.
>
> Obviosuly this means that if individual planes need different tiling
> layouts and whatnot we will need a new modifier for each combination
> of planes with different tiling layouts.
>
> For a bit of extra backwards compatilbilty the kernel will allow
> non-zero modifier[1+] but it require that they will match modifier[0].
> This in case there's existing userspace out there that sets
> modifier[1+] to something non-zero with planar formats.
>
> Mostly a cocci job, with a bit of manual stuff mixed in.
>
> @@
> struct drm_framebuffer *fb;
> expression E;
> @@
> - fb->modifier[E]
> + fb->modifier
>
> @@
> struct drm_framebuffer fb;
> expression E;
> @@
> - fb.modifier[E]
> + fb.modifier
>
> Cc: Kristian Høgsberg <hoegsberg@gmail.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> Cc: dczaplejewicz@collabora.co.uk
> Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 2 +-
> drivers/gpu/drm/drm_framebuffer.c | 7 +++
> drivers/gpu/drm/drm_modeset_helper.c | 2 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
> drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
> include/drm/drm_framebuffer.h | 4 +-
> include/uapi/drm/drm_mode.h | 13 +++---
> 12 files changed, 79 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 57e0a6e96f6d..bfaa6e4a9989 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>
> drm_printf(p, "\t\tformat=%s\n",
> drm_get_format_name(fb->pixel_format, &format_name));
> + drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
> drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
> drm_printf(p, "\t\tlayers:\n");
> for (i = 0; i < n; i++) {
> drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
> drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
> - drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
> }
> }
> drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 06ad3d1350c4..cbf0c893f426 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> return -EINVAL;
> }
>
> + if (r->flags & DRM_MODE_FB_MODIFIERS &&
+ r->modifier[i] &&
Otherwise this forces the modifiers to be set for all planes, doesn't it?
> + r->modifier[i] != r->modifier[0]) {
> + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> + r->modifier[i], i);
> + return -EINVAL;
> + }
> +
> /* modifier specific checks: */
> switch (r->modifier[i]) {
> case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 12:10 ` Eric Engestrom
@ 2016-11-16 12:33 ` Ville Syrjälä
2016-11-16 13:04 ` Rob Clark
0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2016-11-16 12:33 UTC (permalink / raw)
To: Eric Engestrom
Cc: Daniel Vetter, dczaplejewicz, Kristian Høgsberg, dri-devel,
Ben Widawsky
On Wed, Nov 16, 2016 at 12:10:42PM +0000, Eric Engestrom wrote:
> On Wednesday, 2016-11-16 13:33:16 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > It has been suggested that having per-plane modifiers is making life
> > more difficult for userspace, so let's just retire modifier[1-3] and
> > use modifier[0] to apply to the entire framebuffer.
> >
> > Obviosuly this means that if individual planes need different tiling
> > layouts and whatnot we will need a new modifier for each combination
> > of planes with different tiling layouts.
> >
> > For a bit of extra backwards compatilbilty the kernel will allow
> > non-zero modifier[1+] but it require that they will match modifier[0].
> > This in case there's existing userspace out there that sets
> > modifier[1+] to something non-zero with planar formats.
> >
> > Mostly a cocci job, with a bit of manual stuff mixed in.
> >
> > @@
> > struct drm_framebuffer *fb;
> > expression E;
> > @@
> > - fb->modifier[E]
> > + fb->modifier
> >
> > @@
> > struct drm_framebuffer fb;
> > expression E;
> > @@
> > - fb.modifier[E]
> > + fb.modifier
> >
> > Cc: Kristian Høgsberg <hoegsberg@gmail.com>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> > Cc: dczaplejewicz@collabora.co.uk
> > Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 2 +-
> > drivers/gpu/drm/drm_framebuffer.c | 7 +++
> > drivers/gpu/drm/drm_modeset_helper.c | 2 +-
> > drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> > drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
> > drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
> > drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> > drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
> > drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
> > drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
> > include/drm/drm_framebuffer.h | 4 +-
> > include/uapi/drm/drm_mode.h | 13 +++---
> > 12 files changed, 79 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 57e0a6e96f6d..bfaa6e4a9989 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >
> > drm_printf(p, "\t\tformat=%s\n",
> > drm_get_format_name(fb->pixel_format, &format_name));
> > + drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
> > drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
> > drm_printf(p, "\t\tlayers:\n");
> > for (i = 0; i < n; i++) {
> > drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
> > drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
> > - drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
> > }
> > }
> > drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 06ad3d1350c4..cbf0c893f426 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> > return -EINVAL;
> > }
> >
> > + if (r->flags & DRM_MODE_FB_MODIFIERS &&
>
> + r->modifier[i] &&
>
> Otherwise this forces the modifiers to be set for all planes, doesn't it?
All planes up to num_planes, which is exactly what we want.
>
> > + r->modifier[i] != r->modifier[0]) {
> > + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> > + r->modifier[i], i);
> > + return -EINVAL;
> > + }
> > +
> > /* modifier specific checks: */
> > switch (r->modifier[i]) {
> > case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 12:33 ` Ville Syrjälä
@ 2016-11-16 13:04 ` Rob Clark
2016-11-16 14:43 ` Ville Syrjälä
0 siblings, 1 reply; 10+ messages in thread
From: Rob Clark @ 2016-11-16 13:04 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Daniel Vetter, Kristian Høgsberg,
dri-devel@lists.freedesktop.org, dczaplejewicz, Ben Widawsky
On Wed, Nov 16, 2016 at 7:33 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Nov 16, 2016 at 12:10:42PM +0000, Eric Engestrom wrote:
>> On Wednesday, 2016-11-16 13:33:16 +0200, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > It has been suggested that having per-plane modifiers is making life
>> > more difficult for userspace, so let's just retire modifier[1-3] and
>> > use modifier[0] to apply to the entire framebuffer.
>> >
>> > Obviosuly this means that if individual planes need different tiling
>> > layouts and whatnot we will need a new modifier for each combination
>> > of planes with different tiling layouts.
>> >
>> > For a bit of extra backwards compatilbilty the kernel will allow
>> > non-zero modifier[1+] but it require that they will match modifier[0].
>> > This in case there's existing userspace out there that sets
>> > modifier[1+] to something non-zero with planar formats.
>> >
>> > Mostly a cocci job, with a bit of manual stuff mixed in.
>> >
>> > @@
>> > struct drm_framebuffer *fb;
>> > expression E;
>> > @@
>> > - fb->modifier[E]
>> > + fb->modifier
>> >
>> > @@
>> > struct drm_framebuffer fb;
>> > expression E;
>> > @@
>> > - fb.modifier[E]
>> > + fb.modifier
>> >
>> > Cc: Kristian Høgsberg <hoegsberg@gmail.com>
>> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>> > Cc: Rob Clark <robdclark@gmail.com>
>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
>> > Cc: dczaplejewicz@collabora.co.uk
>> > Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> > drivers/gpu/drm/drm_atomic.c | 2 +-
>> > drivers/gpu/drm/drm_framebuffer.c | 7 +++
>> > drivers/gpu/drm/drm_modeset_helper.c | 2 +-
>> > drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
>> > drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
>> > drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
>> > drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>> > drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
>> > drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
>> > drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
>> > include/drm/drm_framebuffer.h | 4 +-
>> > include/uapi/drm/drm_mode.h | 13 +++---
>> > 12 files changed, 79 insertions(+), 69 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> > index 57e0a6e96f6d..bfaa6e4a9989 100644
>> > --- a/drivers/gpu/drm/drm_atomic.c
>> > +++ b/drivers/gpu/drm/drm_atomic.c
>> > @@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>> >
>> > drm_printf(p, "\t\tformat=%s\n",
>> > drm_get_format_name(fb->pixel_format, &format_name));
>> > + drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
>> > drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
>> > drm_printf(p, "\t\tlayers:\n");
>> > for (i = 0; i < n; i++) {
>> > drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
>> > drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
>> > - drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
>> > }
>> > }
>> > drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
>> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> > index 06ad3d1350c4..cbf0c893f426 100644
>> > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > @@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>> > return -EINVAL;
>> > }
>> >
>> > + if (r->flags & DRM_MODE_FB_MODIFIERS &&
>>
>> + r->modifier[i] &&
>>
>> Otherwise this forces the modifiers to be set for all planes, doesn't it?
>
> All planes up to num_planes, which is exactly what we want.
hmm, why not accept modifier[n]==0 || modifier[n]==modifier[0]?
That gives us the option to reclaim modifier[1..3] later (possibly
with the exception of the tiled nv12 case)..
Maybe we never end up with a need for modifier[1..3] but seems like
leaving the option open is useful..
Other than that, ack on the internal changes.
BR,
-R
>>
>> > + r->modifier[i] != r->modifier[0]) {
>> > + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
>> > + r->modifier[i], i);
>> > + return -EINVAL;
>> > + }
>> > +
>> > /* modifier specific checks: */
>> > switch (r->modifier[i]) {
>> > case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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] 10+ messages in thread* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 13:04 ` Rob Clark
@ 2016-11-16 14:43 ` Ville Syrjälä
2016-11-16 14:54 ` Daniel Vetter
2016-11-16 15:08 ` Rob Clark
0 siblings, 2 replies; 10+ messages in thread
From: Ville Syrjälä @ 2016-11-16 14:43 UTC (permalink / raw)
To: Rob Clark
Cc: Daniel Vetter, Kristian Høgsberg,
dri-devel@lists.freedesktop.org, dczaplejewicz, Ben Widawsky
On Wed, Nov 16, 2016 at 08:04:23AM -0500, Rob Clark wrote:
> On Wed, Nov 16, 2016 at 7:33 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Nov 16, 2016 at 12:10:42PM +0000, Eric Engestrom wrote:
> >> On Wednesday, 2016-11-16 13:33:16 +0200, ville.syrjala@linux.intel.com wrote:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> > It has been suggested that having per-plane modifiers is making life
> >> > more difficult for userspace, so let's just retire modifier[1-3] and
> >> > use modifier[0] to apply to the entire framebuffer.
> >> >
> >> > Obviosuly this means that if individual planes need different tiling
> >> > layouts and whatnot we will need a new modifier for each combination
> >> > of planes with different tiling layouts.
> >> >
> >> > For a bit of extra backwards compatilbilty the kernel will allow
> >> > non-zero modifier[1+] but it require that they will match modifier[0].
> >> > This in case there's existing userspace out there that sets
> >> > modifier[1+] to something non-zero with planar formats.
> >> >
> >> > Mostly a cocci job, with a bit of manual stuff mixed in.
> >> >
> >> > @@
> >> > struct drm_framebuffer *fb;
> >> > expression E;
> >> > @@
> >> > - fb->modifier[E]
> >> > + fb->modifier
> >> >
> >> > @@
> >> > struct drm_framebuffer fb;
> >> > expression E;
> >> > @@
> >> > - fb.modifier[E]
> >> > + fb.modifier
> >> >
> >> > Cc: Kristian Høgsberg <hoegsberg@gmail.com>
> >> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> >> > Cc: Rob Clark <robdclark@gmail.com>
> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> >> > Cc: dczaplejewicz@collabora.co.uk
> >> > Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > ---
> >> > drivers/gpu/drm/drm_atomic.c | 2 +-
> >> > drivers/gpu/drm/drm_framebuffer.c | 7 +++
> >> > drivers/gpu/drm/drm_modeset_helper.c | 2 +-
> >> > drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> >> > drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
> >> > drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
> >> > drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> >> > drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
> >> > drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
> >> > drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
> >> > include/drm/drm_framebuffer.h | 4 +-
> >> > include/uapi/drm/drm_mode.h | 13 +++---
> >> > 12 files changed, 79 insertions(+), 69 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> > index 57e0a6e96f6d..bfaa6e4a9989 100644
> >> > --- a/drivers/gpu/drm/drm_atomic.c
> >> > +++ b/drivers/gpu/drm/drm_atomic.c
> >> > @@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> >> >
> >> > drm_printf(p, "\t\tformat=%s\n",
> >> > drm_get_format_name(fb->pixel_format, &format_name));
> >> > + drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
> >> > drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
> >> > drm_printf(p, "\t\tlayers:\n");
> >> > for (i = 0; i < n; i++) {
> >> > drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
> >> > drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
> >> > - drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
> >> > }
> >> > }
> >> > drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
> >> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> >> > index 06ad3d1350c4..cbf0c893f426 100644
> >> > --- a/drivers/gpu/drm/drm_framebuffer.c
> >> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> >> > @@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> >> > return -EINVAL;
> >> > }
> >> >
> >> > + if (r->flags & DRM_MODE_FB_MODIFIERS &&
> >>
> >> + r->modifier[i] &&
> >>
> >> Otherwise this forces the modifiers to be set for all planes, doesn't it?
> >
> > All planes up to num_planes, which is exactly what we want.
>
> hmm, why not accept modifier[n]==0 || modifier[n]==modifier[0]?
Hmm. That would have meant [0]=tiled, [1]=linear or something like that.
If anyone depends on that then I guess we would be breaking it regardless.
My way we'd beak it with an error, and your way we'd break it silently.
Either way doesn't seem ideal for our abi. But if we have no userspace
doing that then I guess we can do what you suggest.
>
> That gives us the option to reclaim modifier[1..3] later (possibly
> with the exception of the tiled nv12 case)..
That sort of exception is starting to get quite ugly. IMO either we
allow the use of modifiers[1-3] or we don't, no nv12 special cases.
>
> Maybe we never end up with a need for modifier[1..3] but seems like
> leaving the option open is useful..
>
> Other than that, ack on the internal changes.
>
> BR,
> -R
>
> >>
> >> > + r->modifier[i] != r->modifier[0]) {
> >> > + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> >> > + r->modifier[i], i);
> >> > + return -EINVAL;
> >> > + }
> >> > +
> >> > /* modifier specific checks: */
> >> > switch (r->modifier[i]) {
> >> > case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 14:43 ` Ville Syrjälä
@ 2016-11-16 14:54 ` Daniel Vetter
2016-11-16 15:08 ` Rob Clark
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2016-11-16 14:54 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Daniel Vetter, Kristian Høgsberg,
dri-devel@lists.freedesktop.org, dczaplejewicz, Ben Widawsky
On Wed, Nov 16, 2016 at 04:43:23PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 16, 2016 at 08:04:23AM -0500, Rob Clark wrote:
> > On Wed, Nov 16, 2016 at 7:33 AM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > On Wed, Nov 16, 2016 at 12:10:42PM +0000, Eric Engestrom wrote:
> > >> On Wednesday, 2016-11-16 13:33:16 +0200, ville.syrjala@linux.intel.com wrote:
> > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> >
> > >> > It has been suggested that having per-plane modifiers is making life
> > >> > more difficult for userspace, so let's just retire modifier[1-3] and
> > >> > use modifier[0] to apply to the entire framebuffer.
> > >> >
> > >> > Obviosuly this means that if individual planes need different tiling
> > >> > layouts and whatnot we will need a new modifier for each combination
> > >> > of planes with different tiling layouts.
> > >> >
> > >> > For a bit of extra backwards compatilbilty the kernel will allow
> > >> > non-zero modifier[1+] but it require that they will match modifier[0].
> > >> > This in case there's existing userspace out there that sets
> > >> > modifier[1+] to something non-zero with planar formats.
> > >> >
> > >> > Mostly a cocci job, with a bit of manual stuff mixed in.
> > >> >
> > >> > @@
> > >> > struct drm_framebuffer *fb;
> > >> > expression E;
> > >> > @@
> > >> > - fb->modifier[E]
> > >> > + fb->modifier
> > >> >
> > >> > @@
> > >> > struct drm_framebuffer fb;
> > >> > expression E;
> > >> > @@
> > >> > - fb.modifier[E]
> > >> > + fb.modifier
> > >> >
> > >> > Cc: Kristian Høgsberg <hoegsberg@gmail.com>
> > >> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > >> > Cc: Rob Clark <robdclark@gmail.com>
> > >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >> > Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> > >> > Cc: dczaplejewicz@collabora.co.uk
> > >> > Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > ---
> > >> > drivers/gpu/drm/drm_atomic.c | 2 +-
> > >> > drivers/gpu/drm/drm_framebuffer.c | 7 +++
> > >> > drivers/gpu/drm/drm_modeset_helper.c | 2 +-
> > >> > drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> > >> > drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
> > >> > drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
> > >> > drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> > >> > drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
> > >> > drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
> > >> > drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
> > >> > include/drm/drm_framebuffer.h | 4 +-
> > >> > include/uapi/drm/drm_mode.h | 13 +++---
> > >> > 12 files changed, 79 insertions(+), 69 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > >> > index 57e0a6e96f6d..bfaa6e4a9989 100644
> > >> > --- a/drivers/gpu/drm/drm_atomic.c
> > >> > +++ b/drivers/gpu/drm/drm_atomic.c
> > >> > @@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
> > >> >
> > >> > drm_printf(p, "\t\tformat=%s\n",
> > >> > drm_get_format_name(fb->pixel_format, &format_name));
> > >> > + drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
> > >> > drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
> > >> > drm_printf(p, "\t\tlayers:\n");
> > >> > for (i = 0; i < n; i++) {
> > >> > drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
> > >> > drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
> > >> > - drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
> > >> > }
> > >> > }
> > >> > drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
> > >> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > >> > index 06ad3d1350c4..cbf0c893f426 100644
> > >> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > >> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > >> > @@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> > >> > return -EINVAL;
> > >> > }
> > >> >
> > >> > + if (r->flags & DRM_MODE_FB_MODIFIERS &&
> > >>
> > >> + r->modifier[i] &&
> > >>
> > >> Otherwise this forces the modifiers to be set for all planes, doesn't it?
> > >
> > > All planes up to num_planes, which is exactly what we want.
> >
> > hmm, why not accept modifier[n]==0 || modifier[n]==modifier[0]?
>
> Hmm. That would have meant [0]=tiled, [1]=linear or something like that.
> If anyone depends on that then I guess we would be breaking it regardless.
> My way we'd beak it with an error, and your way we'd break it silently.
> Either way doesn't seem ideal for our abi. But if we have no userspace
> doing that then I guess we can do what you suggest.
If possible I'm voting for enforcing modifier[n] == modifier[0], no
exception for 0 modifier.
> > That gives us the option to reclaim modifier[1..3] later (possibly
> > with the exception of the tiled nv12 case)..
>
> That sort of exception is starting to get quite ugly. IMO either we
> allow the use of modifiers[1-3] or we don't, no nv12 special cases.
Imo modifier[1-3] are dead weight, any attempt at recovering them will
lead to trouble and confusion. We can fix this with addfb3 ;-)
> > Maybe we never end up with a need for modifier[1..3] but seems like
> > leaving the option open is useful..
Bytes aren't that expensive that we need to recover 24 of them in a
temporary ioctl call.
-Daniel
> >
> > Other than that, ack on the internal changes.
> >
> > BR,
> > -R
> >
> > >>
> > >> > + r->modifier[i] != r->modifier[0]) {
> > >> > + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> > >> > + r->modifier[i], i);
> > >> > + return -EINVAL;
> > >> > + }
> > >> > +
> > >> > /* modifier specific checks: */
> > >> > switch (r->modifier[i]) {
> > >> > case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
--
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] 10+ messages in thread* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 14:43 ` Ville Syrjälä
2016-11-16 14:54 ` Daniel Vetter
@ 2016-11-16 15:08 ` Rob Clark
1 sibling, 0 replies; 10+ messages in thread
From: Rob Clark @ 2016-11-16 15:08 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Daniel Vetter, Kristian Høgsberg,
dri-devel@lists.freedesktop.org, dczaplejewicz, Ben Widawsky
On Wed, Nov 16, 2016 at 9:43 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Nov 16, 2016 at 08:04:23AM -0500, Rob Clark wrote:
>> On Wed, Nov 16, 2016 at 7:33 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Wed, Nov 16, 2016 at 12:10:42PM +0000, Eric Engestrom wrote:
>> >> On Wednesday, 2016-11-16 13:33:16 +0200, ville.syrjala@linux.intel.com wrote:
>> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> >
>> >> > It has been suggested that having per-plane modifiers is making life
>> >> > more difficult for userspace, so let's just retire modifier[1-3] and
>> >> > use modifier[0] to apply to the entire framebuffer.
>> >> >
>> >> > Obviosuly this means that if individual planes need different tiling
>> >> > layouts and whatnot we will need a new modifier for each combination
>> >> > of planes with different tiling layouts.
>> >> >
>> >> > For a bit of extra backwards compatilbilty the kernel will allow
>> >> > non-zero modifier[1+] but it require that they will match modifier[0].
>> >> > This in case there's existing userspace out there that sets
>> >> > modifier[1+] to something non-zero with planar formats.
>> >> >
>> >> > Mostly a cocci job, with a bit of manual stuff mixed in.
>> >> >
>> >> > @@
>> >> > struct drm_framebuffer *fb;
>> >> > expression E;
>> >> > @@
>> >> > - fb->modifier[E]
>> >> > + fb->modifier
>> >> >
>> >> > @@
>> >> > struct drm_framebuffer fb;
>> >> > expression E;
>> >> > @@
>> >> > - fb.modifier[E]
>> >> > + fb.modifier
>> >> >
>> >> > Cc: Kristian Høgsberg <hoegsberg@gmail.com>
>> >> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>> >> > Cc: Rob Clark <robdclark@gmail.com>
>> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >> > Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
>> >> > Cc: dczaplejewicz@collabora.co.uk
>> >> > Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
>> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > ---
>> >> > drivers/gpu/drm/drm_atomic.c | 2 +-
>> >> > drivers/gpu/drm/drm_framebuffer.c | 7 +++
>> >> > drivers/gpu/drm/drm_modeset_helper.c | 2 +-
>> >> > drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
>> >> > drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
>> >> > drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
>> >> > drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>> >> > drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
>> >> > drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
>> >> > drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
>> >> > include/drm/drm_framebuffer.h | 4 +-
>> >> > include/uapi/drm/drm_mode.h | 13 +++---
>> >> > 12 files changed, 79 insertions(+), 69 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >> > index 57e0a6e96f6d..bfaa6e4a9989 100644
>> >> > --- a/drivers/gpu/drm/drm_atomic.c
>> >> > +++ b/drivers/gpu/drm/drm_atomic.c
>> >> > @@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>> >> >
>> >> > drm_printf(p, "\t\tformat=%s\n",
>> >> > drm_get_format_name(fb->pixel_format, &format_name));
>> >> > + drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
>> >> > drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
>> >> > drm_printf(p, "\t\tlayers:\n");
>> >> > for (i = 0; i < n; i++) {
>> >> > drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
>> >> > drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
>> >> > - drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
>> >> > }
>> >> > }
>> >> > drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
>> >> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> >> > index 06ad3d1350c4..cbf0c893f426 100644
>> >> > --- a/drivers/gpu/drm/drm_framebuffer.c
>> >> > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> >> > @@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>> >> > return -EINVAL;
>> >> > }
>> >> >
>> >> > + if (r->flags & DRM_MODE_FB_MODIFIERS &&
>> >>
>> >> + r->modifier[i] &&
>> >>
>> >> Otherwise this forces the modifiers to be set for all planes, doesn't it?
>> >
>> > All planes up to num_planes, which is exactly what we want.
>>
>> hmm, why not accept modifier[n]==0 || modifier[n]==modifier[0]?
>
> Hmm. That would have meant [0]=tiled, [1]=linear or something like that.
> If anyone depends on that then I guess we would be breaking it regardless.
> My way we'd beak it with an error, and your way we'd break it silently.
> Either way doesn't seem ideal for our abi. But if we have no userspace
> doing that then I guess we can do what you suggest.
>
>>
>> That gives us the option to reclaim modifier[1..3] later (possibly
>> with the exception of the tiled nv12 case)..
>
> That sort of exception is starting to get quite ugly. IMO either we
> allow the use of modifiers[1-3] or we don't, no nv12 special cases.
well, tiled nv12 is, I think, the only case of planar+modifier in use
currently. And it isn't one where we are going to likely need to add
other modifiers (compression, etc)
I'm also not sure it is used outside of some test code.. drm-hwc2 is
first place where it actually becomes plausible to scanout tiled nv12
other than fullscreen. And until recent patches on mesa-dev we didn't
have an egl extension to import tiled-nv12 for non-fullscreen case.
So I think we could even get away with insisting that
modifiers[1..3]==0 in all cases.
BR,
-R
>>
>> Maybe we never end up with a need for modifier[1..3] but seems like
>> leaving the option open is useful..
>>
>> Other than that, ack on the internal changes.
>>
>> BR,
>> -R
>>
>> >>
>> >> > + r->modifier[i] != r->modifier[0]) {
>> >> > + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
>> >> > + r->modifier[i], i);
>> >> > + return -EINVAL;
>> >> > + }
>> >> > +
>> >> > /* modifier specific checks: */
>> >> > switch (r->modifier[i]) {
>> >> > case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 11:33 [RFC][PATCH] drm: Nuke modifier[1-3] ville.syrjala
2016-11-16 12:10 ` Eric Engestrom
@ 2016-11-16 15:30 ` Ben Widawsky
2016-11-16 15:36 ` Daniel Stone
2016-11-17 10:59 ` Daniel Vetter
3 siblings, 0 replies; 10+ messages in thread
From: Ben Widawsky @ 2016-11-16 15:30 UTC (permalink / raw)
To: ville.syrjala
Cc: Daniel Vetter, Kristian Høgsberg, dri-devel, dczaplejewicz
Acked-by: Ben Widawsky <ben@bwidawsk.net>
On 16-11-16 13:33:16, Ville Syrjälä wrote:
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>It has been suggested that having per-plane modifiers is making life
>more difficult for userspace, so let's just retire modifier[1-3] and
>use modifier[0] to apply to the entire framebuffer.
>
>Obviosuly this means that if individual planes need different tiling
>layouts and whatnot we will need a new modifier for each combination
>of planes with different tiling layouts.
>
>For a bit of extra backwards compatilbilty the kernel will allow
>non-zero modifier[1+] but it require that they will match modifier[0].
>This in case there's existing userspace out there that sets
>modifier[1+] to something non-zero with planar formats.
>
>Mostly a cocci job, with a bit of manual stuff mixed in.
>
>@@
>struct drm_framebuffer *fb;
>expression E;
>@@
>- fb->modifier[E]
>+ fb->modifier
>
>@@
>struct drm_framebuffer fb;
>expression E;
>@@
>- fb.modifier[E]
>+ fb.modifier
>
>Cc: Kristian Høgsberg <hoegsberg@gmail.com>
>Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>Cc: Rob Clark <robdclark@gmail.com>
>Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
>Cc: dczaplejewicz@collabora.co.uk
>Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/drm_atomic.c | 2 +-
> drivers/gpu/drm/drm_framebuffer.c | 7 +++
> drivers/gpu/drm/drm_modeset_helper.c | 2 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
> drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
> include/drm/drm_framebuffer.h | 4 +-
> include/uapi/drm/drm_mode.h | 13 +++---
> 12 files changed, 79 insertions(+), 69 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>index 57e0a6e96f6d..bfaa6e4a9989 100644
>--- a/drivers/gpu/drm/drm_atomic.c
>+++ b/drivers/gpu/drm/drm_atomic.c
>@@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>
> drm_printf(p, "\t\tformat=%s\n",
> drm_get_format_name(fb->pixel_format, &format_name));
>+ drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
> drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
> drm_printf(p, "\t\tlayers:\n");
> for (i = 0; i < n; i++) {
> drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
> drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
>- drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
> }
> }
> drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
>diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>index 06ad3d1350c4..cbf0c893f426 100644
>--- a/drivers/gpu/drm/drm_framebuffer.c
>+++ b/drivers/gpu/drm/drm_framebuffer.c
>@@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> return -EINVAL;
> }
>
>+ if (r->flags & DRM_MODE_FB_MODIFIERS &&
>+ r->modifier[i] != r->modifier[0]) {
>+ DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
>+ r->modifier[i], i);
>+ return -EINVAL;
>+ }
>+
> /* modifier specific checks: */
> switch (r->modifier[i]) {
> case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
>diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
>index 2f452b3dd40e..633355e02398 100644
>--- a/drivers/gpu/drm/drm_modeset_helper.c
>+++ b/drivers/gpu/drm/drm_modeset_helper.c
>@@ -93,8 +93,8 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
> for (i = 0; i < 4; i++) {
> fb->pitches[i] = mode_cmd->pitches[i];
> fb->offsets[i] = mode_cmd->offsets[i];
>- fb->modifier[i] = mode_cmd->modifier[i];
> }
>+ fb->modifier = mode_cmd->modifier[0];
> fb->pixel_format = mode_cmd->pixel_format;
> fb->flags = mode_cmd->flags;
> }
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index 1cc971cb6cb1..ae136af4aa37 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -1897,7 +1897,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
> fbdev_fb->base.height,
> fbdev_fb->base.depth,
> fbdev_fb->base.bits_per_pixel,
>- fbdev_fb->base.modifier[0],
>+ fbdev_fb->base.modifier,
> drm_framebuffer_read_refcount(&fbdev_fb->base));
> describe_obj(m, fbdev_fb->obj);
> seq_putc(m, '\n');
>@@ -1915,7 +1915,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
> fb->base.height,
> fb->base.depth,
> fb->base.bits_per_pixel,
>- fb->base.modifier[0],
>+ fb->base.modifier,
> drm_framebuffer_read_refcount(&fb->base));
> describe_obj(m, fb->obj);
> seq_putc(m, '\n');
>diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>index ff821649486e..dbe9fb41ae53 100644
>--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>@@ -144,8 +144,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> if (state->fb && drm_rotation_90_or_270(state->rotation)) {
> struct drm_format_name_buf format_name;
>
>- if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>- state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
>+ if (state->fb->modifier != I915_FORMAT_MOD_Y_TILED &&
>+ state->fb->modifier != I915_FORMAT_MOD_Yf_TILED) {
> DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> return -EINVAL;
> }
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 2ebb8b833395..0d4884f1d321 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -2195,7 +2195,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
>- alignment = intel_surf_alignment(dev_priv, fb->modifier[0]);
>+ alignment = intel_surf_alignment(dev_priv, fb->modifier);
>
> intel_fill_fb_ggtt_view(&view, fb, rotation);
>
>@@ -2356,13 +2356,13 @@ static u32 intel_adjust_tile_offset(int *x, int *y,
>
> WARN_ON(new_offset > old_offset);
>
>- if (fb->modifier[plane] != DRM_FORMAT_MOD_NONE) {
>+ if (fb->modifier != DRM_FORMAT_MOD_NONE) {
> unsigned int tile_size, tile_width, tile_height;
> unsigned int pitch_tiles;
>
> tile_size = intel_tile_size(dev_priv);
> intel_tile_dims(dev_priv, &tile_width, &tile_height,
>- fb->modifier[plane], cpp);
>+ fb->modifier, cpp);
>
> if (drm_rotation_90_or_270(rotation)) {
> pitch_tiles = pitch / tile_height;
>@@ -2405,7 +2405,7 @@ static u32 _intel_compute_tile_offset(const struct drm_i915_private *dev_priv,
> unsigned int rotation,
> u32 alignment)
> {
>- uint64_t fb_modifier = fb->modifier[plane];
>+ uint64_t fb_modifier = fb->modifier;
> unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, plane);
> u32 offset, offset_aligned;
>
>@@ -2464,7 +2464,7 @@ u32 intel_compute_tile_offset(int *x, int *y,
> if (fb->pixel_format == DRM_FORMAT_NV12 && plane == 1)
> alignment = 4096;
> else
>- alignment = intel_surf_alignment(dev_priv, fb->modifier[plane]);
>+ alignment = intel_surf_alignment(dev_priv, fb->modifier);
>
> return _intel_compute_tile_offset(dev_priv, x, y, fb, plane, pitch,
> rotation, alignment);
>@@ -2546,13 +2546,13 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> DRM_ROTATE_0, tile_size);
> offset /= tile_size;
>
>- if (fb->modifier[i] != DRM_FORMAT_MOD_NONE) {
>+ if (fb->modifier != DRM_FORMAT_MOD_NONE) {
> unsigned int tile_width, tile_height;
> unsigned int pitch_tiles;
> struct drm_rect r;
>
> intel_tile_dims(dev_priv, &tile_width, &tile_height,
>- fb->modifier[i], cpp);
>+ fb->modifier, cpp);
>
> rot_info->plane[i].offset = offset;
> rot_info->plane[i].stride = DIV_ROUND_UP(fb->pitches[i], tile_width * cpp);
>@@ -2711,7 +2711,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> mode_cmd.width = fb->width;
> mode_cmd.height = fb->height;
> mode_cmd.pitches[0] = fb->pitches[0];
>- mode_cmd.modifier[0] = fb->modifier[0];
>+ mode_cmd.modifier[0] = fb->modifier;
> mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
>
> if (intel_framebuffer_init(dev, to_intel_framebuffer(fb),
>@@ -2841,7 +2841,7 @@ static int skl_max_plane_width(const struct drm_framebuffer *fb, int plane,
> {
> int cpp = drm_format_plane_cpp(fb->pixel_format, plane);
>
>- switch (fb->modifier[plane]) {
>+ switch (fb->modifier) {
> case DRM_FORMAT_MOD_NONE:
> case I915_FORMAT_MOD_X_TILED:
> switch (cpp) {
>@@ -2872,7 +2872,7 @@ static int skl_max_plane_width(const struct drm_framebuffer *fb, int plane,
> }
> break;
> default:
>- MISSING_CASE(fb->modifier[plane]);
>+ MISSING_CASE(fb->modifier);
> }
>
> return 2048;
>@@ -2900,7 +2900,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> intel_add_fb_offsets(&x, &y, plane_state, 0);
> offset = intel_compute_tile_offset(&x, &y, plane_state, 0);
>
>- alignment = intel_surf_alignment(dev_priv, fb->modifier[0]);
>+ alignment = intel_surf_alignment(dev_priv, fb->modifier);
>
> /*
> * AUX surface offset is specified as the distance from the
>@@ -2917,7 +2917,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> *
> * TODO: linear and Y-tiled seem fine, Yf untested,
> */
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED) {
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED) {
> int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>
> while ((x + w) * cpp > fb->pitches[0]) {
>@@ -3066,7 +3066,7 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
> }
>
> if (INTEL_GEN(dev_priv) >= 4 &&
>- fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ fb->modifier == I915_FORMAT_MOD_X_TILED)
> dspcntr |= DISPPLANE_TILED;
>
> if (rotation & DRM_ROTATE_180)
>@@ -3177,7 +3177,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
> BUG();
> }
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> dspcntr |= DISPPLANE_TILED;
>
> if (rotation & DRM_ROTATE_180)
>@@ -3287,9 +3287,9 @@ u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
> if (drm_rotation_90_or_270(rotation)) {
> int cpp = drm_format_plane_cpp(fb->pixel_format, plane);
>
>- stride /= intel_tile_height(dev_priv, fb->modifier[0], cpp);
>+ stride /= intel_tile_height(dev_priv, fb->modifier, cpp);
> } else {
>- stride /= intel_fb_stride_alignment(dev_priv, fb->modifier[0],
>+ stride /= intel_fb_stride_alignment(dev_priv, fb->modifier,
> fb->pixel_format);
> }
>
>@@ -3405,7 +3405,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
> PLANE_CTL_PIPE_CSC_ENABLE;
>
> plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
>- plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>+ plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> plane_ctl |= skl_plane_ctl_rotation(rotation);
>
>@@ -8731,7 +8731,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> if (INTEL_INFO(dev)->gen >= 4) {
> if (val & DISPPLANE_TILED) {
> plane_config->tiling = I915_TILING_X;
>- fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
>+ fb->modifier = I915_FORMAT_MOD_X_TILED;
> }
> }
>
>@@ -8760,7 +8760,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>
> aligned_height = intel_fb_align_height(dev, fb->height,
> fb->pixel_format,
>- fb->modifier[0]);
>+ fb->modifier);
>
> plane_config->size = fb->pitches[0] * aligned_height;
>
>@@ -9774,17 +9774,17 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> tiling = val & PLANE_CTL_TILED_MASK;
> switch (tiling) {
> case PLANE_CTL_TILED_LINEAR:
>- fb->modifier[0] = DRM_FORMAT_MOD_NONE;
>+ fb->modifier = DRM_FORMAT_MOD_NONE;
> break;
> case PLANE_CTL_TILED_X:
> plane_config->tiling = I915_TILING_X;
>- fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
>+ fb->modifier = I915_FORMAT_MOD_X_TILED;
> break;
> case PLANE_CTL_TILED_Y:
>- fb->modifier[0] = I915_FORMAT_MOD_Y_TILED;
>+ fb->modifier = I915_FORMAT_MOD_Y_TILED;
> break;
> case PLANE_CTL_TILED_YF:
>- fb->modifier[0] = I915_FORMAT_MOD_Yf_TILED;
>+ fb->modifier = I915_FORMAT_MOD_Yf_TILED;
> break;
> default:
> MISSING_CASE(tiling);
>@@ -9801,13 +9801,13 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> fb->width = ((val >> 0) & 0x1fff) + 1;
>
> val = I915_READ(PLANE_STRIDE(pipe, 0));
>- stride_mult = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
>+ stride_mult = intel_fb_stride_alignment(dev_priv, fb->modifier,
> fb->pixel_format);
> fb->pitches[0] = (val & 0x3ff) * stride_mult;
>
> aligned_height = intel_fb_align_height(dev, fb->height,
> fb->pixel_format,
>- fb->modifier[0]);
>+ fb->modifier);
>
> plane_config->size = fb->pitches[0] * aligned_height;
>
>@@ -9875,7 +9875,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> if (INTEL_INFO(dev)->gen >= 4) {
> if (val & DISPPLANE_TILED) {
> plane_config->tiling = I915_TILING_X;
>- fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
>+ fb->modifier = I915_FORMAT_MOD_X_TILED;
> }
> }
>
>@@ -9904,7 +9904,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>
> aligned_height = intel_fb_align_height(dev, fb->height,
> fb->pixel_format,
>- fb->modifier[0]);
>+ fb->modifier);
>
> plane_config->size = fb->pitches[0] * aligned_height;
>
>@@ -11812,7 +11812,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> intel_ring_emit(ring, fb->pitches[0]);
> intel_ring_emit(ring, intel_crtc->flip_work->gtt_offset |
>- intel_fb_modifier_to_tiling(fb->modifier[0]));
>+ intel_fb_modifier_to_tiling(fb->modifier));
>
> /* XXX Enabling the panel-fitter across page-flip is so far
> * untested on non-native modes, so ignore it for now.
>@@ -11845,7 +11845,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, MI_DISPLAY_FLIP |
> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> intel_ring_emit(ring, fb->pitches[0] |
>- intel_fb_modifier_to_tiling(fb->modifier[0]));
>+ intel_fb_modifier_to_tiling(fb->modifier));
> intel_ring_emit(ring, intel_crtc->flip_work->gtt_offset);
>
> /* Contrary to the suggestions in the documentation,
>@@ -11951,7 +11951,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>
> intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
> intel_ring_emit(ring, fb->pitches[0] |
>- intel_fb_modifier_to_tiling(fb->modifier[0]));
>+ intel_fb_modifier_to_tiling(fb->modifier));
> intel_ring_emit(ring, intel_crtc->flip_work->gtt_offset);
> intel_ring_emit(ring, (MI_NOOP));
>
>@@ -11997,7 +11997,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
>
> ctl = I915_READ(PLANE_CTL(pipe, 0));
> ctl &= ~PLANE_CTL_TILED_MASK;
>- switch (fb->modifier[0]) {
>+ switch (fb->modifier) {
> case DRM_FORMAT_MOD_NONE:
> break;
> case I915_FORMAT_MOD_X_TILED:
>@@ -12010,7 +12010,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> ctl |= PLANE_CTL_TILED_YF;
> break;
> default:
>- MISSING_CASE(fb->modifier[0]);
>+ MISSING_CASE(fb->modifier);
> }
>
> /*
>@@ -12035,7 +12035,7 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc,
>
> dspcntr = I915_READ(reg);
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> dspcntr |= DISPPLANE_TILED;
> else
> dspcntr &= ~DISPPLANE_TILED;
>@@ -12251,7 +12251,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> engine = dev_priv->engine[BCS];
>- if (fb->modifier[0] != old_fb->modifier[0])
>+ if (fb->modifier != old_fb->modifier)
> /* vlv: DISPLAY_FLIP fails to change tiling */
> engine = NULL;
> } else if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv)) {
>@@ -12407,7 +12407,7 @@ static bool intel_wm_need_update(struct drm_plane *plane,
> if (!cur->base.fb || !new->base.fb)
> return false;
>
>- if (cur->base.fb->modifier[0] != new->base.fb->modifier[0] ||
>+ if (cur->base.fb->modifier != new->base.fb->modifier ||
> cur->base.rotation != new->base.rotation ||
> drm_rect_width(&new->base.src) != drm_rect_width(&cur->base.src) ||
> drm_rect_height(&new->base.src) != drm_rect_height(&cur->base.src) ||
>@@ -15138,7 +15138,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> return -ENOMEM;
> }
>
>- if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
>+ if (fb->modifier != DRM_FORMAT_MOD_NONE) {
> DRM_DEBUG_KMS("cursor cannot be tiled\n");
> return -EINVAL;
> }
>diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>index fc958d5ed0dc..beb08982dc0b 100644
>--- a/drivers/gpu/drm/i915/intel_fbdev.c
>+++ b/drivers/gpu/drm/i915/intel_fbdev.c
>@@ -633,7 +633,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> cur_size = intel_crtc->config->base.adjusted_mode.crtc_vdisplay;
> cur_size = intel_fb_align_height(dev, cur_size,
> fb->base.pixel_format,
>- fb->base.modifier[0]);
>+ fb->base.modifier);
> cur_size *= fb->base.pitches[0];
> DRM_DEBUG_KMS("pipe %c area: %dx%d, bpp: %d, size: %d\n",
> pipe_name(intel_crtc->pipe),
>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>index 1331bcc41868..572919775ece 100644
>--- a/drivers/gpu/drm/i915/intel_pm.c
>+++ b/drivers/gpu/drm/i915/intel_pm.c
>@@ -3065,7 +3065,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> latency = dev_priv->wm.skl_latency[level];
>
> if (skl_needs_memory_bw_wa(intel_state) &&
>- plane->base.state->fb->modifier[0] ==
>+ plane->base.state->fb->modifier ==
> I915_FORMAT_MOD_X_TILED)
> latency += 15;
>
>@@ -3325,8 +3325,8 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
> return 0;
>
> /* For Non Y-tile return 8-blocks */
>- if (fb->modifier[0] != I915_FORMAT_MOD_Y_TILED &&
>- fb->modifier[0] != I915_FORMAT_MOD_Yf_TILED)
>+ if (fb->modifier != I915_FORMAT_MOD_Y_TILED &&
>+ fb->modifier != I915_FORMAT_MOD_Yf_TILED)
> return 8;
>
> src_w = drm_rect_width(&intel_pstate->base.src) >> 16;
>@@ -3595,7 +3595,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> return 0;
> }
>
>- if (apply_memory_bw_wa && fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (apply_memory_bw_wa && fb->modifier == I915_FORMAT_MOD_X_TILED)
> latency += 15;
>
> width = drm_rect_width(&intel_pstate->base.src) >> 16;
>@@ -3634,12 +3634,12 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> y_min_scanlines *= 2;
>
> plane_bytes_per_line = width * cpp;
>- if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>- fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
>+ if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
>+ fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
> plane_blocks_per_line /= y_min_scanlines;
>- } else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) {
>+ } else if (fb->modifier == DRM_FORMAT_MOD_NONE) {
> plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
> + 1;
> } else {
>@@ -3654,8 +3654,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>
> y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>- fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
>+ if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
>+ fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> selected_result = max(method2, y_tile_minimum);
> } else {
> if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
>@@ -3671,8 +3671,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> res_lines = DIV_ROUND_UP(selected_result, plane_blocks_per_line);
>
> if (level >= 1 && level <= 7) {
>- if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>- fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
>+ if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
>+ fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> res_blocks += y_tile_minimum;
> res_lines += y_min_scanlines;
> } else {
>diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>index c2b629c922e7..8f131a08d440 100644
>--- a/drivers/gpu/drm/i915/intel_sprite.c
>+++ b/drivers/gpu/drm/i915/intel_sprite.c
>@@ -224,7 +224,7 @@ skl_update_plane(struct drm_plane *drm_plane,
> PLANE_CTL_PIPE_CSC_ENABLE;
>
> plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
>- plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>+ plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
>
> plane_ctl |= skl_plane_ctl_rotation(rotation);
>
>@@ -406,7 +406,7 @@ vlv_update_plane(struct drm_plane *dplane,
> */
> sprctl |= SP_GAMMA_ENABLE;
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> sprctl |= SP_TILED;
>
> if (rotation & DRM_ROTATE_180)
>@@ -448,7 +448,7 @@ vlv_update_plane(struct drm_plane *dplane,
> I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
> I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
> else
> I915_WRITE(SPLINOFF(pipe, plane), linear_offset);
>@@ -531,7 +531,7 @@ ivb_update_plane(struct drm_plane *plane,
> */
> sprctl |= SPRITE_GAMMA_ENABLE;
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> sprctl |= SPRITE_TILED;
>
> if (rotation & DRM_ROTATE_180)
>@@ -584,7 +584,7 @@ ivb_update_plane(struct drm_plane *plane,
> * register */
> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> I915_WRITE(SPROFFSET(pipe), (y << 16) | x);
>- else if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ else if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> else
> I915_WRITE(SPRLINOFF(pipe), linear_offset);
>@@ -669,7 +669,7 @@ ilk_update_plane(struct drm_plane *plane,
> */
> dvscntr |= DVS_GAMMA_ENABLE;
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> dvscntr |= DVS_TILED;
>
> if (rotation & DRM_ROTATE_180)
>@@ -712,7 +712,7 @@ ilk_update_plane(struct drm_plane *plane,
> I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
> I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>
>- if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
>+ if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x);
> else
> I915_WRITE(DVSLINOFF(pipe), linear_offset);
>diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
>index 3903dbcda763..911e4690d36a 100644
>--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
>+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
>@@ -40,7 +40,7 @@ enum mdp4_frame_format mdp4_get_frame_format(struct drm_framebuffer *fb)
> {
> bool is_tile = false;
>
>- if (fb->modifier[1] == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE)
>+ if (fb->modifier == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE)
> is_tile = true;
>
> if (fb->pixel_format == DRM_FORMAT_NV12 && is_tile)
>diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
>index f5ae1f436a4b..b3141a0e609b 100644
>--- a/include/drm/drm_framebuffer.h
>+++ b/include/drm/drm_framebuffer.h
>@@ -149,12 +149,12 @@ struct drm_framebuffer {
> */
> unsigned int offsets[4];
> /**
>- * @modifier: Data layout modifier, per buffer. This is used to describe
>+ * @modifier: Data layout modifier. This is used to describe
> * tiling, or also special layouts (like compression) of auxiliary
> * buffers. For userspace created object this is copied from
> * drm_mode_fb_cmd2.
> */
>- uint64_t modifier[4];
>+ uint64_t modifier;
> /**
> * @width: Logical width of the visible area of the framebuffer, in
> * pixels.
>diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>index 01000c9f7c2c..7620bb15ec4f 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -414,17 +414,20 @@ struct drm_mode_fb_cmd2 {
> * offsets[1]. Note that offsets[0] will generally
> * be 0 (but this is not required).
> *
>- * To accommodate tiled, compressed, etc formats, a per-plane
>+ * To accommodate tiled, compressed, etc formats, a
> * modifier can be specified. The default value of zero
> * indicates "native" format as specified by the fourcc.
>- * Vendor specific modifier token. This allows, for example,
>- * different tiling/swizzling pattern on different planes.
>- * See discussion above of DRM_FORMAT_MOD_xxx.
>+ * Vendor specific modifier token. Note that even though
>+ * it looks like we have a modifier per-plane, we in fact
>+ * do not. The modifier for each plane must be identical.
>+ * Thus all combinations of different data layouts for
>+ * multi plane formats must be enumerated as separate
>+ * modifiers.
> */
> __u32 handles[4];
> __u32 pitches[4]; /* pitch for each plane */
> __u32 offsets[4]; /* offset of each plane */
>- __u64 modifier[4]; /* ie, tiling, compressed (per plane) */
>+ __u64 modifier[4]; /* ie, tiling, compress */
> };
>
> #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
>--
>2.7.4
>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 11:33 [RFC][PATCH] drm: Nuke modifier[1-3] ville.syrjala
2016-11-16 12:10 ` Eric Engestrom
2016-11-16 15:30 ` Ben Widawsky
@ 2016-11-16 15:36 ` Daniel Stone
2016-11-17 10:59 ` Daniel Vetter
3 siblings, 0 replies; 10+ messages in thread
From: Daniel Stone @ 2016-11-16 15:36 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Daniel Vetter, dczaplejewicz, Kristian Høgsberg, dri-devel,
Ben Widawsky
Hi,
On 16 November 2016 at 11:33, <ville.syrjala@linux.intel.com> wrote:
> It has been suggested that having per-plane modifiers is making life
> more difficult for userspace, so let's just retire modifier[1-3] and
> use modifier[0] to apply to the entire framebuffer.
>
> Obviosuly this means that if individual planes need different tiling
> layouts and whatnot we will need a new modifier for each combination
> of planes with different tiling layouts.
>
> For a bit of extra backwards compatilbilty the kernel will allow
> non-zero modifier[1+] but it require that they will match modifier[0].
> This in case there's existing userspace out there that sets
> modifier[1+] to something non-zero with planar formats.
This doesn't particularly affect Wayland or EGL either way for import:
we already need to store fd/offset/stride separately for every plane,
so holding a modifier too isn't any increase in complexity. It does
affect advertisement and negotiation though. I'll prepare some
clarifying wording for the EGL spec, to clarify that the modifier must
be equal for all planes.
Acked-by: Daniel Stone <daniels@collabora.com>
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC][PATCH] drm: Nuke modifier[1-3]
2016-11-16 11:33 [RFC][PATCH] drm: Nuke modifier[1-3] ville.syrjala
` (2 preceding siblings ...)
2016-11-16 15:36 ` Daniel Stone
@ 2016-11-17 10:59 ` Daniel Vetter
3 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2016-11-17 10:59 UTC (permalink / raw)
To: ville.syrjala
Cc: Daniel Vetter, Kristian Høgsberg, dri-devel, dczaplejewicz,
Ben Widawsky
On Wed, Nov 16, 2016 at 01:33:16PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> It has been suggested that having per-plane modifiers is making life
> more difficult for userspace, so let's just retire modifier[1-3] and
> use modifier[0] to apply to the entire framebuffer.
>
> Obviosuly this means that if individual planes need different tiling
> layouts and whatnot we will need a new modifier for each combination
> of planes with different tiling layouts.
>
> For a bit of extra backwards compatilbilty the kernel will allow
> non-zero modifier[1+] but it require that they will match modifier[0].
> This in case there's existing userspace out there that sets
> modifier[1+] to something non-zero with planar formats.
>
> Mostly a cocci job, with a bit of manual stuff mixed in.
>
> @@
> struct drm_framebuffer *fb;
> expression E;
> @@
> - fb->modifier[E]
> + fb->modifier
>
> @@
> struct drm_framebuffer fb;
> expression E;
> @@
> - fb.modifier[E]
> + fb.modifier
>
> Cc: Kristian Høgsberg <hoegsberg@gmail.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> Cc: dczaplejewicz@collabora.co.uk
> Suggested-by: Kristian Høgsberg <hoegsberg@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Applied to drm-misc, thx.
-Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 2 +-
> drivers/gpu/drm/drm_framebuffer.c | 7 +++
> drivers/gpu/drm/drm_modeset_helper.c | 2 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> drivers/gpu/drm/i915/intel_atomic_plane.c | 4 +-
> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++----------------
> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 22 +++++-----
> drivers/gpu/drm/i915/intel_sprite.c | 14 +++---
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 2 +-
> include/drm/drm_framebuffer.h | 4 +-
> include/uapi/drm/drm_mode.h | 13 +++---
> 12 files changed, 79 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 57e0a6e96f6d..bfaa6e4a9989 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -922,12 +922,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>
> drm_printf(p, "\t\tformat=%s\n",
> drm_get_format_name(fb->pixel_format, &format_name));
> + drm_printf(p, "\t\t\tmodifier=0x%llx\n", fb->modifier);
> drm_printf(p, "\t\tsize=%dx%d\n", fb->width, fb->height);
> drm_printf(p, "\t\tlayers:\n");
> for (i = 0; i < n; i++) {
> drm_printf(p, "\t\t\tpitch[%d]=%u\n", i, fb->pitches[i]);
> drm_printf(p, "\t\t\toffset[%d]=%u\n", i, fb->offsets[i]);
> - drm_printf(p, "\t\t\tmodifier[%d]=0x%llx\n", i, fb->modifier[i]);
> }
> }
> drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 06ad3d1350c4..cbf0c893f426 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -177,6 +177,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> return -EINVAL;
> }
>
> + if (r->flags & DRM_MODE_FB_MODIFIERS &&
> + r->modifier[i] != r->modifier[0]) {
> + DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> + r->modifier[i], i);
> + return -EINVAL;
> + }
> +
> /* modifier specific checks: */
> switch (r->modifier[i]) {
> case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 2f452b3dd40e..633355e02398 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -93,8 +93,8 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
> for (i = 0; i < 4; i++) {
> fb->pitches[i] = mode_cmd->pitches[i];
> fb->offsets[i] = mode_cmd->offsets[i];
> - fb->modifier[i] = mode_cmd->modifier[i];
> }
> + fb->modifier = mode_cmd->modifier[0];
> fb->pixel_format = mode_cmd->pixel_format;
> fb->flags = mode_cmd->flags;
> }
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1cc971cb6cb1..ae136af4aa37 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1897,7 +1897,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
> fbdev_fb->base.height,
> fbdev_fb->base.depth,
> fbdev_fb->base.bits_per_pixel,
> - fbdev_fb->base.modifier[0],
> + fbdev_fb->base.modifier,
> drm_framebuffer_read_refcount(&fbdev_fb->base));
> describe_obj(m, fbdev_fb->obj);
> seq_putc(m, '\n');
> @@ -1915,7 +1915,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
> fb->base.height,
> fb->base.depth,
> fb->base.bits_per_pixel,
> - fb->base.modifier[0],
> + fb->base.modifier,
> drm_framebuffer_read_refcount(&fb->base));
> describe_obj(m, fb->obj);
> seq_putc(m, '\n');
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index ff821649486e..dbe9fb41ae53 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -144,8 +144,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> if (state->fb && drm_rotation_90_or_270(state->rotation)) {
> struct drm_format_name_buf format_name;
>
> - if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> - state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> + if (state->fb->modifier != I915_FORMAT_MOD_Y_TILED &&
> + state->fb->modifier != I915_FORMAT_MOD_Yf_TILED) {
> DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> return -EINVAL;
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ebb8b833395..0d4884f1d321 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2195,7 +2195,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> - alignment = intel_surf_alignment(dev_priv, fb->modifier[0]);
> + alignment = intel_surf_alignment(dev_priv, fb->modifier);
>
> intel_fill_fb_ggtt_view(&view, fb, rotation);
>
> @@ -2356,13 +2356,13 @@ static u32 intel_adjust_tile_offset(int *x, int *y,
>
> WARN_ON(new_offset > old_offset);
>
> - if (fb->modifier[plane] != DRM_FORMAT_MOD_NONE) {
> + if (fb->modifier != DRM_FORMAT_MOD_NONE) {
> unsigned int tile_size, tile_width, tile_height;
> unsigned int pitch_tiles;
>
> tile_size = intel_tile_size(dev_priv);
> intel_tile_dims(dev_priv, &tile_width, &tile_height,
> - fb->modifier[plane], cpp);
> + fb->modifier, cpp);
>
> if (drm_rotation_90_or_270(rotation)) {
> pitch_tiles = pitch / tile_height;
> @@ -2405,7 +2405,7 @@ static u32 _intel_compute_tile_offset(const struct drm_i915_private *dev_priv,
> unsigned int rotation,
> u32 alignment)
> {
> - uint64_t fb_modifier = fb->modifier[plane];
> + uint64_t fb_modifier = fb->modifier;
> unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, plane);
> u32 offset, offset_aligned;
>
> @@ -2464,7 +2464,7 @@ u32 intel_compute_tile_offset(int *x, int *y,
> if (fb->pixel_format == DRM_FORMAT_NV12 && plane == 1)
> alignment = 4096;
> else
> - alignment = intel_surf_alignment(dev_priv, fb->modifier[plane]);
> + alignment = intel_surf_alignment(dev_priv, fb->modifier);
>
> return _intel_compute_tile_offset(dev_priv, x, y, fb, plane, pitch,
> rotation, alignment);
> @@ -2546,13 +2546,13 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> DRM_ROTATE_0, tile_size);
> offset /= tile_size;
>
> - if (fb->modifier[i] != DRM_FORMAT_MOD_NONE) {
> + if (fb->modifier != DRM_FORMAT_MOD_NONE) {
> unsigned int tile_width, tile_height;
> unsigned int pitch_tiles;
> struct drm_rect r;
>
> intel_tile_dims(dev_priv, &tile_width, &tile_height,
> - fb->modifier[i], cpp);
> + fb->modifier, cpp);
>
> rot_info->plane[i].offset = offset;
> rot_info->plane[i].stride = DIV_ROUND_UP(fb->pitches[i], tile_width * cpp);
> @@ -2711,7 +2711,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> mode_cmd.width = fb->width;
> mode_cmd.height = fb->height;
> mode_cmd.pitches[0] = fb->pitches[0];
> - mode_cmd.modifier[0] = fb->modifier[0];
> + mode_cmd.modifier[0] = fb->modifier;
> mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
>
> if (intel_framebuffer_init(dev, to_intel_framebuffer(fb),
> @@ -2841,7 +2841,7 @@ static int skl_max_plane_width(const struct drm_framebuffer *fb, int plane,
> {
> int cpp = drm_format_plane_cpp(fb->pixel_format, plane);
>
> - switch (fb->modifier[plane]) {
> + switch (fb->modifier) {
> case DRM_FORMAT_MOD_NONE:
> case I915_FORMAT_MOD_X_TILED:
> switch (cpp) {
> @@ -2872,7 +2872,7 @@ static int skl_max_plane_width(const struct drm_framebuffer *fb, int plane,
> }
> break;
> default:
> - MISSING_CASE(fb->modifier[plane]);
> + MISSING_CASE(fb->modifier);
> }
>
> return 2048;
> @@ -2900,7 +2900,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> intel_add_fb_offsets(&x, &y, plane_state, 0);
> offset = intel_compute_tile_offset(&x, &y, plane_state, 0);
>
> - alignment = intel_surf_alignment(dev_priv, fb->modifier[0]);
> + alignment = intel_surf_alignment(dev_priv, fb->modifier);
>
> /*
> * AUX surface offset is specified as the distance from the
> @@ -2917,7 +2917,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> *
> * TODO: linear and Y-tiled seem fine, Yf untested,
> */
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED) {
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED) {
> int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>
> while ((x + w) * cpp > fb->pitches[0]) {
> @@ -3066,7 +3066,7 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
> }
>
> if (INTEL_GEN(dev_priv) >= 4 &&
> - fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + fb->modifier == I915_FORMAT_MOD_X_TILED)
> dspcntr |= DISPPLANE_TILED;
>
> if (rotation & DRM_ROTATE_180)
> @@ -3177,7 +3177,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
> BUG();
> }
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> dspcntr |= DISPPLANE_TILED;
>
> if (rotation & DRM_ROTATE_180)
> @@ -3287,9 +3287,9 @@ u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
> if (drm_rotation_90_or_270(rotation)) {
> int cpp = drm_format_plane_cpp(fb->pixel_format, plane);
>
> - stride /= intel_tile_height(dev_priv, fb->modifier[0], cpp);
> + stride /= intel_tile_height(dev_priv, fb->modifier, cpp);
> } else {
> - stride /= intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> + stride /= intel_fb_stride_alignment(dev_priv, fb->modifier,
> fb->pixel_format);
> }
>
> @@ -3405,7 +3405,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
> PLANE_CTL_PIPE_CSC_ENABLE;
>
> plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> - plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
> + plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> plane_ctl |= skl_plane_ctl_rotation(rotation);
>
> @@ -8731,7 +8731,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> if (INTEL_INFO(dev)->gen >= 4) {
> if (val & DISPPLANE_TILED) {
> plane_config->tiling = I915_TILING_X;
> - fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
> + fb->modifier = I915_FORMAT_MOD_X_TILED;
> }
> }
>
> @@ -8760,7 +8760,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>
> aligned_height = intel_fb_align_height(dev, fb->height,
> fb->pixel_format,
> - fb->modifier[0]);
> + fb->modifier);
>
> plane_config->size = fb->pitches[0] * aligned_height;
>
> @@ -9774,17 +9774,17 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> tiling = val & PLANE_CTL_TILED_MASK;
> switch (tiling) {
> case PLANE_CTL_TILED_LINEAR:
> - fb->modifier[0] = DRM_FORMAT_MOD_NONE;
> + fb->modifier = DRM_FORMAT_MOD_NONE;
> break;
> case PLANE_CTL_TILED_X:
> plane_config->tiling = I915_TILING_X;
> - fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
> + fb->modifier = I915_FORMAT_MOD_X_TILED;
> break;
> case PLANE_CTL_TILED_Y:
> - fb->modifier[0] = I915_FORMAT_MOD_Y_TILED;
> + fb->modifier = I915_FORMAT_MOD_Y_TILED;
> break;
> case PLANE_CTL_TILED_YF:
> - fb->modifier[0] = I915_FORMAT_MOD_Yf_TILED;
> + fb->modifier = I915_FORMAT_MOD_Yf_TILED;
> break;
> default:
> MISSING_CASE(tiling);
> @@ -9801,13 +9801,13 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> fb->width = ((val >> 0) & 0x1fff) + 1;
>
> val = I915_READ(PLANE_STRIDE(pipe, 0));
> - stride_mult = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> + stride_mult = intel_fb_stride_alignment(dev_priv, fb->modifier,
> fb->pixel_format);
> fb->pitches[0] = (val & 0x3ff) * stride_mult;
>
> aligned_height = intel_fb_align_height(dev, fb->height,
> fb->pixel_format,
> - fb->modifier[0]);
> + fb->modifier);
>
> plane_config->size = fb->pitches[0] * aligned_height;
>
> @@ -9875,7 +9875,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> if (INTEL_INFO(dev)->gen >= 4) {
> if (val & DISPPLANE_TILED) {
> plane_config->tiling = I915_TILING_X;
> - fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
> + fb->modifier = I915_FORMAT_MOD_X_TILED;
> }
> }
>
> @@ -9904,7 +9904,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>
> aligned_height = intel_fb_align_height(dev, fb->height,
> fb->pixel_format,
> - fb->modifier[0]);
> + fb->modifier);
>
> plane_config->size = fb->pitches[0] * aligned_height;
>
> @@ -11812,7 +11812,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> intel_ring_emit(ring, fb->pitches[0]);
> intel_ring_emit(ring, intel_crtc->flip_work->gtt_offset |
> - intel_fb_modifier_to_tiling(fb->modifier[0]));
> + intel_fb_modifier_to_tiling(fb->modifier));
>
> /* XXX Enabling the panel-fitter across page-flip is so far
> * untested on non-native modes, so ignore it for now.
> @@ -11845,7 +11845,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, MI_DISPLAY_FLIP |
> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> intel_ring_emit(ring, fb->pitches[0] |
> - intel_fb_modifier_to_tiling(fb->modifier[0]));
> + intel_fb_modifier_to_tiling(fb->modifier));
> intel_ring_emit(ring, intel_crtc->flip_work->gtt_offset);
>
> /* Contrary to the suggestions in the documentation,
> @@ -11951,7 +11951,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>
> intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
> intel_ring_emit(ring, fb->pitches[0] |
> - intel_fb_modifier_to_tiling(fb->modifier[0]));
> + intel_fb_modifier_to_tiling(fb->modifier));
> intel_ring_emit(ring, intel_crtc->flip_work->gtt_offset);
> intel_ring_emit(ring, (MI_NOOP));
>
> @@ -11997,7 +11997,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
>
> ctl = I915_READ(PLANE_CTL(pipe, 0));
> ctl &= ~PLANE_CTL_TILED_MASK;
> - switch (fb->modifier[0]) {
> + switch (fb->modifier) {
> case DRM_FORMAT_MOD_NONE:
> break;
> case I915_FORMAT_MOD_X_TILED:
> @@ -12010,7 +12010,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> ctl |= PLANE_CTL_TILED_YF;
> break;
> default:
> - MISSING_CASE(fb->modifier[0]);
> + MISSING_CASE(fb->modifier);
> }
>
> /*
> @@ -12035,7 +12035,7 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc,
>
> dspcntr = I915_READ(reg);
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> dspcntr |= DISPPLANE_TILED;
> else
> dspcntr &= ~DISPPLANE_TILED;
> @@ -12251,7 +12251,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> engine = dev_priv->engine[BCS];
> - if (fb->modifier[0] != old_fb->modifier[0])
> + if (fb->modifier != old_fb->modifier)
> /* vlv: DISPLAY_FLIP fails to change tiling */
> engine = NULL;
> } else if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv)) {
> @@ -12407,7 +12407,7 @@ static bool intel_wm_need_update(struct drm_plane *plane,
> if (!cur->base.fb || !new->base.fb)
> return false;
>
> - if (cur->base.fb->modifier[0] != new->base.fb->modifier[0] ||
> + if (cur->base.fb->modifier != new->base.fb->modifier ||
> cur->base.rotation != new->base.rotation ||
> drm_rect_width(&new->base.src) != drm_rect_width(&cur->base.src) ||
> drm_rect_height(&new->base.src) != drm_rect_height(&cur->base.src) ||
> @@ -15138,7 +15138,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> return -ENOMEM;
> }
>
> - if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
> + if (fb->modifier != DRM_FORMAT_MOD_NONE) {
> DRM_DEBUG_KMS("cursor cannot be tiled\n");
> return -EINVAL;
> }
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index fc958d5ed0dc..beb08982dc0b 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -633,7 +633,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> cur_size = intel_crtc->config->base.adjusted_mode.crtc_vdisplay;
> cur_size = intel_fb_align_height(dev, cur_size,
> fb->base.pixel_format,
> - fb->base.modifier[0]);
> + fb->base.modifier);
> cur_size *= fb->base.pitches[0];
> DRM_DEBUG_KMS("pipe %c area: %dx%d, bpp: %d, size: %d\n",
> pipe_name(intel_crtc->pipe),
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1331bcc41868..572919775ece 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3065,7 +3065,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> latency = dev_priv->wm.skl_latency[level];
>
> if (skl_needs_memory_bw_wa(intel_state) &&
> - plane->base.state->fb->modifier[0] ==
> + plane->base.state->fb->modifier ==
> I915_FORMAT_MOD_X_TILED)
> latency += 15;
>
> @@ -3325,8 +3325,8 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
> return 0;
>
> /* For Non Y-tile return 8-blocks */
> - if (fb->modifier[0] != I915_FORMAT_MOD_Y_TILED &&
> - fb->modifier[0] != I915_FORMAT_MOD_Yf_TILED)
> + if (fb->modifier != I915_FORMAT_MOD_Y_TILED &&
> + fb->modifier != I915_FORMAT_MOD_Yf_TILED)
> return 8;
>
> src_w = drm_rect_width(&intel_pstate->base.src) >> 16;
> @@ -3595,7 +3595,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> return 0;
> }
>
> - if (apply_memory_bw_wa && fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (apply_memory_bw_wa && fb->modifier == I915_FORMAT_MOD_X_TILED)
> latency += 15;
>
> width = drm_rect_width(&intel_pstate->base.src) >> 16;
> @@ -3634,12 +3634,12 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> y_min_scanlines *= 2;
>
> plane_bytes_per_line = width * cpp;
> - if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> - fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> + if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> + fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
> plane_blocks_per_line /= y_min_scanlines;
> - } else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) {
> + } else if (fb->modifier == DRM_FORMAT_MOD_NONE) {
> plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
> + 1;
> } else {
> @@ -3654,8 +3654,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>
> y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> - fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> + if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> + fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> selected_result = max(method2, y_tile_minimum);
> } else {
> if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
> @@ -3671,8 +3671,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> res_lines = DIV_ROUND_UP(selected_result, plane_blocks_per_line);
>
> if (level >= 1 && level <= 7) {
> - if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> - fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> + if (fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> + fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> res_blocks += y_tile_minimum;
> res_lines += y_min_scanlines;
> } else {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c2b629c922e7..8f131a08d440 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -224,7 +224,7 @@ skl_update_plane(struct drm_plane *drm_plane,
> PLANE_CTL_PIPE_CSC_ENABLE;
>
> plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> - plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
> + plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
>
> plane_ctl |= skl_plane_ctl_rotation(rotation);
>
> @@ -406,7 +406,7 @@ vlv_update_plane(struct drm_plane *dplane,
> */
> sprctl |= SP_GAMMA_ENABLE;
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> sprctl |= SP_TILED;
>
> if (rotation & DRM_ROTATE_180)
> @@ -448,7 +448,7 @@ vlv_update_plane(struct drm_plane *dplane,
> I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
> I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
> else
> I915_WRITE(SPLINOFF(pipe, plane), linear_offset);
> @@ -531,7 +531,7 @@ ivb_update_plane(struct drm_plane *plane,
> */
> sprctl |= SPRITE_GAMMA_ENABLE;
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> sprctl |= SPRITE_TILED;
>
> if (rotation & DRM_ROTATE_180)
> @@ -584,7 +584,7 @@ ivb_update_plane(struct drm_plane *plane,
> * register */
> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> I915_WRITE(SPROFFSET(pipe), (y << 16) | x);
> - else if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + else if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> else
> I915_WRITE(SPRLINOFF(pipe), linear_offset);
> @@ -669,7 +669,7 @@ ilk_update_plane(struct drm_plane *plane,
> */
> dvscntr |= DVS_GAMMA_ENABLE;
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> dvscntr |= DVS_TILED;
>
> if (rotation & DRM_ROTATE_180)
> @@ -712,7 +712,7 @@ ilk_update_plane(struct drm_plane *plane,
> I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
> I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>
> - if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> + if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x);
> else
> I915_WRITE(DVSLINOFF(pipe), linear_offset);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> index 3903dbcda763..911e4690d36a 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> @@ -40,7 +40,7 @@ enum mdp4_frame_format mdp4_get_frame_format(struct drm_framebuffer *fb)
> {
> bool is_tile = false;
>
> - if (fb->modifier[1] == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE)
> + if (fb->modifier == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE)
> is_tile = true;
>
> if (fb->pixel_format == DRM_FORMAT_NV12 && is_tile)
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index f5ae1f436a4b..b3141a0e609b 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -149,12 +149,12 @@ struct drm_framebuffer {
> */
> unsigned int offsets[4];
> /**
> - * @modifier: Data layout modifier, per buffer. This is used to describe
> + * @modifier: Data layout modifier. This is used to describe
> * tiling, or also special layouts (like compression) of auxiliary
> * buffers. For userspace created object this is copied from
> * drm_mode_fb_cmd2.
> */
> - uint64_t modifier[4];
> + uint64_t modifier;
> /**
> * @width: Logical width of the visible area of the framebuffer, in
> * pixels.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 01000c9f7c2c..7620bb15ec4f 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -414,17 +414,20 @@ struct drm_mode_fb_cmd2 {
> * offsets[1]. Note that offsets[0] will generally
> * be 0 (but this is not required).
> *
> - * To accommodate tiled, compressed, etc formats, a per-plane
> + * To accommodate tiled, compressed, etc formats, a
> * modifier can be specified. The default value of zero
> * indicates "native" format as specified by the fourcc.
> - * Vendor specific modifier token. This allows, for example,
> - * different tiling/swizzling pattern on different planes.
> - * See discussion above of DRM_FORMAT_MOD_xxx.
> + * Vendor specific modifier token. Note that even though
> + * it looks like we have a modifier per-plane, we in fact
> + * do not. The modifier for each plane must be identical.
> + * Thus all combinations of different data layouts for
> + * multi plane formats must be enumerated as separate
> + * modifiers.
> */
> __u32 handles[4];
> __u32 pitches[4]; /* pitch for each plane */
> __u32 offsets[4]; /* offset of each plane */
> - __u64 modifier[4]; /* ie, tiling, compressed (per plane) */
> + __u64 modifier[4]; /* ie, tiling, compress */
> };
>
> #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
> --
> 2.7.4
>
--
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] 10+ messages in thread