[RFC][PATCH] drm: Nuke modifier[1-3]
Daniel Vetter
daniel at ffwll.ch
Wed Nov 16 14:54:03 UTC 2016
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 at 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 at linux.intel.com wrote:
> > >> > From: Ville Syrjälä <ville.syrjala at 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 at gmail.com>
> > >> > Cc: Ben Widawsky <benjamin.widawsky at intel.com>
> > >> > Cc: Rob Clark <robdclark at gmail.com>
> > >> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > >> > Cc: Tomeu Vizoso <tomeu at tomeuvizoso.net>
> > >> > Cc: dczaplejewicz at collabora.co.uk
> > >> > Suggested-by: Kristian Høgsberg <hoegsberg at gmail.com>
> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at 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
More information about the dri-devel
mailing list