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@xxxxxxxxx> > > > > >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten > > > > ><maarten.lankhorst@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Sharma, > > > > >Shashank <shashank.sharma@xxxxxxxxx> > > > > >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@xxxxxxxxx> > > > > > > > > > >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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
