Re: [v5 08/24] drm/i915: Add intel_color_op

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

Hi Jani,

Thank you for the reviews.
Apologies on late replies, took some time to get back to it.

On 7/4/2025 6:06 PM, Jani Nikula wrote:
> On Wed, 02 Jul 2025, Uma Shankar <uma.shankar@intel.com> wrote:
>> From: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>>
>> Add data structure to store intel specific details of colorop
>>
>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>   .../drm/i915/display/intel_display_types.h    | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index ce45261c4a8f..4b5124a08cc9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1560,6 +1560,10 @@ struct intel_plane {
>>   #define to_intel_framebuffer(fb) \
>>   	container_of_const((fb), struct intel_framebuffer, base)
>>   
>> +#define intel_fb_obj(x) ((x) ? to_intel_bo((x)->obj[0]) : NULL)
>> +#define to_intel_plane_colorop(x) container_of(x, struct intel_plane_colorop, base)
>> +#define to_intel_colorop_state(x) container_of(x, struct intel_plane_colorop_state, uapi)
> 
> I think this makes further refactoring harder. Please make them proper
> functions instead, not even static inlines.
> 
> See what I've done for intel_global_state stuff.

I had to create new files to achieve this. Let me know if it is the 
right way to go about it.

==
Chaitanya

> 
>> +
>>   struct intel_hdmi {
>>   	i915_reg_t hdmi_reg;
>>   	struct {
>> @@ -1932,6 +1936,21 @@ struct intel_dp_mst_encoder {
>>   	struct intel_connector *connector;
>>   };
>>   
>> +struct intel_plane_colorop {
>> +	struct drm_colorop base;
>> +	enum intel_color_block id;
>> +};
>> +
>> +struct intel_plane_colorop_state {
>> +	struct drm_colorop_state uapi;
>> +
>> +	/* TODO: Add hw implementation */
>> +	struct {
>> +		bool active, enable;
>> +		struct drm_property_blob *data;
>> +	} hw;
>> +};
>> +
>>   static inline struct intel_encoder *
>>   intel_attached_encoder(struct intel_connector *connector)
>>   {
>