diff mbox series

[v2,4/4] drm/bridge: dw-hdmi: rework csc related functions

Message ID 20200304232512.51616-5-jernej.skrabec@siol.net (mailing list archive)
State New, archived
Headers show
Series drm/bridge: dw-hdmi: Various updates | expand

Commit Message

Jernej Škrabec March 4, 2020, 11:25 p.m. UTC 
is_color_space_conversion() is a misnomer. It checks not only if color
space conversion is needed, but also if format conversion is needed.
This is actually desired behaviour because result of this function
determines if CSC block should be enabled or not (CSC block can also do
format conversion).

In order to clear misunderstandings, let's rework
is_color_space_conversion() to do exactly what is supposed to do and add
another function which will determine if CSC block must be enabled or
not.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 31 +++++++++++++++--------
 1 file changed, 21 insertions(+), 10 deletions(-)

Comments

Jernej Škrabec March 5, 2020, 4:53 p.m. UTC | #1 
Hi Laurent,

Dne četrtek, 05. marec 2020 ob 00:51:49 CET je Laurent Pinchart napisal(a):
> Hi Jernej,
> 
> Thank you for the patch.
> 
> On Thu, Mar 05, 2020 at 12:25:12AM +0100, Jernej Skrabec wrote:
> > is_color_space_conversion() is a misnomer. It checks not only if color
> > space conversion is needed, but also if format conversion is needed.
> > This is actually desired behaviour because result of this function
> > determines if CSC block should be enabled or not (CSC block can also do
> > format conversion).
> > 
> > In order to clear misunderstandings, let's rework
> > is_color_space_conversion() to do exactly what is supposed to do and add
> > another function which will determine if CSC block must be enabled or
> > not.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 31 +++++++++++++++--------
> >  1 file changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > c8a02e5b5e1b..7724191e0a8b 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -963,11 +963,14 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
> > 
> >  static int is_color_space_conversion(struct dw_hdmi *hdmi)
> >  {
> > 
> > -	return (hdmi->hdmi_data.enc_in_bus_format !=
> > -			hdmi->hdmi_data.enc_out_bus_format) ||
> > -	       (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
> > -		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format) 
&&
> > -		hdmi->hdmi_data.rgb_limited_range);
> > +	struct hdmi_data_info *hdmi_data = &hdmi->hdmi_data;
> > +	bool is_input_rgb, is_output_rgb;
> > +
> > +	is_input_rgb = hdmi_bus_fmt_is_rgb(hdmi_data->enc_in_bus_format);
> > +	is_output_rgb = hdmi_bus_fmt_is_rgb(hdmi_data-
>enc_out_bus_format);
> > +
> > +	return (is_input_rgb != is_output_rgb) ||
> > +	       (is_input_rgb && is_output_rgb && hdmi_data-
>rgb_limited_range);
> > 
> >  }
> >  
> >  static int is_color_space_decimation(struct dw_hdmi *hdmi)
> > 
> > @@ -994,6 +997,13 @@ static int is_color_space_interpolation(struct
> > dw_hdmi *hdmi)> 
> >  	return 0;
> >  
> >  }
> > 
> > +static bool is_conversion_needed(struct dw_hdmi *hdmi)
> 
> Maybe is_csc_needed ?

Ok, I'll fix during applying.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Best regards,
Jernej

> 
> > +{
> > +	return is_color_space_conversion(hdmi) ||
> > +	       is_color_space_decimation(hdmi) ||
> > +	       is_color_space_interpolation(hdmi);
> > +}
> > +
> > 
> >  static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
> >  {
> >  
> >  	const u16 (*csc_coeff)[3][4] = &csc_coeff_default;
> > 
> > @@ -2014,18 +2024,19 @@ static void dw_hdmi_enable_video_path(struct
> > dw_hdmi *hdmi)> 
> >  	hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
> >  	
> >  	/* Enable csc path */
> > 
> > -	if (is_color_space_conversion(hdmi)) {
> > +	if (is_conversion_needed(hdmi)) {
> > 
> >  		hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
> >  		hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
> > 
> > -	}
> > 
> > -	/* Enable color space conversion if needed */
> > -	if (is_color_space_conversion(hdmi))
> > 
> >  		hdmi_writeb(hdmi, 
HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
> >  		
> >  			    HDMI_MC_FLOWCTRL);
> > 
> > -	else
> > +	} else {
> > +		hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CSCCLK_DISABLE;
> > +		hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
> > +
> > 
> >  		hdmi_writeb(hdmi, 
HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_BYPASS,
> >  		
> >  			    HDMI_MC_FLOWCTRL);
> > 
> > +	}
> > 
> >  }
> >  
> >  /* Workaround to clear the overflow condition */
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index c8a02e5b5e1b..7724191e0a8b 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -963,11 +963,14 @@  static void hdmi_video_sample(struct dw_hdmi *hdmi)
 
 static int is_color_space_conversion(struct dw_hdmi *hdmi)
 {
-	return (hdmi->hdmi_data.enc_in_bus_format !=
-			hdmi->hdmi_data.enc_out_bus_format) ||
-	       (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
-		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format) &&
-		hdmi->hdmi_data.rgb_limited_range);
+	struct hdmi_data_info *hdmi_data = &hdmi->hdmi_data;
+	bool is_input_rgb, is_output_rgb;
+
+	is_input_rgb = hdmi_bus_fmt_is_rgb(hdmi_data->enc_in_bus_format);
+	is_output_rgb = hdmi_bus_fmt_is_rgb(hdmi_data->enc_out_bus_format);
+
+	return (is_input_rgb != is_output_rgb) ||
+	       (is_input_rgb && is_output_rgb && hdmi_data->rgb_limited_range);
 }
 
 static int is_color_space_decimation(struct dw_hdmi *hdmi)
@@ -994,6 +997,13 @@  static int is_color_space_interpolation(struct dw_hdmi *hdmi)
 	return 0;
 }
 
+static bool is_conversion_needed(struct dw_hdmi *hdmi)
+{
+	return is_color_space_conversion(hdmi) ||
+	       is_color_space_decimation(hdmi) ||
+	       is_color_space_interpolation(hdmi);
+}
+
 static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
 {
 	const u16 (*csc_coeff)[3][4] = &csc_coeff_default;
@@ -2014,18 +2024,19 @@  static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 	hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
 
 	/* Enable csc path */
-	if (is_color_space_conversion(hdmi)) {
+	if (is_conversion_needed(hdmi)) {
 		hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
 		hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
-	}
 
-	/* Enable color space conversion if needed */
-	if (is_color_space_conversion(hdmi))
 		hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
 			    HDMI_MC_FLOWCTRL);
-	else
+	} else {
+		hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CSCCLK_DISABLE;
+		hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
+
 		hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_BYPASS,
 			    HDMI_MC_FLOWCTRL);
+	}
 }
 
 /* Workaround to clear the overflow condition */