[Intel-gfx] [PATCH 3/4] drm/i915: CSC color correction
Sharma, Shashank
shashank.sharma at intel.com
Wed Sep 10 14:05:34 CEST 2014
Hi Daniel,
> Overall I still think there's a bit too much indirection - I don't
see why
> the color manager needs to sit in a separate file with separate
> registration functions. Doing it like that rips apart the per-crtc
> setup/teardown quite a lot and isn't how properties are handled anywhere
> else.
I dont see any issue creating a new file, for 2 reasons:
- I dont see any value in overpopulating the 11K lines file intel-
display.c any further.
- All these properties are doing color correction, and hence there is
no harm in keeping them in a functionally separate file.
- How does it tear down per CRTC setup creating a separate file :) ?
I can see all standard DRM connector property getting created in
drm_crtc.c, other properties like pfit, force-audio etc are getting
created in intel_modes.c. They are already spread across functional
files, so I find it best to create in color correction file.
Regards
Shashank
On 9/10/2014 12:10 PM, Daniel Vetter wrote:
> On Tue, Sep 09, 2014 at 06:30:09PM -0700, Matt Roper wrote:
>> On Tue, Sep 09, 2014 at 11:53:15AM +0530, shashank.sharma at intel.com wrote:
>>> From: Shashank Sharma <shashank.sharma at intel.com>
>>>
>>> This patch adds support for CSC correction color property.
>>> It does the following:
>>> 1. Creates a new DRM property for CSC correction. Adds this into
>>> mode_config.
>>> 2. Attaches this CSC property to calling CRTC. Creates a blob
>>> to store the correction values, and attaches the blob to CRTC.
>>> 3. Adds function intel_clrmgr_set_csc: This is a wrapper function
>>> which checks the platform type, and calls the valleyview
>>> specific set_csc function. As different platforms may have different
>>> methods of setting CSC, wrapper function is required to route to proper
>>> core CSC set function. In future, the support for other platfroms can be
>>> plugged-in here. Adding this function as .set_property CSC color property.
>>> 4. Adds function vlv_set_csc: core function to program CSC coefficients as per
>>> vlv specs, and then enable CSC.
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>
>> I haven't read up on the hardware programming side of this code to give
>> any comments there, but I got a bit lost trying to follow your blob
>> upload handling here. Like Bob noted, it kind of looks like you're
>> trying to add userspace blob property upload functionality that would
>> really belong in the DRM core. However, in the intermediate/long term
>> there probably isn't really a need for this kind of blob upload support
>> because the atomic propertysets will provide the functionality you need;
>> once we have atomic support, I think it would be better to just make
>> each of these values an independent property and upload all of the
>> values together as part of a single property set. But I realize you're
>> specifically trying to add add this support in a pre-atomic world which
>> makes things more challenging. Atomic is definitely coming, but I think
>> the timeframe is kind of uncertain still, so it's really going to be up
>> to the upstream maintainers on how to proceed. Maybe Daniel can give
>> you some direction?
>
> I've thought the csc stuff here is just a matrix of register values, and
> highly intel specific at that. So might as well keep it as a blob property
> for now until either the specific layout changes or some standard for
> generic csc emerges.
>
> Overall I still think there's a bit too much indirection - I don't see why
> the color manager needs to sit in a separate file with separate
> registration functions. Doing it like that rips apart the per-crtc
> setup/teardown quite a lot and isn't how properties are handled anywhere
> else.
> -Daniel
>
>>
>>
>> Matt
>>
>>> ---
>>> drivers/gpu/drm/drm_crtc.c | 4 +-
>>> drivers/gpu/drm/i915/i915_reg.h | 11 ++
>>> drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++++++++++++++++++++++++++++++++++-
>>> drivers/gpu/drm/i915/intel_clrmgr.h | 16 +++
>>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>>> include/drm/drm_crtc.h | 7 ++
>>> 6 files changed, 244 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>> index 272b66f..be9d110 100644
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -3917,7 +3917,7 @@ done:
>>> return ret;
>>> }
>>>
>>> -static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>>> +struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
>>> void *data)
>>> {
>>> struct drm_property_blob *blob;
>>> @@ -3944,7 +3944,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
>>> return blob;
>>> }
>>>
>>> -static void drm_property_destroy_blob(struct drm_device *dev,
>>> +void drm_property_destroy_blob(struct drm_device *dev,
>>> struct drm_property_blob *blob)
>>> {
>>> drm_mode_object_put(dev, &blob->base);
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 20673cc..e3010b3 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -6183,6 +6183,17 @@ enum punit_power_well {
>>> #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>>> #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>>>
>>> +/* VLV color correction registers */
>>> +/* CSC */
>>> +#define PIPECONF_CSC_ENABLE (1 << 15)
>>> +#define _PIPEACSC (dev_priv->info.display_mmio_offset + \
>>> + 0x600b0)
>>> +#define _PIPEBCSC (dev_priv->info.display_mmio_offset + \
>>> + 0x610b0)
>>> +#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
>>> +#define CSC_OFFSET (_PIPEBCSC - _PIPEACSC)
>>> +#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
>>> +
>>> /* VLV MIPI registers */
>>>
>>> #define _MIPIA_PORT_CTRL (VLV_DISPLAY_BASE + 0x61190)
>>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c b/drivers/gpu/drm/i915/intel_clrmgr.c
>>> index ac0a890..36c64c1 100644
>>> --- a/drivers/gpu/drm/i915/intel_clrmgr.c
>>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.c
>>> @@ -32,6 +32,138 @@
>>> #include "i915_reg.h"
>>> #include "intel_clrmgr.h"
>>>
>>> +/*
>>> +* Names to register with color properties.
>>> +* Sequence must be the same as the order
>>> +* of enum clrmgr_tweaks
>>> +*/
>>> +const char *clrmgr_property_names[] = {
>>> + /* csc */
>>> + "csc-correction",
>>> + /* add a new prop name here */
>>> +};
>>> +
>>> +
>>> +/*
>>> +* Sizes for color properties. This can differ
>>> +* platform by platform, hence 'vlv' prefix
>>> +* The sequence must be same as the order of
>>> +* enum clrmgr_tweaks
>>> +*/
>>> +u32 vlv_color_property_sizes[] = {
>>> + VLV_CSC_MATRIX_MAX_VALS,
>>> + /* Add new property size here */
>>> +};
>>
>> As with the property names, I'm not sure whether having an array here
>> gives us much clarity. I think it's fine to just pass
>> VLV_CSC_MATRIX_MAX_VALS directly to drm_property_create_blob() in the
>> code below.
>>
>>
>>> +
>>> +/* Default CSC values to create property with */
>>> +uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
>>> + 0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
>>> +};
>>> +
>>> +/*
>>> +* vlv_set_csc
>>> +* Valleyview specific csc correction method.
>>> +* Programs the 6 csc registers with 3x3 correction matrix
>>> +* values.
>>> +* inputs:
>>> +* - intel_crtc*
>>> +* - color manager registered property for csc correction
>>> +* - data: pointer to correction values to be applied
>>> +*/
>>> +/* Enable color space conversion on PIPE */
>>> +bool vlv_set_csc(struct intel_crtc *intel_crtc,
>>> + struct drm_property *prop, uint64_t values, bool enable)
>>> +{
>>> + u32 count = 0;
>>> + u32 c0, c1, c2;
>>> + u32 pipeconf, csc_reg, data_size;
>>> + uint64_t *blob_data;
>>> + uint64_t *data = NULL;
>>> + struct drm_device *dev = intel_crtc->base.dev;
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> + struct drm_property_blob *blob = intel_crtc->csc_blob;
>>> +
>>> + /* Sanity */
>>> + if (!blob || (blob->length != VLV_CSC_MATRIX_MAX_VALS)) {
>>> + DRM_ERROR("Invalid/NULL CSC blob\n");
>>> + return false;
>>> + }
>>> + blob_data = (uint64_t *)blob->data;
>>> +
>>> + /* No need of values to disable property */
>>> + if (!enable)
>>> + goto configure;
>>> +
>>> + /* Enabling property needs correction values */
>>> + data_size = VLV_CSC_MATRIX_MAX_VALS;
>>> + data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
>>> + if (!data) {
>>> + DRM_ERROR("Out of memory\n");
>>> + return false;
>>> + }
>>> +
>>> + if (copy_from_user((void *)data, (const void __user *)values,
>>> + data_size * sizeof(uint64_t))) {
>>> + DRM_ERROR("Failed to copy all data\n");
>>> + kfree(data);
>>> + return false;
>>> + }
>>> +
>>> + DRM_DEBUG_DRIVER("Setting CSC on pipe = %d\n", intel_crtc->pipe);
>>> + csc_reg = PIPECSC(intel_crtc->pipe);
>>> +
>>> + /* Read CSC matrix, one row at a time */
>>> + while (count < VLV_CSC_MATRIX_MAX_VALS) {
>>> + c0 = data[count] & VLV_CSC_VALUE_MASK;
>>> + *blob_data++ = c0;
>>> + c1 = data[count] & VLV_CSC_VALUE_MASK;
>>> + *blob_data++ = c1;
>>> + c2 = data[count] & VLV_CSC_VALUE_MASK;
>>> + *blob_data++ = c2;
>>> +
>>> + /* C0 is LSB 12bits, C1 is MSB 16-27 */
>>> + I915_WRITE(csc_reg, (c1 << VLV_CSC_COEFF_SHIFT) | c0);
>>> + csc_reg += 4;
>>> +
>>> + /* C2 is LSB 12 bits */
>>> + I915_WRITE(csc_reg, c2);
>>> + csc_reg += 4;
>>> + }
>>> +
>>> +configure:
>>> +
>>> + /* Enable / Disable csc correction */
>>> + pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
>>> + enable ? (pipeconf |= PIPECONF_CSC_ENABLE) :
>>> + (pipeconf &= ~PIPECONF_CSC_ENABLE);
>>> + I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
>>> + POSTING_READ(PIPECONF(intel_crtc->pipe));
>>> + DRM_DEBUG_DRIVER("CSC successfully %s pipe %C\n",
>>> + enable ? "enabled" : "disabled", pipe_name(intel_crtc->pipe));
>>> +
>>> + kfree(data);
>>> + return true;
>>> +}
>>> +
>>> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
>>> + struct drm_property *prop, uint64_t values)
>>> +{
>>> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>> + struct drm_device *dev = intel_crtc->base.dev;
>>> + bool enable;
>>> +
>>> + /* First value is enable/disable control, others are data */
>>> + enable = *((uint64_t *)values);
>>> + values += (sizeof(uint64_t));
>>> +
>>> + if (IS_VALLEYVIEW(dev))
>>> + return vlv_set_csc(intel_crtc, prop, values, enable);
>>> +
>>> + /* Todo: Support other gen devices */
>>> + DRM_ERROR("Color correction is supported only on VLV for now\n");
>>> + return false;
>>> +}
>>> +
>>> void
>>> intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>>> {
>>> @@ -41,18 +173,92 @@ intel_attach_plane_color_correction(struct intel_plane *intel_plane)
>>> void
>>> intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
>>> {
>>> + struct drm_device *dev = intel_crtc->base.dev;
>>> + struct drm_mode_object *obj = &intel_crtc->base.base;
>>> + struct drm_property *property = NULL;
>>> +
>>> DRM_DEBUG_DRIVER("\n");
>>> + mutex_lock(&dev->mode_config.mutex);
>>> +
>>> + /* color property = csc */
>>> + property = dev->mode_config.csc_property;
>>> + if (!property) {
>>> + DRM_ERROR("No such property to attach\n");
>>> + goto release_mutex;
>>> + }
>>> +
>>> + /* create blob for correction values */
>>> + intel_crtc->csc_blob = drm_property_create_blob(dev,
>>> + vlv_color_property_sizes[csc], (void *)vlv_csc_default);
>>> + if (!intel_crtc->csc_blob) {
>>> + DRM_ERROR("Failed to create property blob\n");
>>> + goto release_mutex;
>>> + }
>>> +
>>> + /* Attach blob with property */
>>> + if (drm_object_property_set_value(obj, property,
>>> + intel_crtc->csc_blob->base.id)) {
>>> + DRM_ERROR("Failed to attach property blob, destroying\n");
>>> + drm_property_destroy_blob(dev, intel_crtc->csc_blob);
>>> + goto release_mutex;
>>> + }
>>> +
>>> + DRM_DEBUG_DRIVER("Successfully attached CSC property\n");
>>> +
>>> +release_mutex:
>>> + mutex_unlock(&dev->mode_config.mutex);
>>> }
>>>
>>> int intel_clrmgr_create_color_properties(struct drm_device *dev)
>>> {
>>> + int ret = 0;
>>> + struct drm_property *property;
>>> +
>>> DRM_DEBUG_DRIVER("\n");
>>> - return 0;
>>> + mutex_lock(&dev->mode_config.mutex);
>>> +
>>> + /* CSC correction color property, blob type, size 0 */
>>> + property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>>> + clrmgr_property_names[csc], 0);
>>> + if (!property) {
>>> + DRM_ERROR("Failed to create property(CSC)\n");
>>> + ret++;
>>> + } else {
>>> + dev->mode_config.csc_property = property;
>>> + DRM_DEBUG_DRIVER("Created property: CSC\n");
>>> + }
>>> +
>>> + mutex_unlock(&dev->mode_config.mutex);
>>> + return ret;
>>> }
>>>
>>> void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
>>> {
>>> + struct drm_crtc *crtc;
>>> + struct intel_crtc *intel_crtc;
>>> +
>>> DRM_DEBUG_DRIVER("\n");
>>> +
>>> + mutex_lock(&dev->mode_config.mutex);
>>> +
>>> + /* CSC correction color property */
>>> + if (dev->mode_config.csc_property) {
>>> + drm_property_destroy(dev, dev->mode_config.csc_property);
>>> + dev->mode_config.csc_property = NULL;
>>> + DRM_DEBUG_DRIVER("Destroyed property: CSC\n");
>>> + }
>>> +
>>> + /* Destroy property blob from each CRTC */
>>> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>> + intel_crtc = to_intel_crtc(crtc);
>>> + if (intel_crtc->csc_blob) {
>>> + drm_property_destroy_blob(dev, intel_crtc->csc_blob);
>>> + intel_crtc->csc_blob = NULL;
>>> + }
>>> + }
>>> +
>>> + mutex_unlock(&dev->mode_config.mutex);
>>> + DRM_DEBUG_DRIVER("Successfully destroyed all color properties\n");
>>> }
>>>
>>> void intel_clrmgr_init(struct drm_device *dev)
>>> diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h b/drivers/gpu/drm/i915/intel_clrmgr.h
>>> index 8cff487..5725d6b 100644
>>> --- a/drivers/gpu/drm/i915/intel_clrmgr.h
>>> +++ b/drivers/gpu/drm/i915/intel_clrmgr.h
>>> @@ -39,6 +39,13 @@
>>> #define CLRMGR_PROP_NAME_MAX 128
>>> #define CLRMGR_PROP_RANGE_MAX 0xFFFFFFFFFFFFFFFF
>>>
>>> +/* CSC */
>>> + /* CSC / Wide gamut */
>>> +#define VLV_CSC_MATRIX_MAX_VALS 9
>>> +#define VLV_CSC_VALUE_MASK 0xFFF
>>> +#define VLV_CSC_COEFF_SHIFT 16
>>> +
>>> +
>>> /* Properties */
>>> enum clrmgr_tweaks {
>>> csc = 0,
>>> @@ -50,6 +57,15 @@ enum clrmgr_tweaks {
>>> };
>>>
>>> /*
>>> +* intel_clrmgr_set_csc
>>> +* CSC correction method is different across various
>>> +* gen devices. This wrapper function calls the respective
>>> +* platform specific function to set CSC
>>> +*/
>>> +bool intel_clrmgr_set_csc(struct drm_crtc *crtc,
>>> + struct drm_property *prop, uint64_t data);
>>> +
>>> +/*
>>> * intel_attach_plane_color_correction:
>>> * Attach plane level color correction DRM properties to
>>> * corresponding plane objects.
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 7ba5785..a10b9bb 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -437,6 +437,7 @@ struct intel_crtc {
>>>
>>> int scanline_offset;
>>> struct intel_mmio_flip mmio_flip;
>>> + struct drm_property_blob *csc_blob;
>>> };
>>>
>>> struct intel_plane_wm_parameters {
>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index 31344bf..487ce44 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -851,6 +851,9 @@ struct drm_mode_config {
>>> struct drm_property *aspect_ratio_property;
>>> struct drm_property *dirty_info_property;
>>>
>>> + /* Color correction properties */
>>> + struct drm_property *csc_property;
>>> +
>>> /* dumb ioctl parameters */
>>> uint32_t preferred_depth, prefer_shadow;
>>>
>>> @@ -981,6 +984,10 @@ extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
>>> char *path);
>>> extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>>> struct edid *edid);
>>> +extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
>>> + int length, void *data);
>>> +extern void drm_property_destroy_blob(struct drm_device *dev,
>>> + struct drm_property_blob *blob);
>>>
>>> static inline bool drm_property_type_is(struct drm_property *property,
>>> uint32_t type)
>>> --
>>> 1.9.1
>>>
>>
>> --
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list