Hello Matt,
Thanks for your detailed descriptions, reviews comments and time.
Please find my comments inline.
Regards
Shashank
On 9/10/2014 6:58 AM, Matt Roper wrote:
On Tue, Sep 09, 2014 at 11:53:12AM +0530, shashank.sharma@xxxxxxxxx wrote:
From: Shashank Sharma <shashank.sharma@xxxxxxxxx>
Color manager is an extention to i915 driver which provides display
tuning and color-correction properties to user space, via DRM propery
interface.Different Intel platforms support different color tuning capabilities
which can be exploited using this framework.
This patch set adds color correction for VLV, and the code is written
in such a way that new color properties and support for other platforms can
be pluggen in, using the same architecture.
For the design discussion, right now only one color correction property (CSC)
has been added. Once we agree on the design, gamma-correction, contrast, brightness,
and hue/saturation would be followed by the same.
What this patch set does, is:
1. Color manager framework for valleyview:
Add basic functions of framework, to create and destroy DRM properties for
color correction. It also adds header, enums and defines.
2. Plug-in color manager attach
Attach created color properties, with the objects. Basically properties get attached to
two type of objects, crtc (pipe level correction) and plane (plane level correction).
3. CSC color correction
First color correction method.
- Add color property for CSC during init.
- Add blob to keep correction values
- Attach DRM CSC propery with each CRTC object
- Core CSC correction for VLV
4. Add set_property function for intel CRTC.
This function checks the type of the property, and calls the
appropriate high level function (intel_clrmgr_set_csc), which does
platform check and calls the core platform color correction
function (vlv_set_csc)
V2: Re-designed based on review comments and suggestions from Matt Roper and Daniel.
- Creating only one property per color correction, and attaching it to every DRM object.
- No additional data structures on top of DRM property.
- No registeration of color property, just create and attach.
- Added properties in dev->mode_config
Hi Shashank, thanks for incorporating the feedback from the last review
round. This patchset is definitely moving in the right direction,
although I still feel that there's still a little bit of unnecessary
work/complexity in creating a "framework" here where we probably don't
need it.
I'll give some more detailed responses on your individual patches, but
at a high level I don't really see the need to treat color correction
properties (csc, gamma, etc.) in a special way with their own
registration framework. There are really three things to consider here:
* How/where to create the properties
* How/where to attach properties to CRTC's and/or planes
* How to handle property changes
For creating properties, at the moment you're doing that via
intel_modeset_init() -> intel_clrmgr_init() ->
intel_clrmgr_create_color_properties(). Presumably we'll add other
(non-color correction) properties to the driver in the future, so it
seems like it would be simpler if we just renamed your
intel_clrmgr_create_color_properties() to something like
intel_modeset_create_properties() and called it directly from
intel_modeset_init(). I don't see the intermediate intel_clrmgr_init()
adding any value at the moment, so I think it can be removed.
I got this point Matt, and we can do this. The only confusion I have is,
we already have places where we create properties, and if we are
intending to create different type of properties in this function, would
that align with the previous property creations places ?
At least we have a common ground among the color properties, that's why
I am sticking them together.
If you dont like this, the other common ground is CRTC/plane properties.
These are the first of CRTC or plane properties.
So I can create a intel_create_crtc_properties()
For attaching properties, I'm not sure I see where that is happening in
your current patchset. You have an intel_attach_pipe_color_correction()
function that sounds promising, but the implementation doesn't seem to
actually be calling drm_object_attach_property() that I can see; instead
it seems to be creating a blob value and trying to set it on the object.
Honestly I think all you really need is a single call to:
drm_object_attach_property(intel_crtc->base.base,
dev->mode_config.csc_property, 0);
Yes, I was suppose to do this, and then create a blob. I will change this.
at the bottom of intel_crtc_init() where you have have your call to
intel_attach_pipe_color_correction() right now. I'm not sure if this
code is expected to stay VLV-specific or whether you've only provided a
single platform for RFC/POC purposes, but if it is expected to stay
limited to VLV you'll probably also want to do an
'if (IS_VALLEYVIEW(dev_priv))' before doing the attach so that the property
won't even show up on platforms where you haven't implemented support
yet.
This will be platform independent code. For any platform, we will attach
all the CRTC properties here. But as you rightly mentioned, I will add
the check once we are ok with this architecture.
Also note that aside from the design/coding stuff there are a couple
more bookkeeping things that will need to be done before this patch set
gets accepted upstream. I think you'll need to update the DocBook
documentation in Documentation/DocBook/drm.tmpl with a description of
your new properties (that compiles into documentation like you see at
https://www.kernel.org/doc/htmldocs/drm/drm-kms-properties.html) and
you'll need to add some tests to intel-gpu-tools to exercise this new
functionality.
Yes, Once we are OK with the architecture, I will add documentation for
all the color properties together. Right now we have one command line
test app to test this, which I will convert into an IGT.
I'll provide some more specific feedback on your individual patches.
Matt
Shashank Sharma (4):
drm/i915: Color manager framework for valleyview
drm/i915: Plug-in color manager attach
drm/i915: CSC color correction
drm/i915: Add set_protpery function
drivers/gpu/drm/drm_crtc.c | 4 +-
drivers/gpu/drm/i915/Makefile | 3 +-
drivers/gpu/drm/i915/i915_reg.h | 11 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 283 +++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_clrmgr.h | 108 +++++++++++++
drivers/gpu/drm/i915/intel_display.c | 25 ++++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_sprite.c | 3 +
include/drm/drm_crtc.h | 7 +
9 files changed, 442 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
