Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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@xxxxxxxxx>
Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux