[Intel-gfx] Design review request: DRM color manager
Daniel Vetter
daniel at ffwll.ch
Mon May 12 10:51:00 CEST 2014
Re-adding dri-devel, all drm core stuff must be discussed there.
But on the actual issue at hand I still don't understand what you're
trying to solve. You add a complete new set of properties, using Intel
names (pipes, planes) for some attributes which at first seems
completely redundant to all the properties we already have.
What's the technical reasons for adding this interface? Your proposal
is only describing how it works, so is lacking this crucial
information.
-Daniel
On Wed, May 7, 2014 at 4:22 PM, Sharma, Shashank
<shashank.sharma at intel.com> wrote:
> Hello all,
>
> As per previous discussions, I am sending the drm-color-manager design for
> review, as inline text. Please have a look and let me know your feedback.
>
> (I tried hard to align the inline text diagram in mail, hope you can see the
> proper picture too)
>
> =================
> DRM Color Manager
> =================
>
> - Color manager provides a common interface for all color correction /
> enhancement properties supported by various hardwares.
> - Color manager will be one Umbrella DRM property (blob type)
> - Driver can register the color correction properties of its HW during
> the init time, and all the corrections can be applied using the same
> interface.
>
> How does a driver register color properties with DRM color manager:
> -------------------------------------------------------------------
>
> - A DRM driver will call drm_register_color_manager() function with
> following information:
> .prop_set_handler
> .prop_get_hanlder
> .color_prop_identifier structure
> {porp_name, prop_id}
> {porp_name, prop_id}
> {porp_name, prop_id}
>
> For example: I915 driver can register as:
> .prop_set_handler = intel_clrmgr_set()
> .prop_get_hanlder = intel_clrmgr_get()
> .color_prop_identifier structure
> {gamma_correction, 0}
> {csc_correction, 1}
> {hue_saturation, 2}
>
>
>
> How will color_manager_set/get() functions work:
> -------------------------------------------------
>
> Color EDID:
> ======================================================================
> || property || Enable/ || Pipe/Plane/ || No of ||
> || ID || Disable || Identifier || Data bytes ||data..
> || <1 Byte> || <1 Byte> || <1 Byte> || <1 Byte> ||
> ======================================================================
>
> - Color EDID is a protocol to extract the color correction
> characterictics from a blob, coming from DRM layer
> as property_set_blob() or loading a blob for property_get_blob()
>
> - Userspace will load this blob with corresponding values and use the
> drm_set_blob(color-manager) interface.
> - DRM layer of get/set_color_manager() will validate the entry, extract
> the following information from blob
> - property
> - enable/disable
> - identifier
> - ptr to start of raw data
> - With this information, drm_get/set_color_manager() layer will call
> driver's .get/set_handler()
> which will do the correction using those values.
> - Based on the driver's requirement, user space can send any type of
> values in byte stream, like
> direct reg values, correction coefficients or any other form of data.
>
> Benefits of this common interface:
> ----------------------------------
> - Single interface for all color properties. No need to create multiple
> properties.
> - HW agonist. Its in hands of driver to register the properties.
> - Any no of properties can be registered.
> - Can be clubbed with modeset implementations.
>
>
>
>
> Regards
> Shashank
>
> On 5/7/2014 7:41 PM, Sharma, Shashank wrote:
>>
>> FYI
>>
>>
>> -----Original Message-----
>> From: Sharma, Shashank
>> Sent: Tuesday, April 22, 2014 8:32 PM
>> To: Daniel Vetter
>> Cc: David Herrmann; intel-gfx at lists.freedesktop.org; Cn, Ramakrishnan;
>> Jindal, Sonika; Shankar, Uma
>> Subject: RE: [Intel-gfx] Design review request: DRM color manager
>>
>> David,
>> My apologies for starting a pre-mature design discussion.
>>
>> Daniel,
>> Thanks for pointing out first two things, It was not known to me, I will
>> take care of this in future.
>>
>> First time I presented color-manager design, in internal display design
>> forum, where most of the reviewers were not there.
>> We took the feedback from people who were present, and implemented the
>> design.
>>
>> When we shared color manager implementation, that design was rejected and
>> one of the feedbacks was that it would be better to discuss it on dri-devel
>> where people outside Intel can give their opinion, and that’s the only
>> reason why I added dri-devel for the new design (Please see the attached
>> mail, I replied to all who were in last communication).
>> Please let me know how do we want to proceed now.
>>
>>
>> Regards
>> Shashank
>> -----Original Message-----
>> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
>> Vetter
>> Sent: Tuesday, April 22, 2014 7:18 PM
>> To: Sharma, Shashank
>> Cc: David Herrmann; intel-gfx at lists.freedesktop.org;
>> dri-devel at lists.freedesktop.org; Thierry Reding; Cn, Ramakrishnan; Alex
>> Deucher; Jindal, Sonika; Shankar, Uma
>> Subject: Re: [Intel-gfx] Design review request: DRM color manager
>>
>> On Tue, Apr 22, 2014 at 12:07:41PM +0000, Sharma, Shashank wrote:
>>>
>>> Thanks again David,
>>> Comments inline.
>>
>>
>> Three things:
>> - Please don't send out .pptx files to upstream/public mailing lists,
>> that's just not how the upstream community works.
>>
>> - Please either fix up ms outlook to do proper in-line quoting or switch
>> to a proper mail client for discussions on dri-devel. I'm ok with this
>> on intel-gfx to some extend since that's our own turf, but on dri-devel
>> the usual rules apply.
>>
>> - I think we should discuss this internally first or at least just on
>> intel-gfx.
>>
>> David, thanks for taking a look at this but imo this shouldn't have
>> escaped yet to the public. My apologies for wasting your time trying to
>> review this proposal.
>>
>> Thanks, Daniel
>>
>>>
>>> Regards
>>> Shashank
>>> -----Original Message-----
>>> From: David Herrmann [mailto:dh.herrmann at gmail.com]
>>> Sent: Tuesday, April 22, 2014 5:10 PM
>>> To: Sharma, Shashank
>>> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
>>> Ville Syrjälä; Thierry Reding; Alex Deucher; Sean Paul;
>>> robdclark at gmail.com; Mukherjee, Indranil; Jindal, Sonika; Korjani,
>>> Vikas; Shankar, Uma; Cn, Ramakrishnan
>>> Subject: Re: Design review request: DRM color manager
>>>
>>> Hi
>>>
>>> On Tue, Apr 22, 2014 at 12:21 PM, Sharma, Shashank
>>> <shashank.sharma at intel.com> wrote:
>>>>
>>>> 1) Why do you register only a single property? Why not register a
>>>> separate property for each color-correction that is available? This way you
>>>> can drop the property-id and use the high-level DRM-prop IDs/names.
>>>>>>
>>>>>> That’s the whole idea of color manager. If we keep on creating
>>>>>> properties for each color correction, there would be a big list and a lot of
>>>>>> properties will be exposed. Instead one common blob which can represent all
>>>>>> the properties, correction values and identifiers. It would be easy to club
>>>>>> with atomic modeset kind-of designs also I believe.
>>>
>>>
>>> Where is the difference? With one _well-defined_ property for each type
>>> we simply move the identification one level up. With your approach you just
>>> move the type-id one level down into the blob.
>>>
>>> Or in other words: Where is the difference between calling
>>> SetProperty() n-times, or calling it once but with a parameter describing
>>> n-properties? With atomic-modesetting we can set as many properties as we
>>> want and make the kernel apply them atomically.
>>>
>>>>>> Actually we also do not want to populate the property space also, as
>>>>>> if there are 10 color correction methods possible for a hardware, we might
>>>>>> end up listing 10 properties. And there won't be common properties across
>>>>>> all the hardwares also. For example, Hardware A can have properties X Y Z
>>>>>> but Hardware B can have W X and Z. This will make the property space
>>>>>> inconsistent. But if we provide one common interface which will cover for
>>>>>> all the properties, for all the hardwares in a single blob. The driver will
>>>>>> dynamically register its property, in its own preferred name. A get_prop()
>>>>>> will always list down all the supported color property by this hardware and
>>>>>> driver.
>>>
>>>
>>>> 2) What is the CRTC-ID for? DRM properties can be set on a specific CRTC
>>>> and/or plane. Isn't that enough information for the driver?
>>>>>>
>>>>>> This is to make it HW agonist. Actually that's CRTC ID / Plane ID /
>>>>>> PIPE ID / all together an identifier. For example if I want to set gamma
>>>>>> correction for sprite planes only, not on primary plane or pipe level, on
>>>>>> VLV, its possible. This gives me flexibility to mention fine-tuned
>>>>>> correction even in a CRTC. The driver's .enable method can take decision on
>>>>>> this identifier based on the hardware capabilities.
>>>
>>>
>>> Yeah, but I meant the drmModeObjectSetProperty() ioctl already tales a
>>> CRTC/Plane/Connector ID. So why duplicate that information in the
>>> blob? And more importantly, what happens if you call
>>> drmModeObjectSetProperty() on a plane but specify a CRTC ID in the blob?
>>> Seems weird to me to support such setups.
>>>
>>>>>> The design is to register color-manager as a CRTC property, to make it
>>>>>> consistent, and then give the fine tuning via this identifier byte.
>>>
>>> Else we have to keep track of this in userspace, that which property is
>>> valid for which extent. For example, Hue and saturation correction, on VLV,
>>> can be applied on Sprite planes only(not on primary plane). So we have to
>>> send a plane as an object here.
>>> Rather in color manager case, we will always send the CRTC as an object
>>> to IOCTL, but will specify SPRITE_PLANE as identifier. Does this sound less
>>> weird now :) ?
>>>
>>>> 3) Please document the payload for each of the properties you define.
>>>> If the property is a blob, there is no reason to make the properties
>>>> generic. User-space requires a common syntax across all drivers, otherwise,
>>>> it cannot make use of generic properties and you should use driver-dependent
>>>> properties instead.
>>>>>>
>>>>>> Can you please elaborate a bit more ? I believe that a blob is a
>>>>>> superset of single and multi-valued properties. So we can use the byte
>>>>>> defined for <no of correction bytes> and specify both single value and multi
>>>>>> value correction using the same interface, >> method and protocol. So any
>>>>>> userspace can just follow this, any can give commands to any driver.
>>>
>>>
>>> Well, your document doesn't describe the payload at all. I just wanted a
>>> description of what kind of information is expected. Number of arguments,
>>> argument size, argument types, argument description.. and so on.
>>>>>>>
>>>>>>> Sure, I will further document it very clearly about arguments and
>>>>>>> descriptions. Actually we have discussed the protocol in the color EDID
>>>>>>> section, which tells us about the 4 byte protocol and expectation, but
>>>>>>> that’s elementary.
>>>
>>>
>>>> 4) We have a tuple-type for properties. So in case you only need 32bit
>>>> payloads for a given property, you can combine enable/disable and value in a
>>>> single 64bit property.
>>>>>>
>>>>>> But properties like CSC and Gamma correction need multiple correction
>>>>>> values, up to 256 32-bit values. For this we need more no of values. AM I
>>>>>> getting it right ?
>>>
>>> Sure.
>>>
>>> Thanks
>>> David
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list