On Wed, Sep 23, 2015 at 11:57:58AM +0000, Sharma, Shashank wrote:
> This would be an interface/design change, to change from one blob of
> correction property, to split into multiple query properties for
> palette_before_blob and palette_after_blob. Please let me know if this
> is really required ?
Yes I think splitting this up into one property per value is the right
approach. That's also why we split the before/after gamma tables and the
ctm each into it's own property.
And I don't think this is a design change of the interface - we still
exchange the exact same values with the exact semantics between kernel and
userspace, there's just a small difference in the actual transport
protocol. Those kinds of minor changes happen all the time when
upstreaming features. And that's the reason why we absolutely _must_ have
a close collaboration between the kernel and userspace side. Which
unfortunately on this feature here took a few months to get going, but
hopefully with shared git trees and talking on irc now that should be
easier.
-Daniel
>
> Regards
> Shashank
> -----Original Message-----
> From: Smith, Gary K
> Sent: Wednesday, September 23, 2015 3:17 PM
> To: Sharma, Shashank; Daniel Vetter; Roper, Matthew D
> Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx at
> lists.freedesktop.org; dri-devel at lists.freedesktop.org; Mukherjee,
> Indranil; Bish, Jim; kausalmalladi at gmail.com; Vetter, Daniel
> Subject: RE: [PATCH 02/23] drm: Add structure for querying palette color
> capabilities
>
> Given that its only one word of info per LUT, I'm OK with it being two
> separate properties.
> I believe it was much more complex previously with a lot more info per LUT,
> which is probably why I preferred a blob.
>
> Thanks
> Gary
>
> -----Original Message-----
> From: Sharma, Shashank
> Sent: Wednesday, September 23, 2015 9:10 AM
> To: Daniel Vetter; Roper, Matthew D
> Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx at
> lists.freedesktop.org; dri-devel at lists.freedesktop.org; Mukherjee,
> Indranil; Bish, Jim; Smith, Gary K; kausalmalladi at gmail.com; Vetter, Daniel
> Subject: Re: [PATCH 02/23] drm: Add structure for querying palette color
> capabilities
>
> Hi Matt, Daniel
> Addressing the review comments from both of you here.
>
> Regards
> Shashank
>
> On 9/22/2015 6:32 PM, Daniel Vetter wrote:
> > On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
> >> On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
> >>> From: Kausal Malladi <kausalmalladi at gmail.com>
> >>>
> >>> The DRM color management framework is targeting various hardware
> >>> platforms and drivers. Different platforms can have different color
> >>> correction and enhancement capabilities.
> >>>
> >>> A commom user space application can query these capabilities using
> >>> the DRM property interface. Each driver can fill this property with
> >>> its platform's palette color capabilities.
> >>>
> >>> This patch adds new structure in DRM layer for querying palette
> >>> color capabilities. This structure will be used by all user space
> >>> agents to configure appropriate color configurations.
> >>>
> >>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> >>> Signed-off-by: Kausal Malladi <kausalmalladi at gmail.com>
> >>
> >> I think you provided an explanation on a previous code review cycle,
> >> but I forget the details now...what's the benefit to using a blob for
> >> caps rather than having these be individual properties? Individual
> >> properties seems more natural to me, but I think you had a
> >> justification for blobbing them together; that reasoning would be
> >> good to include in the commit message.
> >
> > Yeah I'm leaning slightly towards individual props too, that would
> > give us a bit more freedom with placing them (e.g. if someone comes up
> > with funky hw where before_ctm and ctm are per-plane and after_ctm is
> > on the crtc, with only some planes support the before_ctm gamma table).
> This was the part where we spent most of the time during the design review,
> and the reason we came up for this was:
> - This is a read only property, which userspace would like to read only once,
> and cache the information. It was also Gary's opinion to keep this as single
> blob for all.
> - Making individual property needs more information to be provided to user
> space.
> - This is a blob only for pipe level capabilities, the plane level blob will
> be separate from this.
> - We can handle this HW also, by loading proper plane and pipe level
> capability blob. This is more convenient to have all the capabilities
> together at the same place, than keep on querying the same.
> >
> > Also if you do per-prop properties instead of the blob you can drop
> > the version/reserved fields, since properties are inheritedly designed
> > to be extendible. So no need to revision them again (it only leads to
> > more code that might break).
> > -Daniel
> >
> We are anyways planning to drop the version, as per Ville's comment.
> - Shashank
> >>
> >>
> >> Matt
> >>
> >>> ---
> >>> include/uapi/drm/drm.h | 11 +++++++++++
> >>> 1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index
> >>> 3801584..e3c642f 100644
> >>> --- a/include/uapi/drm/drm.h
> >>> +++ b/include/uapi/drm/drm.h
> >>> @@ -829,6 +829,17 @@ struct drm_event_vblank {
> >>> __u32 reserved;
> >>> };
> >>>
> >>> +struct drm_palette_caps {
> >>> + /* Structure version. Should be 1 currently */
> >>> + __u32 version;
> >>> + /* For padding and future use */
> >>> + __u32 reserved;
> >>> + /* This may be 0 if not supported. e.g. plane palette or VLV pipe */
> >>> + __u32 num_samples_before_ctm;
> >>> + /* This will be non-zero for pipe. May be zero for planes on some HW */
> >>> + __u32 num_samples_after_ctm;
> >>> +};
> >>> +
> >>> /* typedef area */
> >>> #ifndef __KERNEL__
> >>> typedef struct drm_clip_rect drm_clip_rect_t;
> >>> --
> >>> 1.9.1
> >>>
> >>
> >> --
> >> Matt Roper
> >> Graphics Software Engineer
> >> IoTG Platform Enabling & Development
> >> Intel Corporation
> >> (916) 356-2795
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
