| Message ID | 20200229163043.158262-3-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: dw-hdmi: Various updates | expand |
Hi Jernej, On 29/02/2020 17:30, Jernej Skrabec wrote: > Currently, is_color_space_conversion() compares not only color spaces > but also formats. For example, function would return true if YCbCr 4:4:4 > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are > the same. > > Fix that by comparing if both values represent RGB color space. > > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi") > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 24965e53d351..9d7bfb1cb213 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -956,7 +956,8 @@ 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; > + return hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) != > + hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format); > } > > static int is_color_space_decimation(struct dw_hdmi *hdmi) > I think in this case you should also fix the CEC enablement to: if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi)) in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be bypassed in decimation case only. Neil
Hi Jernej, Thank you for the patch. On Sat, Feb 29, 2020 at 05:30:41PM +0100, Jernej Skrabec wrote: > Currently, is_color_space_conversion() compares not only color spaces > but also formats. For example, function would return true if YCbCr 4:4:4 > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are > the same. > > Fix that by comparing if both values represent RGB color space. > > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi") > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> This isn't implemented today, but could the CSC be used to convert between different YCbCr encodings ? In any case the patch is correct based on the current implementation, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 24965e53d351..9d7bfb1cb213 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -956,7 +956,8 @@ 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; > + return hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) != > + hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format); > } > > static int is_color_space_decimation(struct dw_hdmi *hdmi)
Hi Neil! Dne ponedeljek, 02. marec 2020 ob 10:26:09 CET je Neil Armstrong napisal(a): > Hi Jernej, > > On 29/02/2020 17:30, Jernej Skrabec wrote: > > Currently, is_color_space_conversion() compares not only color spaces > > but also formats. For example, function would return true if YCbCr 4:4:4 > > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are > > the same. > > > > Fix that by comparing if both values represent RGB color space. > > > > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to > > bridge/dw_hdmi") > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > 24965e53d351..9d7bfb1cb213 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -956,7 +956,8 @@ 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; + return > > hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) != > > + hdmi_bus_fmt_is_rgb(hdmi- >hdmi_data.enc_out_bus_format); > > > > } > > > > static int is_color_space_decimation(struct dw_hdmi *hdmi) > > I think in this case you should also fix the CEC enablement to: you mean CSC, right? > if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi)) > > in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be > bypassed in decimation case only. > Missed that one. I'll fix in v2. Best regards, Jernej
Dne ponedeljek, 02. marec 2020 ob 10:27:48 CET je Laurent Pinchart napisal(a): > Hi Jernej, > > Thank you for the patch. > > On Sat, Feb 29, 2020 at 05:30:41PM +0100, Jernej Skrabec wrote: > > Currently, is_color_space_conversion() compares not only color spaces > > but also formats. For example, function would return true if YCbCr 4:4:4 > > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are > > the same. > > > > Fix that by comparing if both values represent RGB color space. > > > > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to > > bridge/dw_hdmi") > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > This isn't implemented today, but could the CSC be used to convert > between different YCbCr encodings ? Yes, CSC offers great flexibility, but unfortunately that also means that you have as much CSC matrices as there is possible conversions. This explodes quickly, especially if you convert from one YCbCr encoding to another (BT.601, BT.709, BT.2020) and also considering range (full, limited). If you don't mind doing some calculations in code, this becames much simpler, but doing fixed point arithmetic isn't fun. Is floating point arithmetic allowed in kernel? I wrote a simple program to produce all those CSC matrices for sun4i-drm driver: http://ix.io/2dak Note that it's for RGB <-> YUV conversion, but DW HDMI has a bit different order. I believe it's GRB, but I'm not 100% sure. You can also do various color adjustements, like brigthness, but that would also mean that you have to multiply all matrices to get final one which you can then write into registers. Best regards, Jernej > > In any case the patch is correct based on the current implementation, so > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > 24965e53d351..9d7bfb1cb213 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -956,7 +956,8 @@ 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; + return > > hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) != > > + hdmi_bus_fmt_is_rgb(hdmi- >hdmi_data.enc_out_bus_format); > > > > } > > > > static int is_color_space_decimation(struct dw_hdmi *hdmi)
Dne ponedeljek, 02. marec 2020 ob 10:26:09 CET je Neil Armstrong napisal(a): > Hi Jernej, > > On 29/02/2020 17:30, Jernej Skrabec wrote: > > Currently, is_color_space_conversion() compares not only color spaces > > but also formats. For example, function would return true if YCbCr 4:4:4 > > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are > > the same. > > > > Fix that by comparing if both values represent RGB color space. > > > > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to > > bridge/dw_hdmi") > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > 24965e53d351..9d7bfb1cb213 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -956,7 +956,8 @@ 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; + return > > hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) != > > + hdmi_bus_fmt_is_rgb(hdmi- >hdmi_data.enc_out_bus_format); > > > > } > > > > static int is_color_space_decimation(struct dw_hdmi *hdmi) > > I think in this case you should also fix the CEC enablement to: > if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi)) > > in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be > bypassed in decimation case only. On second thought, I think original implementation is correct, just misnamed. Laurent, Neil, do you agree if I replace this patch with patch which renames is_color_space_conversion() to is_conversion_needed() ? Best regards, Jernej
On 02/03/2020 17:42, Jernej Škrabec wrote: > Dne ponedeljek, 02. marec 2020 ob 10:26:09 CET je Neil Armstrong napisal(a): >> Hi Jernej, >> >> On 29/02/2020 17:30, Jernej Skrabec wrote: >>> Currently, is_color_space_conversion() compares not only color spaces >>> but also formats. For example, function would return true if YCbCr 4:4:4 >>> and YCbCr 4:2:2 would be set. Obviously in that case color spaces are >>> the same. >>> >>> Fix that by comparing if both values represent RGB color space. >>> >>> Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to >>> bridge/dw_hdmi") >>> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> >>> --- >>> >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >>> 24965e53d351..9d7bfb1cb213 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> @@ -956,7 +956,8 @@ 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; + return >>> hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) != >>> + hdmi_bus_fmt_is_rgb(hdmi- >> hdmi_data.enc_out_bus_format); >>> >>> } >>> >>> static int is_color_space_decimation(struct dw_hdmi *hdmi) >> >> I think in this case you should also fix the CEC enablement to: >> if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi)) >> >> in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be >> bypassed in decimation case only. > > On second thought, I think original implementation is correct, just misnamed. > Laurent, Neil, do you agree if I replace this patch with patch which renames > is_color_space_conversion() to is_conversion_needed() ? Sure, Neil > > Best regards, > Jernej > > >
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 24965e53d351..9d7bfb1cb213 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -956,7 +956,8 @@ 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; + return hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) != + hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format); } static int is_color_space_decimation(struct dw_hdmi *hdmi)
Currently, is_color_space_conversion() compares not only color spaces but also formats. For example, function would return true if YCbCr 4:4:4 and YCbCr 4:2:2 would be set. Obviously in that case color spaces are the same. Fix that by comparing if both values represent RGB color space. Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi") Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)