From: Jonas Karlman <[email protected]>
Configure the correct mtmdsclock for deep colors to prepare support
for 10, 12 & 16bit output.
Signed-off-by: Jonas Karlman <[email protected]>
Signed-off-by: Neil Armstrong <[email protected]>
---
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);
+ 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;
+ 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);
+
/* Set up HDMI_FC_INVIDCONF */
inv_val = (hdmi->hdmi_data.hdcp_enable ||
(dw_hdmi_support_scdc(hdmi) &&
--
2.22.0
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 <[email protected]>
>
> Configure the correct mtmdsclock for deep colors to prepare support
> for 10, 12 & 16bit output.
>
> Signed-off-by: Jonas Karlman <[email protected]>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> 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) &&
--
Regards,
Laurent Pinchart
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 <[email protected]>
>>
>> Configure the correct mtmdsclock for deep colors to prepare support
>> for 10, 12 & 16bit output.
>>
>> Signed-off-by: Jonas Karlman <[email protected]>
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>
>> Configure the correct mtmdsclock for deep colors to prepare support
>> for 10, 12 & 16bit output.
>>
>> Signed-off-by: Jonas Karlman <[email protected]>
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>> 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
Hi Neil,
On Mon, Mar 02, 2020 at 04:54:17PM +0100, Neil Armstrong wrote:
> On 02/03/2020 10:05, Laurent Pinchart wrote:
> > On Thu, Feb 06, 2020 at 08:18:24PM +0100, Neil Armstrong wrote:
> >> From: Jonas Karlman <[email protected]>
> >>
> >> Configure the correct mtmdsclock for deep colors to prepare support
> >> for 10, 12 & 16bit output.
> >>
> >> Signed-off-by: Jonas Karlman <[email protected]>
> >> Signed-off-by: Neil Armstrong <[email protected]>
> >> ---
> >> 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 ?
Sure, works for me.
--
Regards,
Laurent Pinchart