Function sun8i_vi_layer_get_csc_mode() is supposed to return CSC mode but due to inproper return type (bool instead of u32) it returns just 0 or 1. Colors are wrong for YVU formats because of that.
Fixes: daab3d0e8e2b ("drm/sun4i: de2: csc_mode in de2 format struct is mostly redundant") Reported-by: Roman Stratiienko r.stratiienko@gmail.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 22c8c5375d0d..c0147af6a840 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -211,7 +211,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel, return 0; }
-static bool sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format) +static u32 sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format) { if (!format->is_yuv) return SUN8I_CSC_MODE_OFF;
ср, 2 сент. 2020 г. в 00:58, Jernej Skrabec jernej.skrabec@siol.net:
Function sun8i_vi_layer_get_csc_mode() is supposed to return CSC mode but due to inproper return type (bool instead of u32) it returns just 0 or 1. Colors are wrong for YVU formats because of that.
Fixes: daab3d0e8e2b ("drm/sun4i: de2: csc_mode in de2 format struct is mostly redundant") Reported-by: Roman Stratiienko r.stratiienko@gmail.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 22c8c5375d0d..c0147af6a840 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -211,7 +211,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel, return 0; }
-static bool sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format) +static u32 sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format) { if (!format->is_yuv) return SUN8I_CSC_MODE_OFF; -- 2.28.0
Hi Jernej,
Thank you for the fix. I can confirm this patch fixes the issue with wrong colors.
Let me share my thoughts: I've looked into csc code, and it seems to me reordering U, V offsets should be a much simpler solution than applying color transformation matrices.It should also simplify adding more color encodings in the future.
Regards, Roman
Dne sreda, 02. september 2020 ob 09:01:17 CEST je Roman Stratiienko napisal(a):
ср, 2 сент. 2020 г. в 00:58, Jernej Skrabec jernej.skrabec@siol.net:
Function sun8i_vi_layer_get_csc_mode() is supposed to return CSC mode but due to inproper return type (bool instead of u32) it returns just 0 or 1. Colors are wrong for YVU formats because of that.
Fixes: daab3d0e8e2b ("drm/sun4i: de2: csc_mode in de2 format struct is mostly redundant") Reported-by: Roman Stratiienko r.stratiienko@gmail.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 22c8c5375d0d..c0147af6a840 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -211,7 +211,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,> return 0;
}
-static bool sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format) +static u32 sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format)> {
if (!format->is_yuv) return SUN8I_CSC_MODE_OFF;-- 2.28.0
Hi Jernej,
Thank you for the fix. I can confirm this patch fixes the issue with wrong colors.
Thanks! Can I assume that this means your Tested-by tag can be added?
Let me share my thoughts: I've looked into csc code, and it seems to me reordering U, V offsets should be a much simpler solution than applying color transformation matrices.It should also simplify adding more color encodings in the future.
Switching offsets assumes that you have separate planes for U and V which may not be true in the future. I agree that CSC matrices are needlessly duplicated for handling U/V switch. I have a patch which reorganize matrix on the fly when coefficients are written in registers but since it's a part of a bigger, unfinished series, I didn't sent it out yet. Only difference in YUV and YVU CSC matrices are switched 2nd and 3rd column.
Best regards, Jernej
Regards, Roman
ср, 2 сент. 2020 г. в 19:46, Jernej Škrabec jernej.skrabec@siol.net:
Dne sreda, 02. september 2020 ob 09:01:17 CEST je Roman Stratiienko napisal(a):
ср, 2 сент. 2020 г. в 00:58, Jernej Skrabec jernej.skrabec@siol.net:
Function sun8i_vi_layer_get_csc_mode() is supposed to return CSC mode but due to inproper return type (bool instead of u32) it returns just 0 or 1. Colors are wrong for YVU formats because of that.
Fixes: daab3d0e8e2b ("drm/sun4i: de2: csc_mode in de2 format struct is mostly redundant") Reported-by: Roman Stratiienko r.stratiienko@gmail.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 22c8c5375d0d..c0147af6a840 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -211,7 +211,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,> return 0;
}
-static bool sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format) +static u32 sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format)> {
if (!format->is_yuv) return SUN8I_CSC_MODE_OFF;-- 2.28.0
Hi Jernej,
Thank you for the fix. I can confirm this patch fixes the issue with wrong colors.
Thanks! Can I assume that this means your Tested-by tag can be added?
Sure.
Tested-by: Roman Stratiienko r.stratiienko@gmail.com
Let me share my thoughts: I've looked into csc code, and it seems to me reordering U, V offsets should be a much simpler solution than applying color transformation matrices.It should also simplify adding more color encodings in the future.
Switching offsets assumes that you have separate planes for U and V which may not be true in the future. I agree that CSC matrices are needlessly duplicated for handling U/V switch. I have a patch which reorganize matrix on the fly when coefficients are written in registers but since it's a part of a bigger, unfinished series, I didn't sent it out yet. Only difference in YUV and YVU CSC matrices are switched 2nd and 3rd column.
This sounds reasonable. Thank you for your explanation.
Regards, Roman
Best regards, Jernej
Regards, Roman
On Wed, Sep 02, 2020 at 12:03:05AM +0200, Jernej Skrabec wrote:
Function sun8i_vi_layer_get_csc_mode() is supposed to return CSC mode but due to inproper return type (bool instead of u32) it returns just 0 or 1. Colors are wrong for YVU formats because of that.
Fixes: daab3d0e8e2b ("drm/sun4i: de2: csc_mode in de2 format struct is mostly redundant") Reported-by: Roman Stratiienko r.stratiienko@gmail.com Signed-off-by: Jernej Skrabec jernej.skrabec@siol.net
Applied, thanks! Maxime
dri-devel@lists.freedesktop.org
-
Jernej Skrabec -
Jernej Škrabec
-
Maxime Ripard -
Roman Stratiienko