| Message ID | 20200206191834.6125-2-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/bridge: dw-hdmi: implement bus-format negotiation and YUV420 support | expand |
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@kwiboo.se> > > Configure the correct mtmdsclock for deep colors to prepare support > for 10, 12 & 16bit output. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > Signed-off-by: Neil Armstrong <narmstrong@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) &&
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@kwiboo.se> >> >> Configure the correct mtmdsclock for deep colors to prepare support >> for 10, 12 & 16bit output. >> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >> Signed-off-by: Neil Armstrong <narmstrong@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 ? 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 <jonas@kwiboo.se> >> >> Configure the correct mtmdsclock for deep colors to prepare support >> for 10, 12 & 16bit output. >> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >> Signed-off-by: Neil Armstrong <narmstrong@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
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 <jonas@kwiboo.se> > >> > >> Configure the correct mtmdsclock for deep colors to prepare support > >> for 10, 12 & 16bit output. > >> > >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > >> Signed-off-by: Neil Armstrong <narmstrong@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 ? Sure, works for me.
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) &&