Re: [PATCH v5 3/8] drm: rcar-du: Add support for CMM

[Kieran Bingham]

On 15/10/2019 14:33, Jacopo Mondi wrote:
> Hi Kieran, thanks for review

<snip>

>>> +config DRM_RCAR_CMM
>>> +	bool "R-Car DU Color Management Module (CMM) Support"
>>> +	depends on DRM && OF
>>> +	depends on DRM_RCAR_DU
>>
>>
>> DRM_RCAR_DU already depends on both DRM && OF, so I wonder if those are
>> needed to be specified explicitly.
>>
>> Doesn't hurt of course, but I see DRM_RCAR_DW_HDMI does the same, and so
>> does DRM_RCAR_LVDS, so I don't think you need to remove it.
>>
> 
> I did the same as it is done for HDMI and LVDS here. The extra
> dependencies could be dropped yes, I chose to be consistent.

Consistent is fine with me.


<snip>

>>> +struct rcar_cmm {
>>> +	void __iomem *base;
>>> +
>>> +	/*
>>> +	 * @lut:		1D-LUT status
>>> +	 * @lut.enabled:	1D-LUT enabled flag
>>> +	 */
>>> +	struct {
>>> +		bool enabled;
>>> +	} lut;
>>
>> This used to be a more complex structure in an earlier version storing a
>> cached version of the table. Now that the cached entry is removed, does
>> this need to be such a complex structure rather than just say, a bool
>> lut_enabled?
>>
>> (We will soon add an equivalent clu_enabled too, but I don't know what
>> other per-table options we'll need.)
>>
>> In fact, we'll potentially have other options specific to the HGO, and
>> CSC at some point in the future - so grouping by entity is indeed a good
>> thing IMO.
> 
> You are right, I pondered a bit it this was worth it, but I assume the
> other CMM functions would have required some more complex fields so I
> chose to keep it separate. I have no problem to make this a
> lut_enabled, but I fear as soon as we support say, double buffering
> for the lut, having a dedicated struct would be nice.
> 
> Is it ok if I keep this the way it is?

Certainly fine for me. (That's what I tried to imply with "so grouping
by entity is indeed a good thing IMO.")

<snip>
--
Kieran