Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

From: Rob Clark <robdclark@gmail.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>,
	Intel GFX <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2
Date: Thu, 12 Jan 2017 09:56:39 -0500	[thread overview]
Message-ID: <CAF6AEGuxCzOrtz=kZ6K0mJWEfxPaeELA4V=S5LBstAKKHciAnw@mail.gmail.com> (raw)
In-Reply-To: <20170112093820.GQ31595@intel.com>

On Thu, Jan 12, 2017 at 4:38 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Jan 11, 2017 at 08:43:16PM -0500, Rob Clark wrote:
>> On Wed, Jan 11, 2017 at 7:51 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> >
>> > +struct drm_format_modifier {
>> > +       /* Bitmask of formats in get_plane format list this info
>> > +        * applies to. */
>> > +       uint64_t formats;
>>
>> re: the uabi, I'd suggest to at least make this 'u32 offset; u32
>> formats'.. we can keep the existing implementation in this patch and
>> always set 'offset' to zero, and let the first one to hit more than 32
>> formats deal with the implementation.  (Maybe a strategically placed
>> WARN_ON() if you go that route..)
>
> Isn't an implicit offset enough? As in first mask for a specific
> modifier is for format indexes 0-63, second mask for the same modifier
> is for 64-127, and so on.

hmm, hadn't thought of that approach.  Definitely if we go w/ implicit
then we want to have userspace support from the get-go.  For explicit,
I guess userspace could complain and ignore if it saw a non-zero
offset similar to what we do w/ pad and unknown flags in the other
direction?

Not sure if that would fly or not..  I guess it is not a *critical*
fail, it just means userspace won't realize that some modifiers are
supported on some formats.. otoh the implicit approach could confuse a
userspace that didn't realize the offset into thinking modifiers
*were* supported on formats where they are not.. that seems like a
bigger problem.

BR,
-R

> The bigger issue is the userspace side I think. If we don't add the
> userspace side code to handle this case from the get go, it's going to
> be hard to actually start doing it from the kernel side.
>
>>
>> Otherwise I guess it is just a couple years until getplane3 ;-)
>>
>> BR,
>> -R
>>
>> > +
>> > +       /* This modifier can be used with the format for this plane. */
>> > +       uint64_t modifier;
>> > +};
>> _______________________________________________
>> 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