[v8,09/14] drm: bridge: samsung-dsim: Add atomic_get_input_bus_fmts

Finding the right input bus format throughout the pipeline is hard
so add atomic_get_input_bus_fmts callback and initialize with the
proper input format from list of supported output formats.

This format can be used in pipeline for negotiating bus format between
the DSI-end of this bridge and the other component closer to pipeline
components.

List of Pixel formats are taken from,
AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
3.7.4 Pixel formats
Table 14. DSI pixel packing formats

v8:
* added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2

v7, v6, v5, v4:
* none

v3:
* include media-bus-format.h

v2:
* none

v1:
* new patch

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
  

On 11/10/22 19:38, Jagan Teki wrote:
> Finding the right input bus format throughout the pipeline is hard
> so add atomic_get_input_bus_fmts callback and initialize with the
> proper input format from list of supported output formats.
> 
> This format can be used in pipeline for negotiating bus format between
> the DSI-end of this bridge and the other component closer to pipeline
> components.
> 
> List of Pixel formats are taken from,
> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> 3.7.4 Pixel formats
> Table 14. DSI pixel packing formats
> 
> v8:
> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
> 
> v7, v6, v5, v4:
> * none
> 
> v3:
> * include media-bus-format.h
> 
> v2:
> * none
> 
> v1:
> * new patch
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 0fe153b29e4f..33e5ae9c865f 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -15,6 +15,7 @@
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/irq.h>
> +#include <linux/media-bus-format.h>
>   #include <linux/of_device.h>
>   #include <linux/phy/phy.h>
>   
> @@ -1321,6 +1322,57 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>   	pm_runtime_put_sync(dsi->dev);
>   }
>   
> +/*
> + * This pixel output formats list referenced from,
> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> + * 3.7.4 Pixel formats
> + * Table 14. DSI pixel packing formats
> + */
> +static const u32 samsung_dsim_pixel_output_fmts[] = {

You can also add :

MEDIA_BUS_FMT_YUYV10_1X20

MEDIA_BUS_FMT_YUYV12_1X24

> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +	MEDIA_BUS_FMT_RGB101010_1X30,
> +	MEDIA_BUS_FMT_RGB121212_1X36,
> +	MEDIA_BUS_FMT_RGB565_1X16,
> +	MEDIA_BUS_FMT_RGB666_1X18,
> +	MEDIA_BUS_FMT_RGB888_1X24,
> +};
> +
> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
> +		if (samsung_dsim_pixel_output_fmts[i] == fmt)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static u32 *
> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> +				       struct drm_bridge_state *bridge_state,
> +				       struct drm_crtc_state *crtc_state,
> +				       struct drm_connector_state *conn_state,
> +				       u32 output_fmt,
> +				       unsigned int *num_input_fmts)
> +{
> +	u32 *input_fmts;
> +
> +	if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> +		return NULL;
> +
> +	*num_input_fmts = 1;

Shouldn't this be 6 ?

> +	input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	input_fmts[0] = output_fmt;

Shouldn't this be a list of all 6 supported pixel formats ?

(replace 6 with 8 with the two new YUYV10/YUYV12 pixel formats above)
  

On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote:
>
> On 11/10/22 19:38, Jagan Teki wrote:
> > Finding the right input bus format throughout the pipeline is hard
> > so add atomic_get_input_bus_fmts callback and initialize with the
> > proper input format from list of supported output formats.
> >
> > This format can be used in pipeline for negotiating bus format between
> > the DSI-end of this bridge and the other component closer to pipeline
> > components.
> >
> > List of Pixel formats are taken from,
> > AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> > 3.7.4 Pixel formats
> > Table 14. DSI pixel packing formats
> >
> > v8:
> > * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
> >
> > v7, v6, v5, v4:
> > * none
> >
> > v3:
> > * include media-bus-format.h
> >
> > v2:
> > * none
> >
> > v1:
> > * new patch
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >   drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
> >   1 file changed, 53 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 0fe153b29e4f..33e5ae9c865f 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/clk.h>
> >   #include <linux/delay.h>
> >   #include <linux/irq.h>
> > +#include <linux/media-bus-format.h>
> >   #include <linux/of_device.h>
> >   #include <linux/phy/phy.h>
> >
> > @@ -1321,6 +1322,57 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> >       pm_runtime_put_sync(dsi->dev);
> >   }
> >
> > +/*
> > + * This pixel output formats list referenced from,
> > + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> > + * 3.7.4 Pixel formats
> > + * Table 14. DSI pixel packing formats
> > + */
> > +static const u32 samsung_dsim_pixel_output_fmts[] = {
>
> You can also add :
>
> MEDIA_BUS_FMT_YUYV10_1X20
>
> MEDIA_BUS_FMT_YUYV12_1X24

Are these for the below formats?

"Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2
 Packed Pixel Stream, 24-bit YCbCr, 4:2:2"
>
> > +     MEDIA_BUS_FMT_UYVY8_1X16,
> > +     MEDIA_BUS_FMT_RGB101010_1X30,
> > +     MEDIA_BUS_FMT_RGB121212_1X36,
> > +     MEDIA_BUS_FMT_RGB565_1X16,
> > +     MEDIA_BUS_FMT_RGB666_1X18,
> > +     MEDIA_BUS_FMT_RGB888_1X24,
> > +};
> > +
> > +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
> > +             if (samsung_dsim_pixel_output_fmts[i] == fmt)
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static u32 *
> > +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> > +                                    struct drm_bridge_state *bridge_state,
> > +                                    struct drm_crtc_state *crtc_state,
> > +                                    struct drm_connector_state *conn_state,
> > +                                    u32 output_fmt,
> > +                                    unsigned int *num_input_fmts)
> > +{
> > +     u32 *input_fmts;
> > +
> > +     if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> > +             return NULL;
> > +
> > +     *num_input_fmts = 1;
>
> Shouldn't this be 6 ?
>
> > +     input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> > +     if (!input_fmts)
> > +             return NULL;
> > +
> > +     input_fmts[0] = output_fmt;
>
> Shouldn't this be a list of all 6 supported pixel formats ?

Negotiation would settle to return one input_fmt from the list of
supporting output_fmts. so the num_input_fmts would be 1 rather than
the number of fmts in the supporting list. This is what I understood
from the atomic_get_input_bus_fmts API. let me know if I miss
something here.

Jagan.
  

On 10.11.2022 19:38, Jagan Teki wrote:
> Finding the right input bus format throughout the pipeline is hard
> so add atomic_get_input_bus_fmts callback and initialize with the
> proper input format from list of supported output formats.
>
> This format can be used in pipeline for negotiating bus format between
> the DSI-end of this bridge and the other component closer to pipeline
> components.
>
> List of Pixel formats are taken from,
> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> 3.7.4 Pixel formats
> Table 14. DSI pixel packing formats
>
> v8:
> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
>
> v7, v6, v5, v4:
> * none
>
> v3:
> * include media-bus-format.h
>
> v2:
> * none
>
> v1:
> * new patch
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 0fe153b29e4f..33e5ae9c865f 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -15,6 +15,7 @@
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/irq.h>
> +#include <linux/media-bus-format.h>
>   #include <linux/of_device.h>
>   #include <linux/phy/phy.h>
>   
> @@ -1321,6 +1322,57 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>   	pm_runtime_put_sync(dsi->dev);
>   }
>   
> +/*
> + * This pixel output formats list referenced from,
> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> + * 3.7.4 Pixel formats
> + * Table 14. DSI pixel packing formats
> + */
> +static const u32 samsung_dsim_pixel_output_fmts[] = {
> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +	MEDIA_BUS_FMT_RGB101010_1X30,
> +	MEDIA_BUS_FMT_RGB121212_1X36,
> +	MEDIA_BUS_FMT_RGB565_1X16,
> +	MEDIA_BUS_FMT_RGB666_1X18,
> +	MEDIA_BUS_FMT_RGB888_1X24,
> +};
> +
> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
> +		if (samsung_dsim_pixel_output_fmts[i] == fmt)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static u32 *
> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> +				       struct drm_bridge_state *bridge_state,
> +				       struct drm_crtc_state *crtc_state,
> +				       struct drm_connector_state *conn_state,
> +				       u32 output_fmt,
> +				       unsigned int *num_input_fmts)
> +{
> +	u32 *input_fmts;
> +
> +	if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> +		return NULL;


Please add support for MEDIA_BUS_FMT_FIXED and maybe default to 
MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.

Otherwise the above check breaks all current clients of the Samsung 
DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all 
DSI panels requests such format on my test systems...


> +
> +	*num_input_fmts = 1;
> +
> +	input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	input_fmts[0] = output_fmt;
> +
> +	return input_fmts;
> +}
> +
>   static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
>   				     struct drm_bridge_state *bridge_state,
>   				     struct drm_crtc_state *crtc_state,
> @@ -1385,6 +1437,7 @@ static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
>   	.atomic_duplicate_state		= drm_atomic_helper_bridge_duplicate_state,
>   	.atomic_destroy_state		= drm_atomic_helper_bridge_destroy_state,
>   	.atomic_reset			= drm_atomic_helper_bridge_reset,
> +	.atomic_get_input_bus_fmts	= samsung_dsim_atomic_get_input_bus_fmts,
>   	.atomic_check			= samsung_dsim_atomic_check,
>   	.atomic_pre_enable		= samsung_dsim_atomic_pre_enable,
>   	.atomic_enable			= samsung_dsim_atomic_enable,

Best regards
  

On 14.11.2022 11:57, Marek Szyprowski wrote:
> On 10.11.2022 19:38, Jagan Teki wrote:
>> Finding the right input bus format throughout the pipeline is hard
>> so add atomic_get_input_bus_fmts callback and initialize with the
>> proper input format from list of supported output formats.
>>
>> This format can be used in pipeline for negotiating bus format between
>> the DSI-end of this bridge and the other component closer to pipeline
>> components.
>>
>> List of Pixel formats are taken from,
>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>> 3.7.4 Pixel formats
>> Table 14. DSI pixel packing formats
>>
>> v8:
>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
>>
>> v7, v6, v5, v4:
>> * none
>>
>> v3:
>> * include media-bus-format.h
>>
>> v2:
>> * none
>>
>> v1:
>> * new patch
>>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>> ---
>>   drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 0fe153b29e4f..33e5ae9c865f 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <linux/irq.h>
>> +#include <linux/media-bus-format.h>
>>   #include <linux/of_device.h>
>>   #include <linux/phy/phy.h>
>>   @@ -1321,6 +1322,57 @@ static void 
>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>       pm_runtime_put_sync(dsi->dev);
>>   }
>>   +/*
>> + * This pixel output formats list referenced from,
>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>> + * 3.7.4 Pixel formats
>> + * Table 14. DSI pixel packing formats
>> + */
>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>> +    MEDIA_BUS_FMT_UYVY8_1X16,
>> +    MEDIA_BUS_FMT_RGB101010_1X30,
>> +    MEDIA_BUS_FMT_RGB121212_1X36,
>> +    MEDIA_BUS_FMT_RGB565_1X16,
>> +    MEDIA_BUS_FMT_RGB666_1X18,
>> +    MEDIA_BUS_FMT_RGB888_1X24,
>> +};
>> +
>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
>> +        if (samsung_dsim_pixel_output_fmts[i] == fmt)
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static u32 *
>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>> +                       struct drm_bridge_state *bridge_state,
>> +                       struct drm_crtc_state *crtc_state,
>> +                       struct drm_connector_state *conn_state,
>> +                       u32 output_fmt,
>> +                       unsigned int *num_input_fmts)
>> +{
>> +    u32 *input_fmts;
>> +
>> +    if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>> +        return NULL;
>
>
> Please add support for MEDIA_BUS_FMT_FIXED and maybe default to 
> MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
>
> Otherwise the above check breaks all current clients of the Samsung 
> DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all 
> DSI panels requests such format on my test systems...

I've checked a bit more the bus format related code and it looks that 
there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_* 
formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for 
them. On Arndale board with Toshiba tc358764 bridge the 
MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in 
samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel, 
but this doesn't look like a format really needed on that bridge). A bit 
more logic seems to be needed there to make it working properly.

I suggest to move all this bus format related changes into a separate 
patchset and adjust other bridges/panels/etc as needed in it.

>
>> +
>> +    *num_input_fmts = 1;
>> +
>> +    input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>> +    if (!input_fmts)
>> +        return NULL;
>> +
>> +    input_fmts[0] = output_fmt;
>> +
>> +    return input_fmts;
>> +}
>> +
>>   static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
>>                        struct drm_bridge_state *bridge_state,
>>                        struct drm_crtc_state *crtc_state,
>> @@ -1385,6 +1437,7 @@ static const struct drm_bridge_funcs 
>> samsung_dsim_bridge_funcs = {
>>       .atomic_duplicate_state        = 
>> drm_atomic_helper_bridge_duplicate_state,
>>       .atomic_destroy_state        = 
>> drm_atomic_helper_bridge_destroy_state,
>>       .atomic_reset            = drm_atomic_helper_bridge_reset,
>> +    .atomic_get_input_bus_fmts    = 
>> samsung_dsim_atomic_get_input_bus_fmts,
>>       .atomic_check            = samsung_dsim_atomic_check,
>>       .atomic_pre_enable        = samsung_dsim_atomic_pre_enable,
>>       .atomic_enable            = samsung_dsim_atomic_enable,
>
> Best regards

Best regards
  

On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 14.11.2022 11:57, Marek Szyprowski wrote:
> > On 10.11.2022 19:38, Jagan Teki wrote:
> >> Finding the right input bus format throughout the pipeline is hard
> >> so add atomic_get_input_bus_fmts callback and initialize with the
> >> proper input format from list of supported output formats.
> >>
> >> This format can be used in pipeline for negotiating bus format between
> >> the DSI-end of this bridge and the other component closer to pipeline
> >> components.
> >>
> >> List of Pixel formats are taken from,
> >> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> >> 3.7.4 Pixel formats
> >> Table 14. DSI pixel packing formats
> >>
> >> v8:
> >> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
> >>
> >> v7, v6, v5, v4:
> >> * none
> >>
> >> v3:
> >> * include media-bus-format.h
> >>
> >> v2:
> >> * none
> >>
> >> v1:
> >> * new patch
> >>
> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >> ---
> >>   drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
> >>   1 file changed, 53 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> >> b/drivers/gpu/drm/bridge/samsung-dsim.c
> >> index 0fe153b29e4f..33e5ae9c865f 100644
> >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >> @@ -15,6 +15,7 @@
> >>   #include <linux/clk.h>
> >>   #include <linux/delay.h>
> >>   #include <linux/irq.h>
> >> +#include <linux/media-bus-format.h>
> >>   #include <linux/of_device.h>
> >>   #include <linux/phy/phy.h>
> >>   @@ -1321,6 +1322,57 @@ static void
> >> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> >>       pm_runtime_put_sync(dsi->dev);
> >>   }
> >>   +/*
> >> + * This pixel output formats list referenced from,
> >> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> >> + * 3.7.4 Pixel formats
> >> + * Table 14. DSI pixel packing formats
> >> + */
> >> +static const u32 samsung_dsim_pixel_output_fmts[] = {
> >> +    MEDIA_BUS_FMT_UYVY8_1X16,
> >> +    MEDIA_BUS_FMT_RGB101010_1X30,
> >> +    MEDIA_BUS_FMT_RGB121212_1X36,
> >> +    MEDIA_BUS_FMT_RGB565_1X16,
> >> +    MEDIA_BUS_FMT_RGB666_1X18,
> >> +    MEDIA_BUS_FMT_RGB888_1X24,
> >> +};
> >> +
> >> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
> >> +        if (samsung_dsim_pixel_output_fmts[i] == fmt)
> >> +            return true;
> >> +    }
> >> +
> >> +    return false;
> >> +}
> >> +
> >> +static u32 *
> >> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> >> +                       struct drm_bridge_state *bridge_state,
> >> +                       struct drm_crtc_state *crtc_state,
> >> +                       struct drm_connector_state *conn_state,
> >> +                       u32 output_fmt,
> >> +                       unsigned int *num_input_fmts)
> >> +{
> >> +    u32 *input_fmts;
> >> +
> >> +    if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> >> +        return NULL;
> >
> >
> > Please add support for MEDIA_BUS_FMT_FIXED and maybe default to
> > MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
> >
> > Otherwise the above check breaks all current clients of the Samsung
> > DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all
> > DSI panels requests such format on my test systems...
>
> I've checked a bit more the bus format related code and it looks that
> there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_*
> formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for
> them. On Arndale board with Toshiba tc358764 bridge the
> MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in
> samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel,

dsim => tc358764 => panel-simple

If I understand the bridge format negotiation correctly - If
atomic_get_input_bus_fmts is not implemented in tc358764 then
MEDIA_BUS_FMT_FIXED will be the output_fmt for dsim so we can assign
MEDIA_BUS_FMT_RGB888_1X24 for FIXED formats.

from include/drm/drm_bridge.h:

         * This method is called on all elements of the bridge chain as part of
         * the bus format negotiation process that happens in
         * drm_atomic_bridge_chain_select_bus_fmts().
         * This method is optional. When not implemented, the core will bypass
         * bus format negotiation on this element of the bridge without
         * failing, and the previous element in the chain will be passed
         * MEDIA_BUS_FMT_FIXED as its output bus format.

As I can see tc358764 is not implemented either
atomic_get_input_bus_fmts or atomic API, so I think dsim gets the
MEDIA_BUS_FMT_FIXED bridge pipeline. I have tested sn65dsi without
atomic_get_input_bus_fmts I can see the dsim is getting
MEDIA_BUS_FMT_FIXED.

Can you check the same from your side?

On the other side, MEDIA_BUS_FMT_RGB888_1X7X4_SPWG is 24-bit samples
transferred over an LVDS bus with four differential data pairs,
serialized into 7-time slots using SPWG which indeed a
MEDIA_BUS_FMT_RGB888_1X24 input_fmt for the bridge. so handling in the
supported list and reassigning the RGB888_1X24 would be the proper way
to handle this format.

Jagan.
  

Hi Jagan,

On 14.11.2022 18:07, Jagan Teki wrote:
> On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 14.11.2022 11:57, Marek Szyprowski wrote:
>>> On 10.11.2022 19:38, Jagan Teki wrote:
>>>> Finding the right input bus format throughout the pipeline is hard
>>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>>> proper input format from list of supported output formats.
>>>>
>>>> This format can be used in pipeline for negotiating bus format between
>>>> the DSI-end of this bridge and the other component closer to pipeline
>>>> components.
>>>>
>>>> List of Pixel formats are taken from,
>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>> 3.7.4 Pixel formats
>>>> Table 14. DSI pixel packing formats
>>>>
>>>> v8:
>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
>>>>
>>>> v7, v6, v5, v4:
>>>> * none
>>>>
>>>> v3:
>>>> * include media-bus-format.h
>>>>
>>>> v2:
>>>> * none
>>>>
>>>> v1:
>>>> * new patch
>>>>
>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>> ---
>>>>    drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
>>>>    1 file changed, 53 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/clk.h>
>>>>    #include <linux/delay.h>
>>>>    #include <linux/irq.h>
>>>> +#include <linux/media-bus-format.h>
>>>>    #include <linux/of_device.h>
>>>>    #include <linux/phy/phy.h>
>>>>    @@ -1321,6 +1322,57 @@ static void
>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>        pm_runtime_put_sync(dsi->dev);
>>>>    }
>>>>    +/*
>>>> + * This pixel output formats list referenced from,
>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>> + * 3.7.4 Pixel formats
>>>> + * Table 14. DSI pixel packing formats
>>>> + */
>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>>> +    MEDIA_BUS_FMT_UYVY8_1X16,
>>>> +    MEDIA_BUS_FMT_RGB101010_1X30,
>>>> +    MEDIA_BUS_FMT_RGB121212_1X36,
>>>> +    MEDIA_BUS_FMT_RGB565_1X16,
>>>> +    MEDIA_BUS_FMT_RGB666_1X18,
>>>> +    MEDIA_BUS_FMT_RGB888_1X24,
>>>> +};
>>>> +
>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
>>>> +        if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>>> +            return true;
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>> +static u32 *
>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>> +                       struct drm_bridge_state *bridge_state,
>>>> +                       struct drm_crtc_state *crtc_state,
>>>> +                       struct drm_connector_state *conn_state,
>>>> +                       u32 output_fmt,
>>>> +                       unsigned int *num_input_fmts)
>>>> +{
>>>> +    u32 *input_fmts;
>>>> +
>>>> +    if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>>> +        return NULL;
>>>
>>> Please add support for MEDIA_BUS_FMT_FIXED and maybe default to
>>> MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
>>>
>>> Otherwise the above check breaks all current clients of the Samsung
>>> DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all
>>> DSI panels requests such format on my test systems...
>> I've checked a bit more the bus format related code and it looks that
>> there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_*
>> formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for
>> them. On Arndale board with Toshiba tc358764 bridge the
>> MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in
>> samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel,
> dsim => tc358764 => panel-simple
>
> If I understand the bridge format negotiation correctly - If
> atomic_get_input_bus_fmts is not implemented in tc358764 then
> MEDIA_BUS_FMT_FIXED will be the output_fmt for dsim so we can assign
> MEDIA_BUS_FMT_RGB888_1X24 for FIXED formats.
>
> from include/drm/drm_bridge.h:
>
>           * This method is called on all elements of the bridge chain as part of
>           * the bus format negotiation process that happens in
>           * drm_atomic_bridge_chain_select_bus_fmts().
>           * This method is optional. When not implemented, the core will bypass
>           * bus format negotiation on this element of the bridge without
>           * failing, and the previous element in the chain will be passed
>           * MEDIA_BUS_FMT_FIXED as its output bus format.
>
> As I can see tc358764 is not implemented either
> atomic_get_input_bus_fmts or atomic API, so I think dsim gets the
> MEDIA_BUS_FMT_FIXED bridge pipeline. I have tested sn65dsi without
> atomic_get_input_bus_fmts I can see the dsim is getting
> MEDIA_BUS_FMT_FIXED.
>
> Can you check the same from your side?

Here in case of Arndale 5250 with the following pipeline:

dsim => tc358764 => panel-simple (boe,hv070wsa-100 panel)

the DRM core requests MEDIA_BUS_FMT_RGB888_1X7X4_SPWG format, taken from the boe_hv070wsa panel (see from drivers/gpu/drm/panel/panel-simple.c). Please note that in case of Exynos, the reversed bridge chain order is used for dsim, so this is another nasty consequence of that hack. :/

Maybe if no compatible bus format is found, the driver should force 
MEDIA_BUS_FMT_RGB888_1X24 until a proper format negotiation is 
implemented and hacks removed?


Best regards
  

On 15.11.22 09:09, Marek Szyprowski wrote:
> Hi Jagan,
> 
> On 14.11.2022 18:07, Jagan Teki wrote:
>> On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>> On 14.11.2022 11:57, Marek Szyprowski wrote:
>>>> On 10.11.2022 19:38, Jagan Teki wrote:
>>>>> Finding the right input bus format throughout the pipeline is hard
>>>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>>>> proper input format from list of supported output formats.
>>>>>
>>>>> This format can be used in pipeline for negotiating bus format between
>>>>> the DSI-end of this bridge and the other component closer to pipeline
>>>>> components.
>>>>>
>>>>> List of Pixel formats are taken from,
>>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>> 3.7.4 Pixel formats
>>>>> Table 14. DSI pixel packing formats
>>>>>
>>>>> v8:
>>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
>>>>>
>>>>> v7, v6, v5, v4:
>>>>> * none
>>>>>
>>>>> v3:
>>>>> * include media-bus-format.h
>>>>>
>>>>> v2:
>>>>> * none
>>>>>
>>>>> v1:
>>>>> * new patch
>>>>>
>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>> ---
>>>>>    drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
>>>>>    1 file changed, 53 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> @@ -15,6 +15,7 @@
>>>>>    #include <linux/clk.h>
>>>>>    #include <linux/delay.h>
>>>>>    #include <linux/irq.h>
>>>>> +#include <linux/media-bus-format.h>
>>>>>    #include <linux/of_device.h>
>>>>>    #include <linux/phy/phy.h>
>>>>>    @@ -1321,6 +1322,57 @@ static void
>>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>>        pm_runtime_put_sync(dsi->dev);
>>>>>    }
>>>>>    +/*
>>>>> + * This pixel output formats list referenced from,
>>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>> + * 3.7.4 Pixel formats
>>>>> + * Table 14. DSI pixel packing formats
>>>>> + */
>>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>>>> +    MEDIA_BUS_FMT_UYVY8_1X16,
>>>>> +    MEDIA_BUS_FMT_RGB101010_1X30,
>>>>> +    MEDIA_BUS_FMT_RGB121212_1X36,
>>>>> +    MEDIA_BUS_FMT_RGB565_1X16,
>>>>> +    MEDIA_BUS_FMT_RGB666_1X18,
>>>>> +    MEDIA_BUS_FMT_RGB888_1X24,
>>>>> +};
>>>>> +
>>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>>>> +{
>>>>> +    int i;
>>>>> +
>>>>> +    for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
>>>>> +        if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>>>> +            return true;
>>>>> +    }
>>>>> +
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +static u32 *
>>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>>> +                       struct drm_bridge_state *bridge_state,
>>>>> +                       struct drm_crtc_state *crtc_state,
>>>>> +                       struct drm_connector_state *conn_state,
>>>>> +                       u32 output_fmt,
>>>>> +                       unsigned int *num_input_fmts)
>>>>> +{
>>>>> +    u32 *input_fmts;
>>>>> +
>>>>> +    if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>>>> +        return NULL;
>>>>
>>>> Please add support for MEDIA_BUS_FMT_FIXED and maybe default to
>>>> MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
>>>>
>>>> Otherwise the above check breaks all current clients of the Samsung
>>>> DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all
>>>> DSI panels requests such format on my test systems...
>>> I've checked a bit more the bus format related code and it looks that
>>> there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_*
>>> formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for
>>> them. On Arndale board with Toshiba tc358764 bridge the
>>> MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in
>>> samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel,
>> dsim => tc358764 => panel-simple
>>
>> If I understand the bridge format negotiation correctly - If
>> atomic_get_input_bus_fmts is not implemented in tc358764 then
>> MEDIA_BUS_FMT_FIXED will be the output_fmt for dsim so we can assign
>> MEDIA_BUS_FMT_RGB888_1X24 for FIXED formats.
>>
>> from include/drm/drm_bridge.h:
>>
>>           * This method is called on all elements of the bridge chain as part of
>>           * the bus format negotiation process that happens in
>>           * drm_atomic_bridge_chain_select_bus_fmts().
>>           * This method is optional. When not implemented, the core will bypass
>>           * bus format negotiation on this element of the bridge without
>>           * failing, and the previous element in the chain will be passed
>>           * MEDIA_BUS_FMT_FIXED as its output bus format.
>>
>> As I can see tc358764 is not implemented either
>> atomic_get_input_bus_fmts or atomic API, so I think dsim gets the
>> MEDIA_BUS_FMT_FIXED bridge pipeline. I have tested sn65dsi without
>> atomic_get_input_bus_fmts I can see the dsim is getting
>> MEDIA_BUS_FMT_FIXED.
>>
>> Can you check the same from your side?
> 
> Here in case of Arndale 5250 with the following pipeline:
> 
> dsim => tc358764 => panel-simple (boe,hv070wsa-100 panel)
> 
> the DRM core requests MEDIA_BUS_FMT_RGB888_1X7X4_SPWG format, taken from the boe_hv070wsa panel (see from drivers/gpu/drm/panel/panel-simple.c). Please note that in case of Exynos, the reversed bridge chain order is used for dsim, so this is another nasty consequence of that hack. :/
> 
> Maybe if no compatible bus format is found, the driver should force 
> MEDIA_BUS_FMT_RGB888_1X24 until a proper format negotiation is 
> implemented and hacks removed?

For this specific case, wouldn't it be better to just fix the format
negotiation for tc358764 using atomic_get_input_bus_fmts()? It should
probably do the same as sn65dsi83 and request MEDIA_BUS_FMT_RGB888_1X24
from the DSI.

Forwarding the LVDS-specific format to the input is obviously the wrong
thing for the tc358764 driver to do.

But I agree, if there are other problematic pipelines with other
bridge/display drivers that don't pass a correct format, we should
accept them for now and fall back to a sane default
(MEDIA_BUS_FMT_RGB888_1X24) and fix the other drivers afterwards.

Let's not delay this series any further and better work on how to get it
merged before we miss another merge window.
  

On Tue, Nov 15, 2022 at 2:18 PM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 15.11.22 09:09, Marek Szyprowski wrote:
> > Hi Jagan,
> >
> > On 14.11.2022 18:07, Jagan Teki wrote:
> >> On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski
> >> <m.szyprowski@samsung.com> wrote:
> >>> On 14.11.2022 11:57, Marek Szyprowski wrote:
> >>>> On 10.11.2022 19:38, Jagan Teki wrote:
> >>>>> Finding the right input bus format throughout the pipeline is hard
> >>>>> so add atomic_get_input_bus_fmts callback and initialize with the
> >>>>> proper input format from list of supported output formats.
> >>>>>
> >>>>> This format can be used in pipeline for negotiating bus format between
> >>>>> the DSI-end of this bridge and the other component closer to pipeline
> >>>>> components.
> >>>>>
> >>>>> List of Pixel formats are taken from,
> >>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> >>>>> 3.7.4 Pixel formats
> >>>>> Table 14. DSI pixel packing formats
> >>>>>
> >>>>> v8:
> >>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
> >>>>>
> >>>>> v7, v6, v5, v4:
> >>>>> * none
> >>>>>
> >>>>> v3:
> >>>>> * include media-bus-format.h
> >>>>>
> >>>>> v2:
> >>>>> * none
> >>>>>
> >>>>> v1:
> >>>>> * new patch
> >>>>>
> >>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>> ---
> >>>>>    drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
> >>>>>    1 file changed, 53 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> index 0fe153b29e4f..33e5ae9c865f 100644
> >>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> @@ -15,6 +15,7 @@
> >>>>>    #include <linux/clk.h>
> >>>>>    #include <linux/delay.h>
> >>>>>    #include <linux/irq.h>
> >>>>> +#include <linux/media-bus-format.h>
> >>>>>    #include <linux/of_device.h>
> >>>>>    #include <linux/phy/phy.h>
> >>>>>    @@ -1321,6 +1322,57 @@ static void
> >>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> >>>>>        pm_runtime_put_sync(dsi->dev);
> >>>>>    }
> >>>>>    +/*
> >>>>> + * This pixel output formats list referenced from,
> >>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> >>>>> + * 3.7.4 Pixel formats
> >>>>> + * Table 14. DSI pixel packing formats
> >>>>> + */
> >>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
> >>>>> +    MEDIA_BUS_FMT_UYVY8_1X16,
> >>>>> +    MEDIA_BUS_FMT_RGB101010_1X30,
> >>>>> +    MEDIA_BUS_FMT_RGB121212_1X36,
> >>>>> +    MEDIA_BUS_FMT_RGB565_1X16,
> >>>>> +    MEDIA_BUS_FMT_RGB666_1X18,
> >>>>> +    MEDIA_BUS_FMT_RGB888_1X24,
> >>>>> +};
> >>>>> +
> >>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> >>>>> +{
> >>>>> +    int i;
> >>>>> +
> >>>>> +    for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
> >>>>> +        if (samsung_dsim_pixel_output_fmts[i] == fmt)
> >>>>> +            return true;
> >>>>> +    }
> >>>>> +
> >>>>> +    return false;
> >>>>> +}
> >>>>> +
> >>>>> +static u32 *
> >>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> >>>>> +                       struct drm_bridge_state *bridge_state,
> >>>>> +                       struct drm_crtc_state *crtc_state,
> >>>>> +                       struct drm_connector_state *conn_state,
> >>>>> +                       u32 output_fmt,
> >>>>> +                       unsigned int *num_input_fmts)
> >>>>> +{
> >>>>> +    u32 *input_fmts;
> >>>>> +
> >>>>> +    if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> >>>>> +        return NULL;
> >>>>
> >>>> Please add support for MEDIA_BUS_FMT_FIXED and maybe default to
> >>>> MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
> >>>>
> >>>> Otherwise the above check breaks all current clients of the Samsung
> >>>> DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all
> >>>> DSI panels requests such format on my test systems...
> >>> I've checked a bit more the bus format related code and it looks that
> >>> there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_*
> >>> formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for
> >>> them. On Arndale board with Toshiba tc358764 bridge the
> >>> MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in
> >>> samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel,
> >> dsim => tc358764 => panel-simple
> >>
> >> If I understand the bridge format negotiation correctly - If
> >> atomic_get_input_bus_fmts is not implemented in tc358764 then
> >> MEDIA_BUS_FMT_FIXED will be the output_fmt for dsim so we can assign
> >> MEDIA_BUS_FMT_RGB888_1X24 for FIXED formats.
> >>
> >> from include/drm/drm_bridge.h:
> >>
> >>           * This method is called on all elements of the bridge chain as part of
> >>           * the bus format negotiation process that happens in
> >>           * drm_atomic_bridge_chain_select_bus_fmts().
> >>           * This method is optional. When not implemented, the core will bypass
> >>           * bus format negotiation on this element of the bridge without
> >>           * failing, and the previous element in the chain will be passed
> >>           * MEDIA_BUS_FMT_FIXED as its output bus format.
> >>
> >> As I can see tc358764 is not implemented either
> >> atomic_get_input_bus_fmts or atomic API, so I think dsim gets the
> >> MEDIA_BUS_FMT_FIXED bridge pipeline. I have tested sn65dsi without
> >> atomic_get_input_bus_fmts I can see the dsim is getting
> >> MEDIA_BUS_FMT_FIXED.
> >>
> >> Can you check the same from your side?
> >
> > Here in case of Arndale 5250 with the following pipeline:
> >
> > dsim => tc358764 => panel-simple (boe,hv070wsa-100 panel)
> >
> > the DRM core requests MEDIA_BUS_FMT_RGB888_1X7X4_SPWG format, taken from the boe_hv070wsa panel (see from drivers/gpu/drm/panel/panel-simple.c). Please note that in case of Exynos, the reversed bridge chain order is used for dsim, so this is another nasty consequence of that hack. :/
> >
> > Maybe if no compatible bus format is found, the driver should force
> > MEDIA_BUS_FMT_RGB888_1X24 until a proper format negotiation is
> > implemented and hacks removed?
>
> For this specific case, wouldn't it be better to just fix the format
> negotiation for tc358764 using atomic_get_input_bus_fmts()? It should
> probably do the same as sn65dsi83 and request MEDIA_BUS_FMT_RGB888_1X24
> from the DSI.
>
> Forwarding the LVDS-specific format to the input is obviously the wrong
> thing for the tc358764 driver to do.
>
> But I agree, if there are other problematic pipelines with other
> bridge/display drivers that don't pass a correct format, we should
> accept them for now and fall back to a sane default
> (MEDIA_BUS_FMT_RGB888_1X24) and fix the other drivers afterwards.
>
> Let's not delay this series any further and better work on how to get it
> merged before we miss another merge window.

Agreed this point, here is the updated diff (on top of this patch) for
falling back to the default format for those.

+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1387,6 +1387,8 @@ static const u32 samsung_dsim_pixel_output_fmts[] = {
        MEDIA_BUS_FMT_RGB565_1X16,
        MEDIA_BUS_FMT_RGB666_1X18,
        MEDIA_BUS_FMT_RGB888_1X24,
+
+       MEDIA_BUS_FMT_FIXED,
 };

 static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
@@ -1412,7 +1414,13 @@ samsung_dsim_atomic_get_input_bus_fmts(struct
drm_bridge *bridge,
        u32 *input_fmts;

        if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
-               return NULL;
+               /*
+                * Some bridge/display drivers are still not able to pass
+                * the correct format, so handle those pipelines by falling
+                * back to the default format till the supported format list
+                * gets finalized.
+                */
+               output_fmt = MEDIA_BUS_FMT_RGB888_1X24;

        *num_input_fmts = 1;

@@ -1420,7 +1428,14 @@ samsung_dsim_atomic_get_input_bus_fmts(struct
drm_bridge *bridge,
        if (!input_fmts)
                return NULL;

-       input_fmts[0] = output_fmt;
+       switch (output_fmt) {
+       case MEDIA_BUS_FMT_FIXED:
+               input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+               break;
+       default:
+               input_fmts[0] = output_fmt;
+               break;
+       }

        return input_fmts;

If all is okay, I will send v9 which would probably test it for the
final version to merge.

Thanks,
Jagan.
  

On 11/14/22 08:49, Jagan Teki wrote:
> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 11/10/22 19:38, Jagan Teki wrote:
>>> Finding the right input bus format throughout the pipeline is hard
>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>> proper input format from list of supported output formats.
>>>
>>> This format can be used in pipeline for negotiating bus format between
>>> the DSI-end of this bridge and the other component closer to pipeline
>>> components.
>>>
>>> List of Pixel formats are taken from,
>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>> 3.7.4 Pixel formats
>>> Table 14. DSI pixel packing formats
>>>
>>> v8:
>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
>>>
>>> v7, v6, v5, v4:
>>> * none
>>>
>>> v3:
>>> * include media-bus-format.h
>>>
>>> v2:
>>> * none
>>>
>>> v1:
>>> * new patch
>>>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>>    drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
>>>    1 file changed, 53 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -15,6 +15,7 @@
>>>    #include <linux/clk.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/irq.h>
>>> +#include <linux/media-bus-format.h>
>>>    #include <linux/of_device.h>
>>>    #include <linux/phy/phy.h>
>>>
>>> @@ -1321,6 +1322,57 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>        pm_runtime_put_sync(dsi->dev);
>>>    }
>>>
>>> +/*
>>> + * This pixel output formats list referenced from,
>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>> + * 3.7.4 Pixel formats
>>> + * Table 14. DSI pixel packing formats
>>> + */
>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>
>> You can also add :
>>
>> MEDIA_BUS_FMT_YUYV10_1X20
>>
>> MEDIA_BUS_FMT_YUYV12_1X24
> 
> Are these for the below formats?
> 
> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2
>   Packed Pixel Stream, 24-bit YCbCr, 4:2:2"
>>
>>> +     MEDIA_BUS_FMT_UYVY8_1X16,
>>> +     MEDIA_BUS_FMT_RGB101010_1X30,
>>> +     MEDIA_BUS_FMT_RGB121212_1X36,
>>> +     MEDIA_BUS_FMT_RGB565_1X16,
>>> +     MEDIA_BUS_FMT_RGB666_1X18,
>>> +     MEDIA_BUS_FMT_RGB888_1X24,
>>> +};
>>> +
>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
>>> +             if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>> +                     return true;
>>> +     }
>>> +
>>> +     return false;
>>> +}
>>> +
>>> +static u32 *
>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>> +                                    struct drm_bridge_state *bridge_state,
>>> +                                    struct drm_crtc_state *crtc_state,
>>> +                                    struct drm_connector_state *conn_state,
>>> +                                    u32 output_fmt,
>>> +                                    unsigned int *num_input_fmts)
>>> +{
>>> +     u32 *input_fmts;
>>> +
>>> +     if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>> +             return NULL;
>>> +
>>> +     *num_input_fmts = 1;
>>
>> Shouldn't this be 6 ?
>>
>>> +     input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>>> +     if (!input_fmts)
>>> +             return NULL;
>>> +
>>> +     input_fmts[0] = output_fmt;
>>
>> Shouldn't this be a list of all 6 supported pixel formats ?
> 
> Negotiation would settle to return one input_fmt from the list of
> supporting output_fmts. so the num_input_fmts would be 1 rather than
> the number of fmts in the supporting list. This is what I understood
> from the atomic_get_input_bus_fmts API. let me know if I miss
> something here.

How does the negotiation work for this kind of pipeline:

LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector

where all elements (LCDIFv3, DSIM, HDMI bridge) can support either 
RGB888 or packed YUV422 ?

Who decides the format used by such pipeline ?

Why should it be the DSIM bridge and not e.g. the HDMI bridge or the 
LCDIFv3 ?
  

Hi Jagan,

On 15.11.2022 10:20, Jagan Teki wrote:
> On Tue, Nov 15, 2022 at 2:18 PM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>> On 15.11.22 09:09, Marek Szyprowski wrote:
>>> On 14.11.2022 18:07, Jagan Teki wrote:
>>>> On Mon, Nov 14, 2022 at 8:10 PM Marek Szyprowski
>>>> <m.szyprowski@samsung.com> wrote:
>>>>> On 14.11.2022 11:57, Marek Szyprowski wrote:
>>>>>> On 10.11.2022 19:38, Jagan Teki wrote:
>>>>>>> Finding the right input bus format throughout the pipeline is hard
>>>>>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>>>>>> proper input format from list of supported output formats.
>>>>>>>
>>>>>>> This format can be used in pipeline for negotiating bus format between
>>>>>>> the DSI-end of this bridge and the other component closer to pipeline
>>>>>>> components.
>>>>>>>
>>>>>>> List of Pixel formats are taken from,
>>>>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>>>> 3.7.4 Pixel formats
>>>>>>> Table 14. DSI pixel packing formats
>>>>>>>
>>>>>>> v8:
>>>>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2
>>>>>>>
>>>>>>> v7, v6, v5, v4:
>>>>>>> * none
>>>>>>>
>>>>>>> v3:
>>>>>>> * include media-bus-format.h
>>>>>>>
>>>>>>> v2:
>>>>>>> * none
>>>>>>>
>>>>>>> v1:
>>>>>>> * new patch
>>>>>>>
>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/bridge/samsung-dsim.c | 53 +++++++++++++++++++++++++++
>>>>>>>     1 file changed, 53 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>     #include <linux/clk.h>
>>>>>>>     #include <linux/delay.h>
>>>>>>>     #include <linux/irq.h>
>>>>>>> +#include <linux/media-bus-format.h>
>>>>>>>     #include <linux/of_device.h>
>>>>>>>     #include <linux/phy/phy.h>
>>>>>>>     @@ -1321,6 +1322,57 @@ static void
>>>>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>>>>         pm_runtime_put_sync(dsi->dev);
>>>>>>>     }
>>>>>>>     +/*
>>>>>>> + * This pixel output formats list referenced from,
>>>>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>>>> + * 3.7.4 Pixel formats
>>>>>>> + * Table 14. DSI pixel packing formats
>>>>>>> + */
>>>>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>>>>>> +    MEDIA_BUS_FMT_UYVY8_1X16,
>>>>>>> +    MEDIA_BUS_FMT_RGB101010_1X30,
>>>>>>> +    MEDIA_BUS_FMT_RGB121212_1X36,
>>>>>>> +    MEDIA_BUS_FMT_RGB565_1X16,
>>>>>>> +    MEDIA_BUS_FMT_RGB666_1X18,
>>>>>>> +    MEDIA_BUS_FMT_RGB888_1X24,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>>>>>> +{
>>>>>>> +    int i;
>>>>>>> +
>>>>>>> +    for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); i++) {
>>>>>>> +        if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>>>>>> +            return true;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return false;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static u32 *
>>>>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>>>>> +                       struct drm_bridge_state *bridge_state,
>>>>>>> +                       struct drm_crtc_state *crtc_state,
>>>>>>> +                       struct drm_connector_state *conn_state,
>>>>>>> +                       u32 output_fmt,
>>>>>>> +                       unsigned int *num_input_fmts)
>>>>>>> +{
>>>>>>> +    u32 *input_fmts;
>>>>>>> +
>>>>>>> +    if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>>>>>> +        return NULL;
>>>>>> Please add support for MEDIA_BUS_FMT_FIXED and maybe default to
>>>>>> MEDIA_BUS_FMT_RGB888_1X24 if requested format is not matched.
>>>>>>
>>>>>> Otherwise the above check breaks all current clients of the Samsung
>>>>>> DSIM/Exynos DSI. I didn't dig into the bus matching code yet, but all
>>>>>> DSI panels requests such format on my test systems...
>>>>> I've checked a bit more the bus format related code and it looks that
>>>>> there are more issues. The DSI panels don't use the MEDIA_BUS_FMT_*
>>>>> formats thus the bridge negotiation code selects MEDIA_BUS_FMT_FIXED for
>>>>> them. On Arndale board with Toshiba tc358764 bridge the
>>>>> MEDIA_BUS_FMT_RGB888_1X7X4_SPWG output_fmt is requested in
>>>>> samsung_dsim_atomic_get_input_bus_fmts() (forwarded from the LVDS panel,
>>>> dsim => tc358764 => panel-simple
>>>>
>>>> If I understand the bridge format negotiation correctly - If
>>>> atomic_get_input_bus_fmts is not implemented in tc358764 then
>>>> MEDIA_BUS_FMT_FIXED will be the output_fmt for dsim so we can assign
>>>> MEDIA_BUS_FMT_RGB888_1X24 for FIXED formats.
>>>>
>>>> from include/drm/drm_bridge.h:
>>>>
>>>>            * This method is called on all elements of the bridge chain as part of
>>>>            * the bus format negotiation process that happens in
>>>>            * drm_atomic_bridge_chain_select_bus_fmts().
>>>>            * This method is optional. When not implemented, the core will bypass
>>>>            * bus format negotiation on this element of the bridge without
>>>>            * failing, and the previous element in the chain will be passed
>>>>            * MEDIA_BUS_FMT_FIXED as its output bus format.
>>>>
>>>> As I can see tc358764 is not implemented either
>>>> atomic_get_input_bus_fmts or atomic API, so I think dsim gets the
>>>> MEDIA_BUS_FMT_FIXED bridge pipeline. I have tested sn65dsi without
>>>> atomic_get_input_bus_fmts I can see the dsim is getting
>>>> MEDIA_BUS_FMT_FIXED.
>>>>
>>>> Can you check the same from your side?
>>> Here in case of Arndale 5250 with the following pipeline:
>>>
>>> dsim => tc358764 => panel-simple (boe,hv070wsa-100 panel)
>>>
>>> the DRM core requests MEDIA_BUS_FMT_RGB888_1X7X4_SPWG format, taken from the boe_hv070wsa panel (see from drivers/gpu/drm/panel/panel-simple.c). Please note that in case of Exynos, the reversed bridge chain order is used for dsim, so this is another nasty consequence of that hack. :/
>>>
>>> Maybe if no compatible bus format is found, the driver should force
>>> MEDIA_BUS_FMT_RGB888_1X24 until a proper format negotiation is
>>> implemented and hacks removed?
>> For this specific case, wouldn't it be better to just fix the format
>> negotiation for tc358764 using atomic_get_input_bus_fmts()? It should
>> probably do the same as sn65dsi83 and request MEDIA_BUS_FMT_RGB888_1X24
>> from the DSI.
>>
>> Forwarding the LVDS-specific format to the input is obviously the wrong
>> thing for the tc358764 driver to do.
>>
>> But I agree, if there are other problematic pipelines with other
>> bridge/display drivers that don't pass a correct format, we should
>> accept them for now and fall back to a sane default
>> (MEDIA_BUS_FMT_RGB888_1X24) and fix the other drivers afterwards.
>>
>> Let's not delay this series any further and better work on how to get it
>> merged before we miss another merge window.
> Agreed this point, here is the updated diff (on top of this patch) for
> falling back to the default format for those.
>
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1387,6 +1387,8 @@ static const u32 samsung_dsim_pixel_output_fmts[] = {
>          MEDIA_BUS_FMT_RGB565_1X16,
>          MEDIA_BUS_FMT_RGB666_1X18,
>          MEDIA_BUS_FMT_RGB888_1X24,
> +
> +       MEDIA_BUS_FMT_FIXED,
>   };
>
>   static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> @@ -1412,7 +1414,13 @@ samsung_dsim_atomic_get_input_bus_fmts(struct
> drm_bridge *bridge,
>          u32 *input_fmts;
>
>          if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> -               return NULL;
> +               /*
> +                * Some bridge/display drivers are still not able to pass
> +                * the correct format, so handle those pipelines by falling
> +                * back to the default format till the supported format list
> +                * gets finalized.
> +                */
> +               output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
>
>          *num_input_fmts = 1;
>
> @@ -1420,7 +1428,14 @@ samsung_dsim_atomic_get_input_bus_fmts(struct
> drm_bridge *bridge,
>          if (!input_fmts)
>                  return NULL;
>
> -       input_fmts[0] = output_fmt;
> +       switch (output_fmt) {
> +       case MEDIA_BUS_FMT_FIXED:
> +               input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +               break;
> +       default:
> +               input_fmts[0] = output_fmt;
> +               break;
> +       }
>
>          return input_fmts;
>
> If all is okay, I will send v9 which would probably test it for the
> final version to merge.

This works for me (I've manually applied the above changes, due to 
malformed diff).

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Best regards
  

On 15.11.2022 13:00, Marek Vasut wrote:
> On 11/14/22 08:49, Jagan Teki wrote:
>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 11/10/22 19:38, Jagan Teki wrote:
>>>> Finding the right input bus format throughout the pipeline is hard
>>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>>> proper input format from list of supported output formats.
>>>>
>>>> This format can be used in pipeline for negotiating bus format between
>>>> the DSI-end of this bridge and the other component closer to pipeline
>>>> components.
>>>>
>>>> List of Pixel formats are taken from,
>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>> 3.7.4 Pixel formats
>>>> Table 14. DSI pixel packing formats
>>>>
>>>> v8:
>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI 
>>>> DSI/CSI-2
>>>>
>>>> v7, v6, v5, v4:
>>>> * none
>>>>
>>>> v3:
>>>> * include media-bus-format.h
>>>>
>>>> v2:
>>>> * none
>>>>
>>>> v1:
>>>> * new patch
>>>>
>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>> ---
>>>>    drivers/gpu/drm/bridge/samsung-dsim.c | 53 
>>>> +++++++++++++++++++++++++++
>>>>    1 file changed, 53 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/clk.h>
>>>>    #include <linux/delay.h>
>>>>    #include <linux/irq.h>
>>>> +#include <linux/media-bus-format.h>
>>>>    #include <linux/of_device.h>
>>>>    #include <linux/phy/phy.h>
>>>>
>>>> @@ -1321,6 +1322,57 @@ static void 
>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>        pm_runtime_put_sync(dsi->dev);
>>>>    }
>>>>
>>>> +/*
>>>> + * This pixel output formats list referenced from,
>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>> + * 3.7.4 Pixel formats
>>>> + * Table 14. DSI pixel packing formats
>>>> + */
>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>>
>>> You can also add :
>>>
>>> MEDIA_BUS_FMT_YUYV10_1X20
>>>
>>> MEDIA_BUS_FMT_YUYV12_1X24
>>
>> Are these for the below formats?
>>
>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2
>>   Packed Pixel Stream, 24-bit YCbCr, 4:2:2"
>>>
>>>> +     MEDIA_BUS_FMT_UYVY8_1X16,
>>>> +     MEDIA_BUS_FMT_RGB101010_1X30,
>>>> +     MEDIA_BUS_FMT_RGB121212_1X36,
>>>> +     MEDIA_BUS_FMT_RGB565_1X16,
>>>> +     MEDIA_BUS_FMT_RGB666_1X18,
>>>> +     MEDIA_BUS_FMT_RGB888_1X24,
>>>> +};
>>>> +
>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>>> +{
>>>> +     int i;
>>>> +
>>>> +     for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts); 
>>>> i++) {
>>>> +             if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>>> +                     return true;
>>>> +     }
>>>> +
>>>> +     return false;
>>>> +}
>>>> +
>>>> +static u32 *
>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>> +                                    struct drm_bridge_state 
>>>> *bridge_state,
>>>> +                                    struct drm_crtc_state 
>>>> *crtc_state,
>>>> +                                    struct drm_connector_state 
>>>> *conn_state,
>>>> +                                    u32 output_fmt,
>>>> +                                    unsigned int *num_input_fmts)
>>>> +{
>>>> +     u32 *input_fmts;
>>>> +
>>>> +     if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>>> +             return NULL;
>>>> +
>>>> +     *num_input_fmts = 1;
>>>
>>> Shouldn't this be 6 ?
>>>
>>>> +     input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>>>> +     if (!input_fmts)
>>>> +             return NULL;
>>>> +
>>>> +     input_fmts[0] = output_fmt;
>>>
>>> Shouldn't this be a list of all 6 supported pixel formats ?
>>
>> Negotiation would settle to return one input_fmt from the list of
>> supporting output_fmts. so the num_input_fmts would be 1 rather than
>> the number of fmts in the supporting list. This is what I understood
>> from the atomic_get_input_bus_fmts API. let me know if I miss
>> something here.
>
> How does the negotiation work for this kind of pipeline:
>
> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector
>
> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either 
> RGB888 or packed YUV422 ?
>
> Who decides the format used by such pipeline ?
>
> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the 
> LCDIFv3 ?


If I got it right, the drivers reports their preference for the returned 
formats. The format that is first in the reported array is the most 
preferred one. This probably means that in your case the HDMI bridge 
decides by reporting RGB or YUV first (if all elements supports both).


Best regards
  

On 11/16/22 09:07, Marek Szyprowski wrote:
> On 15.11.2022 13:00, Marek Vasut wrote:
>> On 11/14/22 08:49, Jagan Teki wrote:
>>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 11/10/22 19:38, Jagan Teki wrote:
>>>>> Finding the right input bus format throughout the pipeline is hard
>>>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>>>> proper input format from list of supported output formats.
>>>>>
>>>>> This format can be used in pipeline for negotiating bus format between
>>>>> the DSI-end of this bridge and the other component closer to pipeline
>>>>> components.
>>>>>
>>>>> List of Pixel formats are taken from,
>>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>> 3.7.4 Pixel formats
>>>>> Table 14. DSI pixel packing formats
>>>>>
>>>>> v8:
>>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI
>>>>> DSI/CSI-2
>>>>>
>>>>> v7, v6, v5, v4:
>>>>> * none
>>>>>
>>>>> v3:
>>>>> * include media-bus-format.h
>>>>>
>>>>> v2:
>>>>> * none
>>>>>
>>>>> v1:
>>>>> * new patch
>>>>>
>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>> ---
>>>>>     drivers/gpu/drm/bridge/samsung-dsim.c | 53
>>>>> +++++++++++++++++++++++++++
>>>>>     1 file changed, 53 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> @@ -15,6 +15,7 @@
>>>>>     #include <linux/clk.h>
>>>>>     #include <linux/delay.h>
>>>>>     #include <linux/irq.h>
>>>>> +#include <linux/media-bus-format.h>
>>>>>     #include <linux/of_device.h>
>>>>>     #include <linux/phy/phy.h>
>>>>>
>>>>> @@ -1321,6 +1322,57 @@ static void
>>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>>         pm_runtime_put_sync(dsi->dev);
>>>>>     }
>>>>>
>>>>> +/*
>>>>> + * This pixel output formats list referenced from,
>>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>> + * 3.7.4 Pixel formats
>>>>> + * Table 14. DSI pixel packing formats
>>>>> + */
>>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>>>
>>>> You can also add :
>>>>
>>>> MEDIA_BUS_FMT_YUYV10_1X20
>>>>
>>>> MEDIA_BUS_FMT_YUYV12_1X24
>>>
>>> Are these for the below formats?
>>>
>>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2
>>>    Packed Pixel Stream, 24-bit YCbCr, 4:2:2"
>>>>
>>>>> +     MEDIA_BUS_FMT_UYVY8_1X16,
>>>>> +     MEDIA_BUS_FMT_RGB101010_1X30,
>>>>> +     MEDIA_BUS_FMT_RGB121212_1X36,
>>>>> +     MEDIA_BUS_FMT_RGB565_1X16,
>>>>> +     MEDIA_BUS_FMT_RGB666_1X18,
>>>>> +     MEDIA_BUS_FMT_RGB888_1X24,
>>>>> +};
>>>>> +
>>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>>>> +{
>>>>> +     int i;
>>>>> +
>>>>> +     for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts);
>>>>> i++) {
>>>>> +             if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>>>> +                     return true;
>>>>> +     }
>>>>> +
>>>>> +     return false;
>>>>> +}
>>>>> +
>>>>> +static u32 *
>>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>>> +                                    struct drm_bridge_state
>>>>> *bridge_state,
>>>>> +                                    struct drm_crtc_state
>>>>> *crtc_state,
>>>>> +                                    struct drm_connector_state
>>>>> *conn_state,
>>>>> +                                    u32 output_fmt,
>>>>> +                                    unsigned int *num_input_fmts)
>>>>> +{
>>>>> +     u32 *input_fmts;
>>>>> +
>>>>> +     if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>>>> +             return NULL;
>>>>> +
>>>>> +     *num_input_fmts = 1;
>>>>
>>>> Shouldn't this be 6 ?
>>>>
>>>>> +     input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>>>>> +     if (!input_fmts)
>>>>> +             return NULL;
>>>>> +
>>>>> +     input_fmts[0] = output_fmt;
>>>>
>>>> Shouldn't this be a list of all 6 supported pixel formats ?
>>>
>>> Negotiation would settle to return one input_fmt from the list of
>>> supporting output_fmts. so the num_input_fmts would be 1 rather than
>>> the number of fmts in the supporting list. This is what I understood
>>> from the atomic_get_input_bus_fmts API. let me know if I miss
>>> something here.
>>
>> How does the negotiation work for this kind of pipeline:
>>
>> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector
>>
>> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either
>> RGB888 or packed YUV422 ?
>>
>> Who decides the format used by such pipeline ?
>>
>> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the
>> LCDIFv3 ?
> 
> 
> If I got it right, the drivers reports their preference for the returned
> formats. The format that is first in the reported array is the most
> preferred one. This probably means that in your case the HDMI bridge
> decides by reporting RGB or YUV first (if all elements supports both).

But in that case, we need to list input_fmts[0]...input_fmts[n-1] in 
samsung_dsim_atomic_get_input_bus_fmts() and also set *num_input_fmts = 
n, correct ?
  

On 16.11.2022 11:49, Marek Vasut wrote:
> On 11/16/22 09:07, Marek Szyprowski wrote:
>> On 15.11.2022 13:00, Marek Vasut wrote:
>>> On 11/14/22 08:49, Jagan Teki wrote:
>>>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 11/10/22 19:38, Jagan Teki wrote:
>>>>>> Finding the right input bus format throughout the pipeline is hard
>>>>>> so add atomic_get_input_bus_fmts callback and initialize with the
>>>>>> proper input format from list of supported output formats.
>>>>>>
>>>>>> This format can be used in pipeline for negotiating bus format 
>>>>>> between
>>>>>> the DSI-end of this bridge and the other component closer to 
>>>>>> pipeline
>>>>>> components.
>>>>>>
>>>>>> List of Pixel formats are taken from,
>>>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>>> 3.7.4 Pixel formats
>>>>>> Table 14. DSI pixel packing formats
>>>>>>
>>>>>> v8:
>>>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI
>>>>>> DSI/CSI-2
>>>>>>
>>>>>> v7, v6, v5, v4:
>>>>>> * none
>>>>>>
>>>>>> v3:
>>>>>> * include media-bus-format.h
>>>>>>
>>>>>> v2:
>>>>>> * none
>>>>>>
>>>>>> v1:
>>>>>> * new patch
>>>>>>
>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/bridge/samsung-dsim.c | 53
>>>>>> +++++++++++++++++++++++++++
>>>>>>     1 file changed, 53 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> index 0fe153b29e4f..33e5ae9c865f 100644
>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>     #include <linux/clk.h>
>>>>>>     #include <linux/delay.h>
>>>>>>     #include <linux/irq.h>
>>>>>> +#include <linux/media-bus-format.h>
>>>>>>     #include <linux/of_device.h>
>>>>>>     #include <linux/phy/phy.h>
>>>>>>
>>>>>> @@ -1321,6 +1322,57 @@ static void
>>>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>>>         pm_runtime_put_sync(dsi->dev);
>>>>>>     }
>>>>>>
>>>>>> +/*
>>>>>> + * This pixel output formats list referenced from,
>>>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
>>>>>> + * 3.7.4 Pixel formats
>>>>>> + * Table 14. DSI pixel packing formats
>>>>>> + */
>>>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
>>>>>
>>>>> You can also add :
>>>>>
>>>>> MEDIA_BUS_FMT_YUYV10_1X20
>>>>>
>>>>> MEDIA_BUS_FMT_YUYV12_1X24
>>>>
>>>> Are these for the below formats?
>>>>
>>>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2
>>>>    Packed Pixel Stream, 24-bit YCbCr, 4:2:2"
>>>>>
>>>>>> +     MEDIA_BUS_FMT_UYVY8_1X16,
>>>>>> +     MEDIA_BUS_FMT_RGB101010_1X30,
>>>>>> +     MEDIA_BUS_FMT_RGB121212_1X36,
>>>>>> +     MEDIA_BUS_FMT_RGB565_1X16,
>>>>>> +     MEDIA_BUS_FMT_RGB666_1X18,
>>>>>> +     MEDIA_BUS_FMT_RGB888_1X24,
>>>>>> +};
>>>>>> +
>>>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
>>>>>> +{
>>>>>> +     int i;
>>>>>> +
>>>>>> +     for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts);
>>>>>> i++) {
>>>>>> +             if (samsung_dsim_pixel_output_fmts[i] == fmt)
>>>>>> +                     return true;
>>>>>> +     }
>>>>>> +
>>>>>> +     return false;
>>>>>> +}
>>>>>> +
>>>>>> +static u32 *
>>>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>>>> +                                    struct drm_bridge_state
>>>>>> *bridge_state,
>>>>>> +                                    struct drm_crtc_state
>>>>>> *crtc_state,
>>>>>> +                                    struct drm_connector_state
>>>>>> *conn_state,
>>>>>> +                                    u32 output_fmt,
>>>>>> +                                    unsigned int *num_input_fmts)
>>>>>> +{
>>>>>> +     u32 *input_fmts;
>>>>>> +
>>>>>> +     if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
>>>>>> +             return NULL;
>>>>>> +
>>>>>> +     *num_input_fmts = 1;
>>>>>
>>>>> Shouldn't this be 6 ?
>>>>>
>>>>>> +     input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
>>>>>> +     if (!input_fmts)
>>>>>> +             return NULL;
>>>>>> +
>>>>>> +     input_fmts[0] = output_fmt;
>>>>>
>>>>> Shouldn't this be a list of all 6 supported pixel formats ?
>>>>
>>>> Negotiation would settle to return one input_fmt from the list of
>>>> supporting output_fmts. so the num_input_fmts would be 1 rather than
>>>> the number of fmts in the supporting list. This is what I understood
>>>> from the atomic_get_input_bus_fmts API. let me know if I miss
>>>> something here.
>>>
>>> How does the negotiation work for this kind of pipeline:
>>>
>>> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector
>>>
>>> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either
>>> RGB888 or packed YUV422 ?
>>>
>>> Who decides the format used by such pipeline ?
>>>
>>> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the
>>> LCDIFv3 ?
>>
>>
>> If I got it right, the drivers reports their preference for the returned
>> formats. The format that is first in the reported array is the most
>> preferred one. This probably means that in your case the HDMI bridge
>> decides by reporting RGB or YUV first (if all elements supports both).
>
> But in that case, we need to list input_fmts[0]...input_fmts[n-1] in 
> samsung_dsim_atomic_get_input_bus_fmts() and also set *num_input_fmts 
> = n, correct ?
>
Yes, if I got the bus format negotiation code right, but I didn't check 
the details.

Best regards
  

On Wed, Nov 16, 2022 at 4:37 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 16.11.2022 11:49, Marek Vasut wrote:
> > On 11/16/22 09:07, Marek Szyprowski wrote:
> >> On 15.11.2022 13:00, Marek Vasut wrote:
> >>> On 11/14/22 08:49, Jagan Teki wrote:
> >>>> On Sun, Nov 13, 2022 at 5:51 AM Marek Vasut <marex@denx.de> wrote:
> >>>>>
> >>>>> On 11/10/22 19:38, Jagan Teki wrote:
> >>>>>> Finding the right input bus format throughout the pipeline is hard
> >>>>>> so add atomic_get_input_bus_fmts callback and initialize with the
> >>>>>> proper input format from list of supported output formats.
> >>>>>>
> >>>>>> This format can be used in pipeline for negotiating bus format
> >>>>>> between
> >>>>>> the DSI-end of this bridge and the other component closer to
> >>>>>> pipeline
> >>>>>> components.
> >>>>>>
> >>>>>> List of Pixel formats are taken from,
> >>>>>> AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> >>>>>> 3.7.4 Pixel formats
> >>>>>> Table 14. DSI pixel packing formats
> >>>>>>
> >>>>>> v8:
> >>>>>> * added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI
> >>>>>> DSI/CSI-2
> >>>>>>
> >>>>>> v7, v6, v5, v4:
> >>>>>> * none
> >>>>>>
> >>>>>> v3:
> >>>>>> * include media-bus-format.h
> >>>>>>
> >>>>>> v2:
> >>>>>> * none
> >>>>>>
> >>>>>> v1:
> >>>>>> * new patch
> >>>>>>
> >>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/bridge/samsung-dsim.c | 53
> >>>>>> +++++++++++++++++++++++++++
> >>>>>>     1 file changed, 53 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>> index 0fe153b29e4f..33e5ae9c865f 100644
> >>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>> @@ -15,6 +15,7 @@
> >>>>>>     #include <linux/clk.h>
> >>>>>>     #include <linux/delay.h>
> >>>>>>     #include <linux/irq.h>
> >>>>>> +#include <linux/media-bus-format.h>
> >>>>>>     #include <linux/of_device.h>
> >>>>>>     #include <linux/phy/phy.h>
> >>>>>>
> >>>>>> @@ -1321,6 +1322,57 @@ static void
> >>>>>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> >>>>>>         pm_runtime_put_sync(dsi->dev);
> >>>>>>     }
> >>>>>>
> >>>>>> +/*
> >>>>>> + * This pixel output formats list referenced from,
> >>>>>> + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022
> >>>>>> + * 3.7.4 Pixel formats
> >>>>>> + * Table 14. DSI pixel packing formats
> >>>>>> + */
> >>>>>> +static const u32 samsung_dsim_pixel_output_fmts[] = {
> >>>>>
> >>>>> You can also add :
> >>>>>
> >>>>> MEDIA_BUS_FMT_YUYV10_1X20
> >>>>>
> >>>>> MEDIA_BUS_FMT_YUYV12_1X24
> >>>>
> >>>> Are these for the below formats?
> >>>>
> >>>> "Loosely Packed Pixel Stream, 20-bit YCbCr, 4:2:2
> >>>>    Packed Pixel Stream, 24-bit YCbCr, 4:2:2"
> >>>>>
> >>>>>> +     MEDIA_BUS_FMT_UYVY8_1X16,
> >>>>>> +     MEDIA_BUS_FMT_RGB101010_1X30,
> >>>>>> +     MEDIA_BUS_FMT_RGB121212_1X36,
> >>>>>> +     MEDIA_BUS_FMT_RGB565_1X16,
> >>>>>> +     MEDIA_BUS_FMT_RGB666_1X18,
> >>>>>> +     MEDIA_BUS_FMT_RGB888_1X24,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static bool samsung_dsim_pixel_output_fmt_supported(u32 fmt)
> >>>>>> +{
> >>>>>> +     int i;
> >>>>>> +
> >>>>>> +     for (i = 0; i < ARRAY_SIZE(samsung_dsim_pixel_output_fmts);
> >>>>>> i++) {
> >>>>>> +             if (samsung_dsim_pixel_output_fmts[i] == fmt)
> >>>>>> +                     return true;
> >>>>>> +     }
> >>>>>> +
> >>>>>> +     return false;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static u32 *
> >>>>>> +samsung_dsim_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> >>>>>> +                                    struct drm_bridge_state
> >>>>>> *bridge_state,
> >>>>>> +                                    struct drm_crtc_state
> >>>>>> *crtc_state,
> >>>>>> +                                    struct drm_connector_state
> >>>>>> *conn_state,
> >>>>>> +                                    u32 output_fmt,
> >>>>>> +                                    unsigned int *num_input_fmts)
> >>>>>> +{
> >>>>>> +     u32 *input_fmts;
> >>>>>> +
> >>>>>> +     if (!samsung_dsim_pixel_output_fmt_supported(output_fmt))
> >>>>>> +             return NULL;
> >>>>>> +
> >>>>>> +     *num_input_fmts = 1;
> >>>>>
> >>>>> Shouldn't this be 6 ?
> >>>>>
> >>>>>> +     input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> >>>>>> +     if (!input_fmts)
> >>>>>> +             return NULL;
> >>>>>> +
> >>>>>> +     input_fmts[0] = output_fmt;
> >>>>>
> >>>>> Shouldn't this be a list of all 6 supported pixel formats ?
> >>>>
> >>>> Negotiation would settle to return one input_fmt from the list of
> >>>> supporting output_fmts. so the num_input_fmts would be 1 rather than
> >>>> the number of fmts in the supporting list. This is what I understood
> >>>> from the atomic_get_input_bus_fmts API. let me know if I miss
> >>>> something here.
> >>>
> >>> How does the negotiation work for this kind of pipeline:
> >>>
> >>> LCDIFv3<->DSIM<->HDMI bridge<->HDMI connector
> >>>
> >>> where all elements (LCDIFv3, DSIM, HDMI bridge) can support either
> >>> RGB888 or packed YUV422 ?
> >>>
> >>> Who decides the format used by such pipeline ?
> >>>
> >>> Why should it be the DSIM bridge and not e.g. the HDMI bridge or the
> >>> LCDIFv3 ?
> >>
> >>
> >> If I got it right, the drivers reports their preference for the returned
> >> formats. The format that is first in the reported array is the most
> >> preferred one. This probably means that in your case the HDMI bridge
> >> decides by reporting RGB or YUV first (if all elements supports both).
> >
> > But in that case, we need to list input_fmts[0]...input_fmts[n-1] in
> > samsung_dsim_atomic_get_input_bus_fmts() and also set *num_input_fmts
> > = n, correct ?
> >
> Yes, if I got the bus format negotiation code right, but I didn't check
> the details.

It Looks like if the number of formats returned by the array is more
than 1, then the input_fmts array needs to return by listing formats
in decreasing preference order so that the bridge core will try all
formats until it finds one that works. But the question here is how
much is the array number, how to decide? I can see dw-hdmi return a
maximum of 3 possible input formats for an output format so the array
seems to be 3 here, but the rest of drivers like imx8qxp-pixel-link do
return 1 from the list of supported - dsim doing the same here.

If we are sure dsim can support or feature 2 then order RGB888 and
YUV422 decreasing preference and return them otherwise stick with a
single return of checking supported list and return desired one.

Jagan.