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

[Brian Starkey]

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.

Thanks,
-Brian

> 
> -- 
> Ville Syrjälä
> Intel