Hi,
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 <jo...@kwiboo.se>
>>
>> Configure the correct mtmdsclock for deep colors to prepare support
>> for 10, 12 & 16bit output.
>>
>> Signed-off-by: Jonas Karlman <jo...@kwiboo.se>
>> Signed-off-by: Neil Armstrong <narmstr...@baylibre.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 ?
>
> 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.
>
>> + 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 ?
>
>> +
>> /* Set up HDMI_FC_INVIDCONF */
>> inv_val = (hdmi->hdmi_data.hdcp_enable ||
>> (dw_hdmi_support_scdc(hdmi) &&
>
I fixed the calculus and the cast, but I'll rename the mtmdsclock in a
following patch.
is it ok for you ?
Neil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
