Re: [RFC 3/4] drm: add generic blob properties for image enhancement

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 10, 2014 at 09:50:14AM +0530, Rahul Sharma wrote:
> On 8 March 2014 06:16, Matt Roper <matthew.d.roper@intel.com> wrote:
> > On Fri, Mar 07, 2014 at 03:50:54PM +0530, Rahul Sharma wrote:
> >> Hi Daniel,
> >>
> >> On 7 March 2014 14:06, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> > On Thu, Mar 06, 2014 at 11:42:13AM +0530, Rahul Sharma wrote:
> >> >> Add generic KMS blob properties to core drm framework. These
> >> >> are writable blob properties which can be used to set Image
> >> >> Enhancement parameters. The properties which are added here
> >> >> are meant for color reproduction, color saturation and edge
> >> >> enhancement.
> >> >>
> >> >> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> > ...
> >> > Tbh I don't really understand why we can't use normal properties for this.
> >> > We might want to add a new RBG type of property (or YUV fwiw, both with
> >> > and without alpha) to make this standardized (maybe with some fixed point
> >> > value for non-normalizes).
> >>
> >> I dropped Normal properties (the ones which accepts 64 bit data)
> >> because there will be too many of them. As you can see the
> >> count is going upto 24 parameters, means 24 such properties, as
> >> each can carry one parameter. This is too much of overhead.
> >> Please correct me if you mean something different.
> >>
> >> I am not sure what are RGB type properties? I know only about 64-bit
> >> normal properties which are too compact to carry above structures. Are
> >> you talking about set_gamma type of ioctls. Each "set_gamma" type
> >> ioctl needs a addition in callbacks till down the layer which looks bit
> >> unnecessary, while writable blob properties are using same set_property
> >> and get_property infrastructure.
> >>
> >> >
> >> > If the only reason you group them is to atomically commit them, then the
> >> > only thing we need is the atomic ioctl, which can shovel entire groups of
> >> > properties over the wire at once.
> >>
> >> Atomic commit is not an explicit requirement. But splitting them to
> >> many small pieces (in case of normal properties) are resulting into
> >> many user-kernel switching overhead, which seems avoidable.
> >
> > The idea with atomic modeset / nuclear pageflip is that you collect a
> > whole bunch of individual property updates into a "property set"
> > container in userspace (libdrm).  When you're done setting all the
> > values you want and "commit" the property set, all of the values that
> > you collected get passed to the kernel in one go via a new ioctl (and
> > are then updated in an atomic manner by the DRM driver).  So performing
> > your 24 different property updates via the upcoming atomic API's
> > shouldn't be a problem and shouldn't add any unnecessary overhead.
> >
> > The kernel-side of atomic modeset / nuclear pageflip is still a WIP, so
> > it isn't upstream yet, but you can see the userspace-facing API in Rob
> > Clark's repo here:
> >
> >         https://github.com/robclark/libdrm/blob/atomic/xf86drmMode.h
> >
> > Note the drmModePropertySet{Alloc,Add,Commit,Free}() functions near the
> > bottom.
> >
> 
> Thanks Matt,
> 
> I went through the share link. Got the idea of atomic
> modeset. It is a good solution for setting 24 properties
> in one go. But another issue is still unaddressed. I
> mean still need to declare 24 properties which are not
> "Really 24 different properties".
> 
> These are sub-parameters to 3 properties (color
> reproduction, color saturation and edge enhancement).
> Splitting them to 24 pieces, just because we don't have a
> a provision in KMS, is a work around to get things
> working. And, properties like "saturation_hue_gain_red",
> "saturation_hue_gain_green" ... will be undoubtedly ugly.
> 
> In Rob Clark's tree, I noticed
> 
> extern int drmModePropertySetAddBlob(drmModePropertySetPtr set,
>     uint32_t object_id,
>     uint32_t property_id,
>     uint64_t length,
>     void *blob);
> 
> Seems, already some implementation of writable blobs is
> there. I am not able to see the kernel though.
> 
> I request experts, to review this solution again. I found it
> quite useful and hold good with the idea of Atomic modeset.

Like I've said I think creating a standard property for RBG and RBGA
values makes sense, to group this stuff a bit. But I don't think we need
to group things further.

Some clear definitions of these properties is needed though - Sagar from
our display group is working on that a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch