>> Hi Ville, >> >> Thanks for your time and comments. >> I can understand two basic problems what you see in this implementation: >> >> 1. The most important issue from my POV is that it can't be part of the >> atomic modeset. >> 2. it make the whole API inconsistent. >> >> I am not sure if its good to block all current implementation because we >> have thought something for this in atomic modeset. >> I think even in atomic modeset we need the core implementation like this, >> but the interface would be different, which might come in from of a DRM >> property. >> So at that time we can use this core implementation as it is, only the >> interfaces/framework needs to be changed. >> >> In this way we can always go ahead with a current implementation, and can >> just change the interfaces to fit in to the final interface like DRM >> property in atomic modeset. >> Or you can suggest us the expected interface, and we can work on modifying >> that as per expectation.
>The exptected interface will be range properties for stuff like brightness, >contrast etc. controls. There are already such things as connector properties, >but we're going to want something similar as plane or crtc properties. One >thing that worries me about >such properties though is whether we can make >them hardware agnostic and yet allow userspace precise control over the final >image. That is, if we map some fixed input range to a hardware specific output >range, userspace can't know how the actual >output will change when the input >changes. On the other hand if the input is hardware specific, userspace can't >know what value to put in there to get the expected change on the output side. >For bigger stuff like CSC matrices and gamma ramps we will want to use some >reasonably well defined blobs. Ie. the internal strucuture of the blob has to >be documented and it shouldn't contain more than necessary. >Ie. just the CSC matrix coefficients for one matrix, or just the entries for a >single gamma ramp. Again ideally we should make the blobs hardware agnostic, >but still allow precise control over the output data. >I think this is going to involve first going over our hardware features, >trying to find the common patterns between different generations. If there's a >way to make something that works across the board for us, or at least across a >wide range, then we >should also ask for some input on dri-devel whether the >proposed property would work for other people. We may need to define new >property types to more precisely define what the value of the property >actually means. Actually this is what we had done, but we just picked a wrong interface. The reason behind picking sysfs was that we were worried about the increasing no of IOCTL getting listed. We just created a superset of all required inputs for different properties, and then defined a data protocol (color EDID). >> Please correct me if any of my assumptions are not right, or not feasible, >> or if I am just a moron :) . >The implementation itself has to be tied into the pipe config (and eventual >plane config) stuff, which are the structures meant to house the full device >state, which will then be applied in one go. >At the moment it looks like you're writing a bunch of registers from various >places w/o much thought into how those things interact with anything else. For >instance, what's the story with your use of the pipe CSC unit vs. the already >existing "Broadcast >RGB" property? If you have a close look at the header, We are already defining a pipe status map, which at any time tells you, what's the color status of the pipe, just as an independent implementation, instead of a DRM property. As you already know, there is no relation between DRM property and this implementation, we are not doing anything there. Probably, I will spend some more time in how can I club this framework in DRM property, and re-implement the patch accordingly, and come back. At that time, as you suggested, I can take inputs from dri-devel for the actual implementation. Regards Shashank > -----Original Message----- > From: Ville Syrj?l? [mailto:ville.syrjala at linux.intel.com] > Sent: Friday, February 21, 2014 2:47 PM > To: Sharma, Shashank > Cc: intel-gfx at lists.freedesktop.org; Shankar, Uma; > dri-devel at lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework > > On Fri, Feb 21, 2014 at 03:34:43AM +0000, Sharma, Shashank wrote: > > Hi Ville/All, > > > > We gave a presentation on design on this framework, few months ago, in one > > of our common forum with OTC folks. > > We discussed, took review comments, and re-designed the framework, as per > > the feedbacks. > > Apparently I wasn't there. And in any case it would be better to discuss it > on dri-devel where people outside Intel can give their opinion. > > > > > We also discussed the benefits of providing the controls directly from > > /sysfs over going for a UI manager based property settings. > > So I don't understand where are we going wrong, can you please elaborate a > > bit ? > > The most important issue from my POV is that it can't be part of the atomic > modeset. > > Another issue is that it make the whole API inconsistent. Some stuff through > ioctl, some stuff through sysfs, some stuff through whatever the next guy > thinks of. It's not pretty. I've worked in the past with a driver where I had > to poke at various standardish ioctls, custom ioctls, and sysfs to make it do > anything useful, and I have no interest in repeating that experience. sysfs > is especially painful since you have do the string<->binary conversions all > over the place, and also you en up doing open+read/write+close cycles for > every little thing. > > It also adds more entrypoints into the driver for us to worry about. > That means extra worries about the power management stuff and locking at the > very least. > > Also the rules of sysfs say "one item per file". The only allowed exception > to this rule I know of is hardware provided blobs (like EDID, PCI ROM etc.). > Your current implementation breaks this rule blatantly. > > > > > This is just a basic design, and once go ahead with this, we can always > > work on making hardware agnostic, as you recommended. > > > > IMHO, controls from /sysfs would be a very generic interface for all > > linux/drm based platform, where any userspace can read/write and control > > properties. > > We don't even need a UI manager or a minimum executable to play > > around, just a small script can do. But we can always write something on > > top of this, to be included in any UI framework or property. > > If there's a real need to get at properties through sysfs, then we could > think about exposing them all. But that may presents some issues where the > current master suddenly gets confused about its state since someone else went > behind its back and changed a bunch of stuff. > > > > > Regards > > Shashank > > > > -----Original Message----- > > From: Ville Syrj?l? [mailto:ville.syrjala at linux.intel.com] > > Sent: Thursday, February 20, 2014 6:41 PM > > To: Sharma, Shashank > > Cc: intel-gfx at lists.freedesktop.org; Shankar, Uma; > > dri-devel at lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework > > > > On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote: > > > Color manager is a new framework in i915 driver, which provides a > > > unified interface for various color correction methods supported > > > by intel hardwares. The high level overview of this change is: > > > > Would have been good to discuss this idea before implementing it. The plan > > is to use kms properties for this kind of stuff which allows us to hook it > > up with the upcoming atomic modeset API. Just yesterday there was some > > discussion on #dri-devel about exposing user settable blob properties even > > before the atomic modeset API lands (it was always the plan for the atomic > > modeset API anyway). So based on a cursory glance, this looks like it's > > going in the wrong direction. > > > > Also ideally the properties should be hardware agnostic, so a generic > > userspace could use them regardless of the hardware/driver. Obviously that > > might not be possible in all cases, but we should at least spend a bit of > > effort on trying to make that happen for most properties. > > > > -- > > Ville Syrj?l? > > Intel OTC > > -- > Ville Syrj?l? > Intel OTC -- Ville Syrj?l? Intel OTC
