Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Ville Syrjälä]

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

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

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. 

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 ? 

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. 
 
Regards
Shashank    

-----Original Message-----
From: Ville Syrjälä [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.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

Re: [PATCH 0/6] Intel Color Manager Framework

[Ville Syrjälä]

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@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.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

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

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.
 
Please correct me if any of my assumptions are not right, or not feasible, or if I am just a moron :) .
    
Regards
Shashank 
-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Friday, February 21, 2014 2:47 PM
To: Sharma, Shashank
Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@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@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.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

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Ville Syrjälä]

On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
> 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.

>  
> 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?

>     
> Regards
> Shashank 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Friday, February 21, 2014 2:47 PM
> To: Sharma, Shashank
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@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@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.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

Re: [PATCH 0/6] Intel Color Manager Framework

[Alex Deucher]

On Fri, Feb 21, 2014 at 9:46 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
>> 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.
>

Our hardware has similar features, so I'm sure there will be quite a
bit of common ground.  I also vote for properties.

Alex

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3285 bytes --]

On Fri, Feb 21, 2014 at 10:41:14AM -0500, Alex Deucher wrote:
> On Fri, Feb 21, 2014 at 9:46 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
> >> 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.
> >
> 
> Our hardware has similar features, so I'm sure there will be quite a
> bit of common ground.  I also vote for properties.

Thirded. Tegra should be able to use a hardware-agnostic description of
these as well. I wonder if perhaps VESA or some other standard already
defines such a format for some of these properties.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

>> 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@linux.intel.com]
> Sent: Friday, February 21, 2014 2:47 PM
> To: Sharma, Shashank
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; 
> dri-devel@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@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.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

Re: [PATCH 0/6] Intel Color Manager Framework

[Rob Clark]

On Fri, Feb 21, 2014 at 9:20 AM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> 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.

What I would suggest is re-form the userspace facing API to be
properties.. if needed we can add a setblobprop ioctl (Sean Paul was,
I think, adding that already).  Then userspace and use setprop ioctls
for now, and optionally atomic ioctl later when it is in place.  No
reason for it to be blocked waiting for atomic.

btw, I didn't look into the patches yet, but full-nak on idea of
exposing via sysfs.  This should be the sort of thing that is set by
the process that has mastership on the drm device, which we can't
enforce via sysfs.  Using properties seems like the way to go.

BR,
-R

> 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.

Re: [PATCH 0/6] Intel Color Manager Framework

[Sean Paul]

On Fri, Feb 21, 2014 at 9:49 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Fri, Feb 21, 2014 at 9:20 AM, Sharma, Shashank
> <shashank.sharma@intel.com> wrote:
>> 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.
>
> What I would suggest is re-form the userspace facing API to be
> properties.. if needed we can add a setblobprop ioctl (Sean Paul was,
> I think, adding that already).

This is news to me ;-)

I'm pulling the atomic stuff into the CrOS tree, but I think Rahul
Sharma is the guy you're looking for wrt setblobprop
(http://www.spinics.net/lists/dri-devel/msg54010.html).

Sean


> Then userspace and use setprop ioctls
> for now, and optionally atomic ioctl later when it is in place.  No
> reason for it to be blocked waiting for atomic.
>
> btw, I didn't look into the patches yet, but full-nak on idea of
> exposing via sysfs.  This should be the sort of thing that is set by
> the process that has mastership on the drm device, which we can't
> enforce via sysfs.  Using properties seems like the way to go.
>
> BR,
> -R
>
>> 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.

Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

[Stéphane Marchesin]

[-- Attachment #1.1: Type: text/plain, Size: 1980 bytes --]

On Thu, Feb 20, 2014 at 5:11 AM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> 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.
>

+1. We'e looking into hooking up color correction controls, and if the
interface isn't standard our user space won't be portable across drivers.
There are multiple reasons for using drm properties:
- the KMS interface already provides a way to set the gamma ramp, which
this code seems to replicate.
- the KMS interface allows us to name properties independently and
enumerate them. It seems like right now you can't enumerate properties or
guess what "property 0" is. I'd rather set the "Color conversion matrix"
than remember to set "property 0" (and even then, I'm not really sure it
exists).
- you can reuse the get/set infrastructure which is already in place

Another thing that came out of the discussion on irc is that we should
standardize the properties. For example we could use a text file describing
the name of the controls and the format of the data (something similar to
the device tree bindings). That way user space can expect "color conversion
matrix" to mean the same thing everywhere, to get the same data as input,
and to work the same way on all platforms.

Stéphane

[-- Attachment #1.2: Type: text/html, Size: 2564 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

[-- Attachment #1.1: Type: text/plain, Size: 2665 bytes --]

On Thu, Feb 20, 2014 at 5:11 AM, Ville Syrjälä <ville.syrjala@linux.intel.com<mailto:ville.syrjala@linux.intel.com>> wrote:
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.

+1. We'e looking into hooking up color correction controls, and if the interface isn't standard our user space won't be portable across drivers. There are multiple reasons for using drm properties:
- the KMS interface already provides a way to set the gamma ramp, which this code seems to replicate.

The current KMS interface just initializes the gamma soft LUT palette registers, in 8 bit mode corresponding to unit gamma. It's impossible to apply accurate values corresponding to gamma=2.2 or 1.5 from KMS
Because for that we need to program palette registers in 10.6 bit mode of hardware.
>- the KMS interface allows us to name properties independently and enumerate them. It seems like right now you can't enumerate properties or guess what "property 0" is. I'd rather set the "Color conversion matrix" than remember to set >"property 0" (and even then, I'm not really sure it exists).

All the properties are getting enumerated in color manager register function. The framework defines proper identifiers and mapping for each property, and every property is having a corresponding soft-lut to be loaded with correction values.

- you can reuse the get/set infrastructure which is already in place


>Another thing that came out of the discussion on irc is that we should standardize the properties. For example we could use a text file describing the name of the controls and the format of the data (something similar to the device tree >bindings). That way user space can expect "color conversion matrix" to mean the same thing everywhere, to get the same data as input, and to work the same way on all platforms.
If you can please have a look on the header file, we are almost doing the same thing, in form of a protocol.

Stéphane

Regards
Shashank

[-- Attachment #1.2: Type: text/html, Size: 6898 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] Intel Color Manager Framework

[Stéphane Marchesin]

[-- Attachment #1.1: Type: text/plain, Size: 3422 bytes --]

On Fri, Feb 21, 2014 at 7:49 PM, Sharma, Shashank <shashank.sharma@intel.com
> wrote:

>    On Thu, Feb 20, 2014 at 5:11 AM, Ville Syrjälä <
> ville.syrjala@linux.intel.com> wrote:
>
> 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.
>
>
>
> +1. We'e looking into hooking up color correction controls, and if the
> interface isn't standard our user space won't be portable across drivers.
> There are multiple reasons for using drm properties:
>
> - the KMS interface already provides a way to set the gamma ramp, which
> this code seems to replicate.
>
>
>
> The current KMS interface just initializes the gamma soft LUT palette
> registers, in 8 bit mode corresponding to unit gamma. It's impossible to
> apply accurate values corresponding to gamma=2.2 or 1.5 from KMS
>
> Because for that we need to program palette registers in 10.6 bit mode of
> hardware.
>

Then the existing interface should be extended. Otherwise you have two ways
to do the same thing...



>   >- the KMS interface allows us to name properties independently and
> enumerate them. It seems like right now you can't enumerate properties or
> guess what "property 0" is. I'd rather set the "Color conversion matrix"
> than remember to set >"property 0" (and even then, I'm not really sure it
> exists).
>
>
>
> All the properties are getting enumerated in color manager register
> function. The framework defines proper identifiers and mapping for each
> property, and every property is having a corresponding soft-lut to be
> loaded with correction values.
>

Correct me if I'm wrong, but I don't see a way for user space to query the
presence/absence of a given property. KMS allows that.



>
>
> - you can reuse the get/set infrastructure which is already in place
>
>
>
>
>
> >Another thing that came out of the discussion on irc is that we should
> standardize the properties. For example we could use a text file describing
> the name of the controls and the format of the data (something similar to
> the device tree >bindings). That way user space can expect "color
> conversion matrix" to mean the same thing everywhere, to get the same data
> as input, and to work the same way on all platforms.
>
> If you can please have a look on the header file, we are almost doing the
> same thing, in form of a protocol.
>

This protocol however is not extensible. With the KMS interface I can
already do the following from user space:
- query the existence of a given property
- set each property in a portable fashion (for example the same gamma ramp
code works on all DRM drivers)
- easily match properties to a given crtc

Stéphane

[-- Attachment #1.2: Type: text/html, Size: 7067 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] Intel Color Manager Framework

[Sharma, Shashank]

[-- Attachment #1.1: Type: text/plain, Size: 3244 bytes --]

Thanks for your comments Stéphane
Please find mine inline.


In general, I got the overall recommendations that if this implementation comes from generic DRM property, it would be easy to club with general interfaces, and atomic modeset calls.
I will work on this, and will come back with modified patches.

Regards
Shashank
From: Stéphane Marchesin [mailto:stephane.marchesin@gmail.com]
Sent: Monday, February 24, 2014 9:35 AM
To: Sharma, Shashank
Cc: Ville Syrjälä; Intel Graphics Development; Shankar, Uma; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

>+1. We'e looking into hooking up color correction controls, and if the interface isn't standard our user space won't be portable across drivers. There are multiple reasons for using drm properties:
>- the KMS interface already provides a way to set the gamma ramp, which this code seems to replicate.
 >The current KMS interface just initializes the gamma soft LUT palette registers, in 8 bit mode corresponding to unit gamma. It's impossible to apply accurate values corresponding to gamma=2.2 or 1.5 from KMS
>Because for that we need to program palette registers in 10.6 bit mode of hardware.
>Then the existing interface should be extended. Otherwise you have two ways to do the same thing...

Agree.

>- the KMS interface allows us to name properties independently and enumerate them. It seems like right now you can't enumerate properties or guess what "property 0" is. I'd rather set the "Color conversion matrix" than remember to set >"property 0" (and even then, I'm not really sure it exists).

All the properties are getting enumerated in color manager register function. The framework defines proper identifiers and mapping for each property, and every property is having a corresponding soft-lut to be loaded with correction values.
Correct me if I'm wrong, but I don't see a way for user space to query the presence/absence of a given property. KMS allows that.
The color manager read function dumps the no of properties, and current status of the property. But I agree its better interface to have it in form of property, as far as the central control is concerned.

- you can reuse the get/set infrastructure which is already in place


>Another thing that came out of the discussion on irc is that we should standardize the properties. For example we could use a text file describing the name of the controls and the format of the data (something similar to the device tree >bindings). That way user space can expect "color conversion matrix" to mean the same thing everywhere, to get the same data as input, and to work the same way on all platforms.
If you can please have a look on the header file, we are almost doing the same thing, in form of a protocol.
>This protocol however is not extensible. With the KMS interface I can already do the following from user space:
>- query the existence of a given property
>- set each property in a portable fashion (for example the same gamma ramp code works on all DRM drivers)
>- easily match properties to a given crtc

Actually each of this is possible from color manager read/write, read dumps information per pipe basis.



[-- Attachment #1.2: Type: text/html, Size: 11362 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx