Re: [RFC][PATCH] drm: Nuke modifier[1-3]

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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