On Wed, Nov 16, 2016 at 9:43 AM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx wrote:
>> >> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> >
>> >> > 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@xxxxxxxxx>
>> >> > Cc: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
>> >> > Cc: Rob Clark <robdclark@xxxxxxxxx>
>> >> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> >> > Cc: Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx>
>> >> > Cc: dczaplejewicz@xxxxxxxxxxxxxxx
>> >> > Suggested-by: Kristian Høgsberg <hoegsberg@xxxxxxxxx>
>> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> > ---
>> >> > 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@xxxxxxxxxxxxxxxxxxxxx
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
