Re: [PATCH 03/12] drm/i915: Attach color properties to CRTC

["Malladi, Kausal" <Kausal.Malladi@intel.com>]

On Wednesday 08 July 2015 04:53 AM, Matt Roper wrote:
> On Fri, Jul 03, 2015 at 09:01:38AM +0530, Kausal Malladi wrote:
>> This patch does the following:
>> 1. Adds new files intel_color_manager(.c/.h)
>> 2. Attaches color properties to CRTC while initialization
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile              |  3 +-
>>   drivers/gpu/drm/i915/intel_color_manager.c | 61 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h | 28 ++++++++++++++
>>   drivers/gpu/drm/i915/intel_display.c       |  3 ++
>>   drivers/gpu/drm/i915/intel_drv.h           |  4 ++
>>   5 files changed, 98 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>>   create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index de21965..ad928d8 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -56,7 +56,8 @@ i915-y += intel_audio.o \
>>   	  intel_overlay.o \
>>   	  intel_psr.o \
>>   	  intel_sideband.o \
>> -	  intel_sprite.o
>> +	  intel_sprite.o \
>> +	  intel_color_manager.o
>>   i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
>>   i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> new file mode 100644
>> index 0000000..9280ea6
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -0,0 +1,61 @@
>> +/*
>> + * Copyright © 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Shashank Sharma <shashank.sharma@intel.com>
>> + * Kausal Malladi <Kausal.Malladi@intel.com>
>> + */
>> +
>> +#include "intel_color_manager.h"
>> +
>> +void intel_color_manager_attach(struct drm_device *dev,
>> +		struct drm_mode_object *mode_obj)
>> +{
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (mode_obj->type == DRM_MODE_OBJECT_CRTC) {
> Is the expectation that this function will grow to include plane
> properties as well?  Personally I think it would be a little easier to
> read/follow if we had separate functions for attaching the crtc
> properties and the (eventual) plane properties, but it's not a huge
> deal.
Yes, it is expected to grow to include plane properties as well.
>
>> +		if (config->prop_color_capabilities) {
>> +			drm_object_attach_property(mode_obj,
>> +					config->prop_color_capabilities, 0);
>> +			DRM_DEBUG_DRIVER("Attached Color Caps property to CRTC\n");
>> +		} else
>> +			DRM_ERROR("Error attaching Color Capabilities property to CRTC\n");
>> +		if (config->prop_palette_before_ctm) {
>> +			drm_object_attach_property(mode_obj,
>> +					config->prop_palette_before_ctm, 0);
>> +			DRM_DEBUG_DRIVER("Attached Palette (before CTM) property to CRTC\n");
>> +		} else
>> +			DRM_ERROR("Error attaching Palette (before CTM) property to CRTC\n");
>> +		if (config->prop_palette_after_ctm) {
>> +			drm_object_attach_property(mode_obj,
>> +					config->prop_palette_after_ctm, 0);
>> +			DRM_DEBUG_DRIVER("Attached Palette (after CTM) property to CRTC\n");
>> +		} else
>> +			DRM_ERROR("Error attaching Palette (after CTM) property to CRTC\n");
>> +		if (config->prop_ctm) {
>> +			drm_object_attach_property(mode_obj,
>> +					config->prop_ctm, 0);
>> +			DRM_DEBUG_DRIVER("Attached CTM property to CRTC\n");
>> +		} else
>> +			DRM_ERROR("Error attaching CTM property to CRTC\n");
>> +	}
> I agree with Damien's note that we can probably drop the messages here.
> Also note that in cases like this the kernel coding style standards
> indicate that even though your 'else' branches are only a single
> statement, we should still use braces because braces were used on the
> 'if' branch.
Sure, got it.
>
> Do we intend to expose these properties to userspace on all hardware
> generations?  I'd expect us to only add the properties to the crtc's
> (and planes) if the hardware can actually support it.
Yes, sure.
>
>
> Matt
>
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> new file mode 100644
>> index 0000000..c564761
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * Copyright © 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Shashank Sharma <shashank.sharma@intel.com>
>> + * Kausal Malladi <Kausal.Malladi@intel.com>
>> + */
>> +#include <drm/drmP.h>
>> +#include <drm/drm_crtc_helper.h>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 724b0e3..e175df2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14105,6 +14105,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>>   
>>   	intel_crtc->wm.cxsr_allowed = true;
>>   
>> +	/* Attaching color properties to the CRTC */
>> +	intel_color_manager_attach(dev, &intel_crtc->base.base);
>> +
>>   	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>>   	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>>   	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 3f0a890..1e18a7e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1446,4 +1446,8 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>>   			       struct drm_plane_state *state);
>>   extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>>   
>> +/* intel_color_manager.c */
>> +void intel_color_manager_attach(struct drm_device *dev,
>> +		struct drm_mode_object *mode_obj);
>> +
>>   #endif /* __INTEL_DRV_H__ */
>> -- 
>> 2.4.5
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel