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

[Rob Clark]

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]?

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel