linux-kernel - Re: [PATCH v4 01/11] drm/bridge: dw-hdmi: set mtmdsclock for deep color

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]

Date:   Mon, 2 Mar 2020 10:21:22 +0100
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     a.hajda@...sung.com, jonas@...boo.se, jernej.skrabec@...l.net,
        boris.brezillon@...labora.com, linux-amlogic@...ts.infradead.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 01/11] drm/bridge: dw-hdmi: set mtmdsclock for deep
 color

On 02/03/2020 10:05, Laurent Pinchart wrote:
> Hi Neil and Jonas,
> 
> Thank you for the patch.
> 
> On Thu, Feb 06, 2020 at 08:18:24PM +0100, Neil Armstrong wrote:
>> From: Jonas Karlman <jonas@...boo.se>
>>
>> Configure the correct mtmdsclock for deep colors to prepare support
>> for 10, 12 & 16bit output.
>>
>> Signed-off-by: Jonas Karlman <jonas@...boo.se>
>> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 67fca439bbfb..9e0927d22db6 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -1818,9 +1818,26 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>>  
>>  	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
> 
> Nitpicking a bit, I would change
> 
> -	vmode->mtmdsclock = vmode->mpixelclock = mode->clock * 1000;
> +	vmode->mpixelclock = mode->clock * 1000;
> 
> above, and here add
> 
> 	vmode->mtmdsclock = vmode->mpixelclock;
> 
> to keep all mtmdsclock calculation in a single place.
> 
>> +	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) {
>> +		switch (hdmi_bus_fmt_color_depth(
>> +				hdmi->hdmi_data.enc_out_bus_format)) {
>> +		case 16:
>> +			vmode->mtmdsclock = (u64)vmode->mpixelclock * 2;
> 
> Both mpixelclock and mtmdsclock are unsigned int. Is the cast to u64
> needed ?

Good question, should probably be removed.

> 
> On a separate but related note, what does the 'm' in tmdsclock stand for
> ? It seems to originate from the 'm' prefix for mpixelclock, which has
> been there from the start. Unless there's a good reason for the prefix,
> renaming mtmdsclock to tmds_clock (and handling the other fields in the
> hdmi_vmode structure similarly) would increase clarity I think.

The mode->clock is is KHz, thus it seems the m was added to express it's in Hz,
so yeah we could drop the m in tmdsclock.

> 
>> +			break;
>> +		case 12:
>> +			vmode->mtmdsclock = (u64)vmode->mpixelclock * 3 / 2;
>> +			break;
>> +		case 10:
>> +			vmode->mtmdsclock = (u64)vmode->mpixelclock * 5 / 4;
>> +			break;
>> +		}
>> +	}
>> +
>>  	if (hdmi_bus_fmt_is_yuv420(hdmi->hdmi_data.enc_out_bus_format))
>>  		vmode->mtmdsclock /= 2;
>>  
>> +	dev_dbg(hdmi->dev, "final tmdsclk = %d\n", vmode->mtmdsclock);
> 
> s/tmdsclk/tmdsclock/ to match the field name ?

yep

> 
>> +
>>  	/* Set up HDMI_FC_INVIDCONF */
>>  	inv_val = (hdmi->hdmi_data.hdcp_enable ||
>>  		   (dw_hdmi_support_scdc(hdmi) &&
> 

Neil