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 <[email protected]>
Signed-off-by: Jernej Skrabec <[email protected]>
---
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
ср, 2 сент. 2020 г. в 00:58, Jernej Skrabec <[email protected]>:
>
> 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 <[email protected]>
> Signed-off-by: Jernej Skrabec <[email protected]>
> ---
> 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 <[email protected]>:
> > 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
> > <[email protected]>
> > Signed-off-by: Jernej Skrabec <[email protected]>
> > ---
> >
> > 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 <[email protected]>:
>
> Dne sreda, 02. september 2020 ob 09:01:17 CEST je Roman Stratiienko
> napisal(a):
> > ср, 2 сент. 2020 г. в 00:58, Jernej Skrabec <[email protected]>:
> > > 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
> > > <[email protected]>
> > > Signed-off-by: Jernej Skrabec <[email protected]>
> > > ---
> > >
> > > 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 <[email protected]>
>
> >
> > 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 <[email protected]>
> Signed-off-by: Jernej Skrabec <[email protected]>
Applied, thanks!
Maxime