| Message ID | 20200901220305.6809-1-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: Fix DE2 YVU handling | expand |
ср, 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
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;
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(-)