From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 0/6] Intel Color Manager Framework Date: Fri, 21 Feb 2014 11:17:06 +0200 Message-ID: <20140221091706.GJ3852@intel.com> References: <1392899847-2641-1-git-send-email-shashank.sharma@intel.com> <20140220131129.GG3852@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: "Sharma, Shashank" Cc: "intel-gfx@lists.freedesktop.org" , "Shankar, Uma" , "dri-devel@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org 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 on= e of our common forum with OTC folks. = > We discussed, took review comments, and re-designed the framework, as pe= r 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 /s= ysfs 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 w= ork on making hardware agnostic, as you recommended. = > = > IMHO, controls from /sysfs would be a very generic interface for all linu= x/drm based platform, where any userspace can read/write and control proper= ties. = > We don't even need a UI manager or a minimum executable to play around, j= ust 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=E4l=E4 [mailto:ville.syrjala@linux.intel.com] = > Sent: Thursday, February 20, 2014 6:41 PM > To: Sharma, Shashank > Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freede= sktop.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 pla= n is to use kms properties for this kind of stuff which allows us to hook i= t up with the upcoming atomic modeset API. Just yesterday there was some di= scussion on #dri-devel about exposing user settable blob properties even be= fore the atomic modeset API lands (it was always the plan for the atomic mo= deset 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 use= rspace could use them regardless of the hardware/driver. Obviously that mig= ht not be possible in all cases, but we should at least spend a bit of effo= rt on trying to make that happen for most properties. > = > -- > Ville Syrj=E4l=E4 > Intel OTC -- = Ville Syrj=E4l=E4 Intel OTC