[Intel-gfx] [RFC v1 1/7] drm/i915: Add gamma mode property

[Ville Syrjälä]

On Fri, Mar 22, 2019 at 03:42:43PM +0000, Brian Starkey wrote:
> On Fri, Mar 22, 2019 at 04:02:57PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 22, 2019 at 01:39:04PM +0000, Brian Starkey wrote:
> > > Hi,
> > > 
> > > On Fri, Mar 22, 2019 at 01:06:01PM +0000, Shankar, Uma wrote:
> > > > 
> > > > 
> > > > >-----Original Message-----
> > > > >From: Roper, Matthew D
> > > > >Sent: Friday, March 22, 2019 2:50 AM
> > > > >To: Shankar, Uma <uma.shankar at intel.com>
> > > > >Cc: intel-gfx at lists.freedesktop.org; Lankhorst, Maarten
> > > > ><maarten.lankhorst at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>; Sharma,
> > > > >Shashank <shashank.sharma at intel.com>
> > > > >Subject: Re: [RFC v1 1/7] drm/i915: Add gamma mode property
> > > > >
> > > > >On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> > > > >> Gen platforms support multiple gamma modes, currently it's hard coded
> > > > >> to operate only in 1 specific mode.
> > > > >> This patch adds a property to make gamma mode programmable.
> > > > >> User can select which mode should be used for a particular usecase or
> > > > >> scenario.
> > > > >>
> > > > >> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> > > > >
> > > > >I haven't reviewed the series in depth, but I'm a bit confused on how userspace would
> > > > >approach using this property.  This seems to be exposing hardware implementation
> > > > >details that I wouldn't expect userspace to need to worry about (plus I don't think any
> > > > >of the property values here convey any specific meaning to someone who hasn't read
> > > > >the Intel bspec/PRM).  E.g., why does userspace care about "split gamma" when the
> > > > >driver takes care of the programming details and userspace never sees the actual way
> > > > >the registers are laid out and written?
> > > > >My understanding is that what really matters is how many table entries there are for
> > > > >userspace to fill in, what input range(s) they cover, and how the bits of each table
> > > > >entry are interpreted.  I think we'd want to handle this in a vendor-agnostic way in the
> > > > >DRM core if possible; most of the display servers that get used these days are cross-
> > > > >platform and probably won't want to add Intel-specific logic (or platform-specific
> > > > >logic if we wind up with a different set of options on future Intel platforms).
> > > > 
> > > > Ok, will try to re-structure this to make it vendor agnostic way. Also will add enough
> > > > documentation for the usage of the property. 
> > > > 
> > > > @Ville- What do you recommend or suggest for these interfaces.
> > > 
> > > Just to add to the conversation - some of our HW has fixed LUTs, which
> > > isn't really very well exposed by the current UAPI. We do it by having
> > > the kernel driver just look at the userspace-provided blob, and it if
> > > matches the fixed curve we accept it.
> > 
> > So you just have say "Use the sRGB curve" bit in some register etc.?
> 
> Yep, precisely. In the future, maybe multiple fixed LUTs to choose
> from.
> 
> > 
> > I think we should be able to integrate that somehow into my design:
> > https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c21300ff95                                       
> > 
> > Eg. just add a new DRM_MODE_LUT_FIXED_CURVE flag, and then I suppose
> > just add another member into the struct to indicate which curve it
> > is. And we could embed the fixed blob ID in there as well. That way
> > userspace wouldn't even have to actually get the blob for the curve.
> 
> Yeah that (ENUM | BLOB) API let's us be quite "rich" with the
> interface, so it certainly could fit in there.
> 
> If I understand the code correctly, each value in the enum list is the
> ID of a blob, and userspace can query that blob to figure out what the
> entry means (instead of needing _really really long_ descriptive
> strings).
> 
> I wonder if that property type in general is a little _too_ rich. I
> worry that it would be easy to just defer loads of vendor-specific
> detail into the blob, making the API look "generic" on the surface,
> when really it's effectively a list of vendor-defined (void *)s.
> 
> ...that said, in some cases vendor-defined (void *)s is what we might
> want (e.g. scaler coefficient tables).
> 
> Still looks like a neat idea, perhaps just needs some policy.

Yeah, I couldn't come up with anything better really. The options that
I see are as you say really long descriptive string, or we'd have to
update all userspace whenever a new enum value is added so that it
can decide what to do with it.

If we go with this idea, then I would definitely want to NAK any patch
that tries to abuse this for "we just need to expose these random
piles of hardware specific data".

-- 
Ville Syrjälä
Intel