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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2e6e5e8d-e502-5957-5708-4e4d7ef84d8e@ideasonboard.com>
Date:   Tue, 15 Oct 2019 15:26:35 +0100
From:   Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
To:     Jacopo Mondi <jacopo@...ndi.org>
Cc:     Jacopo Mondi <jacopo+renesas@...ndi.org>,
        laurent.pinchart@...asonboard.com, geert@...ux-m68k.org,
        horms@...ge.net.au, uli+renesas@...nd.eu,
        VenkataRajesh.Kalakodima@...bosch.com, airlied@...ux.ie,
        daniel@...ll.ch, koji.matsuoka.xm@...esas.com, muroya@....co.jp,
        Harsha.ManjulaMallikarjun@...bosch.com, ezequiel@...labora.com,
        seanpaul@...omium.org, linux-renesas-soc@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/8] drm: rcar-du: Add support for CMM

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