2020-08-12 07:29:17

by Vinay Simha B N

Subject: [PATCH] drm/bridge/tc358775: Fixes bus formats read

- bus formats read from drm_bridge_state.output_bus_cfg.format
and .atomic_get_input_bus_fmts() instead of connector

Signed-off-by: Vinay Simha BN <[email protected]>

---
v1:
* Laurent Pinchart review comments incorporated
drm_bridge_state.output_bus_cfg.format
instead of connector
---
drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
1 file changed, 59 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
index 7da15cd..5d8714a 100644
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -271,6 +271,13 @@ struct tc_data {
struct gpio_desc *stby_gpio;
u8 lvds_link; /* single-link or dual-link */
u8 bpc;
+ u32 output_bus_fmt;
+};
+
+static const u32 tc_lvds_out_bus_fmts[] = {
+ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
+ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
+ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
};

static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
@@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
ret, addr);
}

-/* helper function to access bus_formats */
-static struct drm_connector *get_connector(struct drm_encoder *encoder)
-{
- struct drm_device *dev = encoder->dev;
- struct drm_connector *connector;
-
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
- if (connector->encoder == encoder)
- return connector;
-
- return NULL;
-}
-
static void tc_bridge_enable(struct drm_bridge *bridge)
{
struct tc_data *tc = bridge_to_tc(bridge);
@@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
u32 val = 0;
u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
struct drm_display_mode *mode;
- struct drm_connector *connector = get_connector(bridge->encoder);

mode = &bridge->encoder->crtc->state->adjusted_mode;

@@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));

dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
- connector->display_info.bus_formats[0],
+ tc->output_bus_fmt,
tc->bpc);
/*
* Default hardware register settings of tc358775 configured
* with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
*/
- if (connector->display_info.bus_formats[0] ==
- MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
+ if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
/* VESA-24 */
d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
@@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
return 0;
}

+static int tc_bridge_atomic_check(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct tc_data *tc = bridge_to_tc(bridge);
+
+ tc->output_bus_fmt = bridge_state->output_bus_cfg.format;
+
+ dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
+
+ return 0;
+}
+
+static u32 *
+tc_bridge_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 = NULL;
+ int i;
+
+ *num_input_fmts = 0;
+
+ for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
+ if (output_fmt == tc_lvds_out_bus_fmts[i]) {
+ *num_input_fmts = 1;
+ input_fmts = kcalloc(*num_input_fmts,
+ sizeof(*input_fmts),
+ GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+
+ input_fmts[0] = output_fmt;
+
+ break;
+ }
+ }
+
+ return input_fmts;
+}
+
static int tc_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
{
@@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
}

static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
+ .atomic_check = tc_bridge_atomic_check,
.attach = tc_bridge_attach,
.pre_enable = tc_bridge_pre_enable,
.enable = tc_bridge_enable,
--
2.7.4

2020-08-12 09:55:49

by Laurent Pinchart

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

Hi Vinay,

Thank you for the patch.

On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> - bus formats read from drm_bridge_state.output_bus_cfg.format
> and .atomic_get_input_bus_fmts() instead of connector
>
> Signed-off-by: Vinay Simha BN <[email protected]>
>
> ---
> v1:
> * Laurent Pinchart review comments incorporated
> drm_bridge_state.output_bus_cfg.format
> instead of connector
> ---
> drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
> 1 file changed, 59 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> index 7da15cd..5d8714a 100644
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -271,6 +271,13 @@ struct tc_data {
> struct gpio_desc *stby_gpio;
> u8 lvds_link; /* single-link or dual-link */
> u8 bpc;
> + u32 output_bus_fmt;
> +};
> +
> +static const u32 tc_lvds_out_bus_fmts[] = {
> + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> };
>
> static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> ret, addr);
> }
>
> -/* helper function to access bus_formats */
> -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> -{
> - struct drm_device *dev = encoder->dev;
> - struct drm_connector *connector;
> -
> - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> - if (connector->encoder == encoder)
> - return connector;
> -
> - return NULL;
> -}
> -
> static void tc_bridge_enable(struct drm_bridge *bridge)
> {
> struct tc_data *tc = bridge_to_tc(bridge);
> @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> u32 val = 0;
> u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> struct drm_display_mode *mode;
> - struct drm_connector *connector = get_connector(bridge->encoder);
>
> mode = &bridge->encoder->crtc->state->adjusted_mode;
>
> @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
>
> dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> - connector->display_info.bus_formats[0],
> + tc->output_bus_fmt,
> tc->bpc);
> /*
> * Default hardware register settings of tc358775 configured
> * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
> */
> - if (connector->display_info.bus_formats[0] ==
> - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> + if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> /* VESA-24 */
> d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
> return 0;
> }
>
> +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct tc_data *tc = bridge_to_tc(bridge);
> +
> + tc->output_bus_fmt = bridge_state->output_bus_cfg.format;

.atomic_check() isn't allowed to modify the device state, neither the
hardware state nor the software state in drm_bridge or tc_data. You can
instead access the bridge state directly in tc_bridge_enable(), with

struct drm_bridge_state *state =
drm_priv_to_bridge_state(bridge->base.state);

> +
> + dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
> +
> + return 0;
> +}
> +
> +static u32 *
> +tc_bridge_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 = NULL;
> + int i;

i only takes positive values, so it can be an unsigned int.

> +
> + *num_input_fmts = 0;
> +
> + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> + *num_input_fmts = 1;
> + input_fmts = kcalloc(*num_input_fmts,
> + sizeof(*input_fmts),
> + GFP_KERNEL);
> + if (!input_fmts)
> + return NULL;
> +
> + input_fmts[0] = output_fmt;

I don't think this is right, the input of the bridge isn't LVDS, is it ?
As far as I can tell, the hardware support transcoding any of the
supported input formats (RGB565, RGB666 or RGB888) to any of the
supported output formats. How about the following ?

static const u32 tc_lvds_in_bus_fmts[] = {
MEDIA_BUS_FMT_RGB565_1X16,
MEDIA_BUS_FMT_RGB666_1X18,
MEDIA_BUS_FMT_RBG888_1X24,
};

...

u32 *input_fmts;
unsigned int i;

*num_input_fmts = 0;

for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
if (output_fmt == tc_lvds_out_bus_fmts[i])
break;
}

if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
return NULL;

input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
GFP_KERNEL);
if (!input_fmts)
return NULL;

for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
input_fmts[i] = tc_lvds_in_bus_fmts[i];

*num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
return input_fmts;

> +
> + break;
> + }
> + }
> +
> + return input_fmts;
> +}
> +
> static int tc_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> }
>
> static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
> + .atomic_check = tc_bridge_atomic_check,
> .attach = tc_bridge_attach,
> .pre_enable = tc_bridge_pre_enable,
> .enable = tc_bridge_enable,

--
Regards,

Laurent Pinchart

2020-08-12 12:41:12

by Vinay Simha B N

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

laurent,

please see the reply inline below.

On Wed, Aug 12, 2020 at 3:24 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Vinay,
>
> Thank you for the patch.
>
> On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> > - bus formats read from drm_bridge_state.output_bus_cfg.format
> > and .atomic_get_input_bus_fmts() instead of connector
> >
> > Signed-off-by: Vinay Simha BN <[email protected]>
> >
> > ---
> > v1:
> > * Laurent Pinchart review comments incorporated
> > drm_bridge_state.output_bus_cfg.format
> > instead of connector
> > ---
> > drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
> > 1 file changed, 59 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> > index 7da15cd..5d8714a 100644
> > --- a/drivers/gpu/drm/bridge/tc358775.c
> > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > @@ -271,6 +271,13 @@ struct tc_data {
> > struct gpio_desc *stby_gpio;
> > u8 lvds_link; /* single-link or dual-link */
> > u8 bpc;
> > + u32 output_bus_fmt;
> > +};
> > +
> > +static const u32 tc_lvds_out_bus_fmts[] = {
> > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > };
> >
> > static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> > @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> > ret, addr);
> > }
> >
> > -/* helper function to access bus_formats */
> > -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> > -{
> > - struct drm_device *dev = encoder->dev;
> > - struct drm_connector *connector;
> > -
> > - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > - if (connector->encoder == encoder)
> > - return connector;
> > -
> > - return NULL;
> > -}
> > -
> > static void tc_bridge_enable(struct drm_bridge *bridge)
> > {
> > struct tc_data *tc = bridge_to_tc(bridge);
> > @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > u32 val = 0;
> > u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> > struct drm_display_mode *mode;
> > - struct drm_connector *connector = get_connector(bridge->encoder);
> >
> > mode = &bridge->encoder->crtc->state->adjusted_mode;
> >
> > @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
> >
> > dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> > - connector->display_info.bus_formats[0],
> > + tc->output_bus_fmt,
> > tc->bpc);
> > /*
> > * Default hardware register settings of tc358775 configured
> > * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
> > */
> > - if (connector->display_info.bus_formats[0] ==
> > - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > + if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > /* VESA-24 */
> > d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> > d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> > @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
> > return 0;
> > }
> >
> > +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct tc_data *tc = bridge_to_tc(bridge);
> > +
> > + tc->output_bus_fmt = bridge_state->output_bus_cfg.format;
>
> .atomic_check() isn't allowed to modify the device state, neither the
> hardware state nor the software state in drm_bridge or tc_data. You can
> instead access the bridge state directly in tc_bridge_enable(), with
>
> struct drm_bridge_state *state =
> drm_priv_to_bridge_state(bridge->base.state);
>
Currently the driver is picking up from the dts panel
(data-mapping = "vesa-24";) or jeida-24 or jeida-18.

Does state->output_bus_cfg.format get set from the data-mapping?

> > +
> > + dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
> > +
> > + return 0;
> > +}
> > +
> > +static u32 *
> > +tc_bridge_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 = NULL;
> > + int i;
>
> i only takes positive values, so it can be an unsigned int.
>
> > +
> > + *num_input_fmts = 0;
> > +
> > + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> > + *num_input_fmts = 1;
> > + input_fmts = kcalloc(*num_input_fmts,
> > + sizeof(*input_fmts),
> > + GFP_KERNEL);
> > + if (!input_fmts)
> > + return NULL;
> > +
> > + input_fmts[0] = output_fmt;
>
> I don't think this is right, the input of the bridge isn't LVDS, is it ?

Input to the bridge is DSI, format is already set

dsi->format = MIPI_DSI_FMT_RGB888;

enum mipi_dsi_pixel_format {
MIPI_DSI_FMT_RGB888,
MIPI_DSI_FMT_RGB666,
MIPI_DSI_FMT_RGB666_PACKED,
MIPI_DSI_FMT_RGB565,
};
include/drm/drm_mipi_dsi.h

Why do we require this atomic_get_input_bus_fmts?

Do i need to implement both atomic_get_input_bus_fmts and
atomic_get_output_bus_fmts?

> As far as I can tell, the hardware support transcoding any of the
> supported input formats (RGB565, RGB666 or RGB888) to any of the
> supported output formats. How about the following ?
>
> static const u32 tc_lvds_in_bus_fmts[] = {
> MEDIA_BUS_FMT_RGB565_1X16,
> MEDIA_BUS_FMT_RGB666_1X18,
> MEDIA_BUS_FMT_RBG888_1X24,
> };
>
> ...
>
> u32 *input_fmts;
> unsigned int i;
>
> *num_input_fmts = 0;
>
> for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> if (output_fmt == tc_lvds_out_bus_fmts[i])
> break;
> }
>
> if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
> return NULL;
>
> input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> GFP_KERNEL);
> if (!input_fmts)
> return NULL;
>
> for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> input_fmts[i] = tc_lvds_in_bus_fmts[i];
>
> *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> return input_fmts;
>
> > +
> > + break;
> > + }
> > + }
> > +
> > + return input_fmts;
> > +}
> > +
> > static int tc_bridge_attach(struct drm_bridge *bridge,
> > enum drm_bridge_attach_flags flags)
> > {
> > @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > }
> >
> > static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
> > + .atomic_check = tc_bridge_atomic_check,
> > .attach = tc_bridge_attach,
> > .pre_enable = tc_bridge_pre_enable,
> > .enable = tc_bridge_enable,
>
> --
> Regards,
>
> Laurent Pinchart



--
regards,
vinaysimha

2020-08-12 13:20:00

by Laurent Pinchart

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

Hi Vinay,

On Wed, Aug 12, 2020 at 06:07:52PM +0530, Vinay Simha B N wrote:
> On Wed, Aug 12, 2020 at 3:24 PM Laurent Pinchart wrote:
> > On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> > > - bus formats read from drm_bridge_state.output_bus_cfg.format
> > > and .atomic_get_input_bus_fmts() instead of connector
> > >
> > > Signed-off-by: Vinay Simha BN <[email protected]>
> > >
> > > ---
> > > v1:
> > > * Laurent Pinchart review comments incorporated
> > > drm_bridge_state.output_bus_cfg.format
> > > instead of connector
> > > ---
> > > drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
> > > 1 file changed, 59 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> > > index 7da15cd..5d8714a 100644
> > > --- a/drivers/gpu/drm/bridge/tc358775.c
> > > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > > @@ -271,6 +271,13 @@ struct tc_data {
> > > struct gpio_desc *stby_gpio;
> > > u8 lvds_link; /* single-link or dual-link */
> > > u8 bpc;
> > > + u32 output_bus_fmt;
> > > +};
> > > +
> > > +static const u32 tc_lvds_out_bus_fmts[] = {
> > > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > };
> > >
> > > static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> > > @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> > > ret, addr);
> > > }
> > >
> > > -/* helper function to access bus_formats */
> > > -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> > > -{
> > > - struct drm_device *dev = encoder->dev;
> > > - struct drm_connector *connector;
> > > -
> > > - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > - if (connector->encoder == encoder)
> > > - return connector;
> > > -
> > > - return NULL;
> > > -}
> > > -
> > > static void tc_bridge_enable(struct drm_bridge *bridge)
> > > {
> > > struct tc_data *tc = bridge_to_tc(bridge);
> > > @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > u32 val = 0;
> > > u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> > > struct drm_display_mode *mode;
> > > - struct drm_connector *connector = get_connector(bridge->encoder);
> > >
> > > mode = &bridge->encoder->crtc->state->adjusted_mode;
> > >
> > > @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
> > >
> > > dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> > > - connector->display_info.bus_formats[0],
> > > + tc->output_bus_fmt,
> > > tc->bpc);
> > > /*
> > > * Default hardware register settings of tc358775 configured
> > > * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
> > > */
> > > - if (connector->display_info.bus_formats[0] ==
> > > - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > + if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > /* VESA-24 */
> > > d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> > > d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> > > @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
> > > return 0;
> > > }
> > >
> > > +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> > > + struct drm_bridge_state *bridge_state,
> > > + struct drm_crtc_state *crtc_state,
> > > + struct drm_connector_state *conn_state)
> > > +{
> > > + struct tc_data *tc = bridge_to_tc(bridge);
> > > +
> > > + tc->output_bus_fmt = bridge_state->output_bus_cfg.format;
> >
> > .atomic_check() isn't allowed to modify the device state, neither the
> > hardware state nor the software state in drm_bridge or tc_data. You can
> > instead access the bridge state directly in tc_bridge_enable(), with
> >
> > struct drm_bridge_state *state =
> > drm_priv_to_bridge_state(bridge->base.state);
>
> Currently the driver is picking up from the dts panel
> (data-mapping = "vesa-24";) or jeida-24 or jeida-18.
>
> Does state->output_bus_cfg.format get set from the data-mapping?

It should. The drm_panel should take care of that. In
panel_simple_get_non_edid_modes(), it calls

if (panel->desc->bus_format)
drm_display_info_set_bus_formats(&connector->display_info,
&panel->desc->bus_format, 1);

to initialize the bus format in display_info. Then, the DRM bridge
helper drm_atomic_bridge_chain_select_bus_fmts() retrieves the output
format by calling .atomic_get_output_bus_fmts() if implemented by the
last bridge in the chain, or directly from the connector display_info.
The last bridge in the chain is a DRM panel bridge, and doesn't
implement .atomic_get_output_bus_fmts(), so the format from display_info
is used, and is stored in the output_bus_cfg.format field of this bridge
in select_bus_fmt_recursive().

If something doesn't work according to the plan, I can help you
debugging.

> > > +
> > > + dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static u32 *
> > > +tc_bridge_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 = NULL;
> > > + int i;
> >
> > i only takes positive values, so it can be an unsigned int.
> >
> > > +
> > > + *num_input_fmts = 0;
> > > +
> > > + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> > > + *num_input_fmts = 1;
> > > + input_fmts = kcalloc(*num_input_fmts,
> > > + sizeof(*input_fmts),
> > > + GFP_KERNEL);
> > > + if (!input_fmts)
> > > + return NULL;
> > > +
> > > + input_fmts[0] = output_fmt;
> >
> > I don't think this is right, the input of the bridge isn't LVDS, is it ?
>
> Input to the bridge is DSI, format is already set
>
> dsi->format = MIPI_DSI_FMT_RGB888;
>
> enum mipi_dsi_pixel_format {
> MIPI_DSI_FMT_RGB888,
> MIPI_DSI_FMT_RGB666,
> MIPI_DSI_FMT_RGB666_PACKED,
> MIPI_DSI_FMT_RGB565,
> };
> include/drm/drm_mipi_dsi.h
>
> Why do we require this atomic_get_input_bus_fmts?
>
> Do i need to implement both atomic_get_input_bus_fmts and
> atomic_get_output_bus_fmts?

.atomic_get_output_bus_fmts() is only need for the last bridge in the
chain, and is not mandatory when that bridge supports a single format.
As this bridge can't be last (if the output is connect to a panel, there
will be a drm_bridge wrapping the drm_panel), you don't have to
implement that operation.

.atomic_get_input_bus_fmts() is used to negotiate formats along the
pipeline. The helps the DRM bridge helpers figure out what formats are
possible, with the help of bridges that must report what input formats
are compatible with a given output format. The DRM bridge helpers will
take care of the rest.

So, for this bridge, the input and output formats are decoupled. The
bridge can output any of the three supported LVDS formats, regardless of
what format it gets at its input. You should thus verify that the output
format you receive in this function is supported (and return NULL if it
isn't), and then return the list of supported input formats. If you
don't implement .atomic_get_input_bus_fmts(), then the DRM bridge
helpers will consider that the input and output formats are the same,
and will set the output format of the previous bridge to, for example,
MEDIA_BUS_FMT_RGB666_1X7X3_SPWG. It may work if the previous bridge
doesn't care about its output format, but if it does, then it will be
puzzled, as the previous bridge outputs DSI, not LVDS.

> > As far as I can tell, the hardware support transcoding any of the
> > supported input formats (RGB565, RGB666 or RGB888) to any of the
> > supported output formats. How about the following ?
> >
> > static const u32 tc_lvds_in_bus_fmts[] = {
> > MEDIA_BUS_FMT_RGB565_1X16,
> > MEDIA_BUS_FMT_RGB666_1X18,
> > MEDIA_BUS_FMT_RBG888_1X24,
> > };
> >
> > ...
> >
> > u32 *input_fmts;
> > unsigned int i;
> >
> > *num_input_fmts = 0;
> >
> > for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > if (output_fmt == tc_lvds_out_bus_fmts[i])
> > break;
> > }
> >
> > if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
> > return NULL;
> >
> > input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> > GFP_KERNEL);
> > if (!input_fmts)
> > return NULL;
> >
> > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> >
> > *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > return input_fmts;
> >
> > > +
> > > + break;
> > > + }
> > > + }
> > > +
> > > + return input_fmts;
> > > +}
> > > +
> > > static int tc_bridge_attach(struct drm_bridge *bridge,
> > > enum drm_bridge_attach_flags flags)
> > > {
> > > @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > > }
> > >
> > > static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
> > > + .atomic_check = tc_bridge_atomic_check,
> > > .attach = tc_bridge_attach,
> > > .pre_enable = tc_bridge_pre_enable,
> > > .enable = tc_bridge_enable,

--
Regards,

Laurent Pinchart

2020-08-12 14:57:11

by Vinay Simha B N

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

laurent,

Video data input format : RGB666 loosely packed 24 bits per pixel
Can we use MEDIA_BUS_FMT_RGB666_1X24_CPADHI? There was no information
wrt CPADHI or for loosely packed

static const u32 tc_lvds_in_bus_fmts[] = {
MEDIA_BUS_FMT_RGB565_1X16,
MEDIA_BUS_FMT_RGB666_1X18,
MEDIA_BUS_FMT_RGB666_1X24_CPADHI,
MEDIA_BUS_FMT_RBG888_1X24,
};

for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
input_fmts[i] = tc_lvds_in_bus_fmts[i];
>> This will have all the available input formats, but finally which video data input format chosen?
Since dsi->format = MIPI_DSI_FMT_RGB888 is used does it chooses
MEDIA_BUS_FMT_RBG888_1X24 by the drm pipeline

On Wed, Aug 12, 2020 at 6:48 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Vinay,
>
> On Wed, Aug 12, 2020 at 06:07:52PM +0530, Vinay Simha B N wrote:
> > On Wed, Aug 12, 2020 at 3:24 PM Laurent Pinchart wrote:
> > > On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> > > > - bus formats read from drm_bridge_state.output_bus_cfg.format
> > > > and .atomic_get_input_bus_fmts() instead of connector
> > > >
> > > > Signed-off-by: Vinay Simha BN <[email protected]>
> > > >
> > > > ---
> > > > v1:
> > > > * Laurent Pinchart review comments incorporated
> > > > drm_bridge_state.output_bus_cfg.format
> > > > instead of connector
> > > > ---
> > > > drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
> > > > 1 file changed, 59 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> > > > index 7da15cd..5d8714a 100644
> > > > --- a/drivers/gpu/drm/bridge/tc358775.c
> > > > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > > > @@ -271,6 +271,13 @@ struct tc_data {
> > > > struct gpio_desc *stby_gpio;
> > > > u8 lvds_link; /* single-link or dual-link */
> > > > u8 bpc;
> > > > + u32 output_bus_fmt;
> > > > +};
> > > > +
> > > > +static const u32 tc_lvds_out_bus_fmts[] = {
> > > > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > > > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > > + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > > };
> > > >
> > > > static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> > > > @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> > > > ret, addr);
> > > > }
> > > >
> > > > -/* helper function to access bus_formats */
> > > > -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> > > > -{
> > > > - struct drm_device *dev = encoder->dev;
> > > > - struct drm_connector *connector;
> > > > -
> > > > - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > > - if (connector->encoder == encoder)
> > > > - return connector;
> > > > -
> > > > - return NULL;
> > > > -}
> > > > -
> > > > static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > {
> > > > struct tc_data *tc = bridge_to_tc(bridge);
> > > > @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > u32 val = 0;
> > > > u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> > > > struct drm_display_mode *mode;
> > > > - struct drm_connector *connector = get_connector(bridge->encoder);
> > > >
> > > > mode = &bridge->encoder->crtc->state->adjusted_mode;
> > > >
> > > > @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
> > > >
> > > > dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> > > > - connector->display_info.bus_formats[0],
> > > > + tc->output_bus_fmt,
> > > > tc->bpc);
> > > > /*
> > > > * Default hardware register settings of tc358775 configured
> > > > * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
> > > > */
> > > > - if (connector->display_info.bus_formats[0] ==
> > > > - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > + if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > /* VESA-24 */
> > > > d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> > > > d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> > > > @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
> > > > return 0;
> > > > }
> > > >
> > > > +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> > > > + struct drm_bridge_state *bridge_state,
> > > > + struct drm_crtc_state *crtc_state,
> > > > + struct drm_connector_state *conn_state)
> > > > +{
> > > > + struct tc_data *tc = bridge_to_tc(bridge);
> > > > +
> > > > + tc->output_bus_fmt = bridge_state->output_bus_cfg.format;
> > >
> > > .atomic_check() isn't allowed to modify the device state, neither the
> > > hardware state nor the software state in drm_bridge or tc_data. You can
> > > instead access the bridge state directly in tc_bridge_enable(), with
> > >
> > > struct drm_bridge_state *state =
> > > drm_priv_to_bridge_state(bridge->base.state);
> >
> > Currently the driver is picking up from the dts panel
> > (data-mapping = "vesa-24";) or jeida-24 or jeida-18.
> >
> > Does state->output_bus_cfg.format get set from the data-mapping?
>
> It should. The drm_panel should take care of that. In
> panel_simple_get_non_edid_modes(), it calls
>
> if (panel->desc->bus_format)
> drm_display_info_set_bus_formats(&connector->display_info,
> &panel->desc->bus_format, 1);
>
> to initialize the bus format in display_info. Then, the DRM bridge
> helper drm_atomic_bridge_chain_select_bus_fmts() retrieves the output
> format by calling .atomic_get_output_bus_fmts() if implemented by the
> last bridge in the chain, or directly from the connector display_info.
> The last bridge in the chain is a DRM panel bridge, and doesn't
> implement .atomic_get_output_bus_fmts(), so the format from display_info
> is used, and is stored in the output_bus_cfg.format field of this bridge
> in select_bus_fmt_recursive().
>
> If something doesn't work according to the plan, I can help you
> debugging.
>
> > > > +
> > > > + dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static u32 *
> > > > +tc_bridge_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 = NULL;
> > > > + int i;
> > >
> > > i only takes positive values, so it can be an unsigned int.
> > >
> > > > +
> > > > + *num_input_fmts = 0;
> > > > +
> > > > + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > > + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> > > > + *num_input_fmts = 1;
> > > > + input_fmts = kcalloc(*num_input_fmts,
> > > > + sizeof(*input_fmts),
> > > > + GFP_KERNEL);
> > > > + if (!input_fmts)
> > > > + return NULL;
> > > > +
> > > > + input_fmts[0] = output_fmt;
> > >
> > > I don't think this is right, the input of the bridge isn't LVDS, is it ?
> >
> > Input to the bridge is DSI, format is already set
> >
> > dsi->format = MIPI_DSI_FMT_RGB888;
> >
> > enum mipi_dsi_pixel_format {
> > MIPI_DSI_FMT_RGB888,
> > MIPI_DSI_FMT_RGB666,
> > MIPI_DSI_FMT_RGB666_PACKED,
> > MIPI_DSI_FMT_RGB565,
> > };
> > include/drm/drm_mipi_dsi.h
> >
> > Why do we require this atomic_get_input_bus_fmts?
> >
> > Do i need to implement both atomic_get_input_bus_fmts and
> > atomic_get_output_bus_fmts?
>
> .atomic_get_output_bus_fmts() is only need for the last bridge in the
> chain, and is not mandatory when that bridge supports a single format.
> As this bridge can't be last (if the output is connect to a panel, there
> will be a drm_bridge wrapping the drm_panel), you don't have to
> implement that operation.
>
> .atomic_get_input_bus_fmts() is used to negotiate formats along the
> pipeline. The helps the DRM bridge helpers figure out what formats are
> possible, with the help of bridges that must report what input formats
> are compatible with a given output format. The DRM bridge helpers will
> take care of the rest.
>
> So, for this bridge, the input and output formats are decoupled. The
> bridge can output any of the three supported LVDS formats, regardless of
> what format it gets at its input. You should thus verify that the output
> format you receive in this function is supported (and return NULL if it
> isn't), and then return the list of supported input formats. If you
> don't implement .atomic_get_input_bus_fmts(), then the DRM bridge
> helpers will consider that the input and output formats are the same,
> and will set the output format of the previous bridge to, for example,
> MEDIA_BUS_FMT_RGB666_1X7X3_SPWG. It may work if the previous bridge
> doesn't care about its output format, but if it does, then it will be
> puzzled, as the previous bridge outputs DSI, not LVDS.
>
> > > As far as I can tell, the hardware support transcoding any of the
> > > supported input formats (RGB565, RGB666 or RGB888) to any of the
> > > supported output formats. How about the following ?
> > >
> > > static const u32 tc_lvds_in_bus_fmts[] = {
> > > MEDIA_BUS_FMT_RGB565_1X16,
> > > MEDIA_BUS_FMT_RGB666_1X18,
> > > MEDIA_BUS_FMT_RBG888_1X24,
> > > };
> > >
> > > ...
> > >
> > > u32 *input_fmts;
> > > unsigned int i;
> > >
> > > *num_input_fmts = 0;
> > >
> > > for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > if (output_fmt == tc_lvds_out_bus_fmts[i])
> > > break;
> > > }
> > >
> > > if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
> > > return NULL;
> > >
> > > input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> > > GFP_KERNEL);
> > > if (!input_fmts)
> > > return NULL;
> > >
> > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > >
> > > *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > > return input_fmts;
> > >
> > > > +
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + return input_fmts;
> > > > +}
> > > > +
> > > > static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > enum drm_bridge_attach_flags flags)
> > > > {
> > > > @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > }
> > > >
> > > > static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
> > > > + .atomic_check = tc_bridge_atomic_check,
> > > > .attach = tc_bridge_attach,
> > > > .pre_enable = tc_bridge_pre_enable,
> > > > .enable = tc_bridge_enable,
>
> --
> Regards,
>
> Laurent Pinchart



--
regards,
vinaysimha

2020-08-12 17:19:01

by Vinay Simha B N

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

laurent,

if i add the .atomic_get_input_bus_fmts =
tc_bridge_get_input_bus_fmts, with the implementation suggested,
system does not boot fully, the reason is, we capture all the
supported input formats, but not sure where to set the final input
format. Please suggest.

for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
input_fmts[i] = tc_lvds_in_bus_fmts[i];

*num_input_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);

On Wed, Aug 12, 2020 at 8:25 PM Vinay Simha B N <[email protected]> wrote:
>
> laurent,
>
> Video data input format : RGB666 loosely packed 24 bits per pixel
> Can we use MEDIA_BUS_FMT_RGB666_1X24_CPADHI? There was no information
> wrt CPADHI or for loosely packed
>
> static const u32 tc_lvds_in_bus_fmts[] = {
> MEDIA_BUS_FMT_RGB565_1X16,
> MEDIA_BUS_FMT_RGB666_1X18,
> MEDIA_BUS_FMT_RGB666_1X24_CPADHI,
> MEDIA_BUS_FMT_RBG888_1X24,
> };
>
> for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> input_fmts[i] = tc_lvds_in_bus_fmts[i];
> >> This will have all the available input formats, but finally which video data input format chosen?
> Since dsi->format = MIPI_DSI_FMT_RGB888 is used does it chooses
> MEDIA_BUS_FMT_RBG888_1X24 by the drm pipeline
>
> On Wed, Aug 12, 2020 at 6:48 PM Laurent Pinchart
> <[email protected]> wrote:
> >
> > Hi Vinay,
> >
> > On Wed, Aug 12, 2020 at 06:07:52PM +0530, Vinay Simha B N wrote:
> > > On Wed, Aug 12, 2020 at 3:24 PM Laurent Pinchart wrote:
> > > > On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> > > > > - bus formats read from drm_bridge_state.output_bus_cfg.format
> > > > > and .atomic_get_input_bus_fmts() instead of connector
> > > > >
> > > > > Signed-off-by: Vinay Simha BN <[email protected]>
> > > > >
> > > > > ---
> > > > > v1:
> > > > > * Laurent Pinchart review comments incorporated
> > > > > drm_bridge_state.output_bus_cfg.format
> > > > > instead of connector
> > > > > ---
> > > > > drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
> > > > > 1 file changed, 59 insertions(+), 17 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> > > > > index 7da15cd..5d8714a 100644
> > > > > --- a/drivers/gpu/drm/bridge/tc358775.c
> > > > > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > > > > @@ -271,6 +271,13 @@ struct tc_data {
> > > > > struct gpio_desc *stby_gpio;
> > > > > u8 lvds_link; /* single-link or dual-link */
> > > > > u8 bpc;
> > > > > + u32 output_bus_fmt;
> > > > > +};
> > > > > +
> > > > > +static const u32 tc_lvds_out_bus_fmts[] = {
> > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > > > + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > > > };
> > > > >
> > > > > static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> > > > > @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> > > > > ret, addr);
> > > > > }
> > > > >
> > > > > -/* helper function to access bus_formats */
> > > > > -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> > > > > -{
> > > > > - struct drm_device *dev = encoder->dev;
> > > > > - struct drm_connector *connector;
> > > > > -
> > > > > - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > > > - if (connector->encoder == encoder)
> > > > > - return connector;
> > > > > -
> > > > > - return NULL;
> > > > > -}
> > > > > -
> > > > > static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > {
> > > > > struct tc_data *tc = bridge_to_tc(bridge);
> > > > > @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > u32 val = 0;
> > > > > u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> > > > > struct drm_display_mode *mode;
> > > > > - struct drm_connector *connector = get_connector(bridge->encoder);
> > > > >
> > > > > mode = &bridge->encoder->crtc->state->adjusted_mode;
> > > > >
> > > > > @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
> > > > >
> > > > > dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> > > > > - connector->display_info.bus_formats[0],
> > > > > + tc->output_bus_fmt,
> > > > > tc->bpc);
> > > > > /*
> > > > > * Default hardware register settings of tc358775 configured
> > > > > * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
> > > > > */
> > > > > - if (connector->display_info.bus_formats[0] ==
> > > > > - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > + if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > /* VESA-24 */
> > > > > d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> > > > > d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> > > > > @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> > > > > + struct drm_bridge_state *bridge_state,
> > > > > + struct drm_crtc_state *crtc_state,
> > > > > + struct drm_connector_state *conn_state)
> > > > > +{
> > > > > + struct tc_data *tc = bridge_to_tc(bridge);
> > > > > +
> > > > > + tc->output_bus_fmt = bridge_state->output_bus_cfg.format;
> > > >
> > > > .atomic_check() isn't allowed to modify the device state, neither the
> > > > hardware state nor the software state in drm_bridge or tc_data. You can
> > > > instead access the bridge state directly in tc_bridge_enable(), with
> > > >
> > > > struct drm_bridge_state *state =
> > > > drm_priv_to_bridge_state(bridge->base.state);
> > >
> > > Currently the driver is picking up from the dts panel
> > > (data-mapping = "vesa-24";) or jeida-24 or jeida-18.
> > >
> > > Does state->output_bus_cfg.format get set from the data-mapping?
> >
> > It should. The drm_panel should take care of that. In
> > panel_simple_get_non_edid_modes(), it calls
> >
> > if (panel->desc->bus_format)
> > drm_display_info_set_bus_formats(&connector->display_info,
> > &panel->desc->bus_format, 1);
> >
> > to initialize the bus format in display_info. Then, the DRM bridge
> > helper drm_atomic_bridge_chain_select_bus_fmts() retrieves the output
> > format by calling .atomic_get_output_bus_fmts() if implemented by the
> > last bridge in the chain, or directly from the connector display_info.
> > The last bridge in the chain is a DRM panel bridge, and doesn't
> > implement .atomic_get_output_bus_fmts(), so the format from display_info
> > is used, and is stored in the output_bus_cfg.format field of this bridge
> > in select_bus_fmt_recursive().
> >
> > If something doesn't work according to the plan, I can help you
> > debugging.
> >
> > > > > +
> > > > > + dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static u32 *
> > > > > +tc_bridge_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 = NULL;
> > > > > + int i;
> > > >
> > > > i only takes positive values, so it can be an unsigned int.
> > > >
> > > > > +
> > > > > + *num_input_fmts = 0;
> > > > > +
> > > > > + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > > > + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> > > > > + *num_input_fmts = 1;
> > > > > + input_fmts = kcalloc(*num_input_fmts,
> > > > > + sizeof(*input_fmts),
> > > > > + GFP_KERNEL);
> > > > > + if (!input_fmts)
> > > > > + return NULL;
> > > > > +
> > > > > + input_fmts[0] = output_fmt;
> > > >
> > > > I don't think this is right, the input of the bridge isn't LVDS, is it ?
> > >
> > > Input to the bridge is DSI, format is already set
> > >
> > > dsi->format = MIPI_DSI_FMT_RGB888;
> > >
> > > enum mipi_dsi_pixel_format {
> > > MIPI_DSI_FMT_RGB888,
> > > MIPI_DSI_FMT_RGB666,
> > > MIPI_DSI_FMT_RGB666_PACKED,
> > > MIPI_DSI_FMT_RGB565,
> > > };
> > > include/drm/drm_mipi_dsi.h
> > >
> > > Why do we require this atomic_get_input_bus_fmts?
> > >
> > > Do i need to implement both atomic_get_input_bus_fmts and
> > > atomic_get_output_bus_fmts?
> >
> > .atomic_get_output_bus_fmts() is only need for the last bridge in the
> > chain, and is not mandatory when that bridge supports a single format.
> > As this bridge can't be last (if the output is connect to a panel, there
> > will be a drm_bridge wrapping the drm_panel), you don't have to
> > implement that operation.
> >
> > .atomic_get_input_bus_fmts() is used to negotiate formats along the
> > pipeline. The helps the DRM bridge helpers figure out what formats are
> > possible, with the help of bridges that must report what input formats
> > are compatible with a given output format. The DRM bridge helpers will
> > take care of the rest.
> >
> > So, for this bridge, the input and output formats are decoupled. The
> > bridge can output any of the three supported LVDS formats, regardless of
> > what format it gets at its input. You should thus verify that the output
> > format you receive in this function is supported (and return NULL if it
> > isn't), and then return the list of supported input formats. If you
> > don't implement .atomic_get_input_bus_fmts(), then the DRM bridge
> > helpers will consider that the input and output formats are the same,
> > and will set the output format of the previous bridge to, for example,
> > MEDIA_BUS_FMT_RGB666_1X7X3_SPWG. It may work if the previous bridge
> > doesn't care about its output format, but if it does, then it will be
> > puzzled, as the previous bridge outputs DSI, not LVDS.
> >
> > > > As far as I can tell, the hardware support transcoding any of the
> > > > supported input formats (RGB565, RGB666 or RGB888) to any of the
> > > > supported output formats. How about the following ?
> > > >
> > > > static const u32 tc_lvds_in_bus_fmts[] = {
> > > > MEDIA_BUS_FMT_RGB565_1X16,
> > > > MEDIA_BUS_FMT_RGB666_1X18,
> > > > MEDIA_BUS_FMT_RBG888_1X24,
> > > > };
> > > >
> > > > ...
> > > >
> > > > u32 *input_fmts;
> > > > unsigned int i;
> > > >
> > > > *num_input_fmts = 0;
> > > >
> > > > for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > > if (output_fmt == tc_lvds_out_bus_fmts[i])
> > > > break;
> > > > }
> > > >
> > > > if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
> > > > return NULL;
> > > >
> > > > input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> > > > GFP_KERNEL);
> > > > if (!input_fmts)
> > > > return NULL;
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > > >
> > > > *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > > > return input_fmts;
> > > >
> > > > > +
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + return input_fmts;
> > > > > +}
> > > > > +
> > > > > static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > > enum drm_bridge_attach_flags flags)
> > > > > {
> > > > > @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > > }
> > > > >
> > > > > static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
> > > > > + .atomic_check = tc_bridge_atomic_check,
> > > > > .attach = tc_bridge_attach,
> > > > > .pre_enable = tc_bridge_pre_enable,
> > > > > .enable = tc_bridge_enable,
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
>
> --
> regards,
> vinaysimha



--
regards,
vinaysimha

2020-08-13 15:41:00

by Vinay Simha B N

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

laurent,

The code sequence was a problem. *num_inputs_fmts =
ARRAY_SIZE(tc_lvds_in_bus_fmts); should come first and then allocate
the kcalloc.

input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
GFP_KERNEL);
..
for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
input_fmts[i] = tc_lvds_in_bus_fmts[i];

*num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);

So, internally in the drm pipeline get set the input format based on
the output formats?

On Wed, Aug 12, 2020 at 10:45 PM Vinay Simha B N <[email protected]> wrote:
>
> laurent,
>
> if i add the .atomic_get_input_bus_fmts =
> tc_bridge_get_input_bus_fmts, with the implementation suggested,
> system does not boot fully, the reason is, we capture all the
> supported input formats, but not sure where to set the final input
> format. Please suggest.
>
> for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> input_fmts[i] = tc_lvds_in_bus_fmts[i];
>
> *num_input_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
>
> On Wed, Aug 12, 2020 at 8:25 PM Vinay Simha B N <[email protected]> wrote:
> >
> > laurent,
> >
> > Video data input format : RGB666 loosely packed 24 bits per pixel
> > Can we use MEDIA_BUS_FMT_RGB666_1X24_CPADHI? There was no information
> > wrt CPADHI or for loosely packed
> >
> > static const u32 tc_lvds_in_bus_fmts[] = {
> > MEDIA_BUS_FMT_RGB565_1X16,
> > MEDIA_BUS_FMT_RGB666_1X18,
> > MEDIA_BUS_FMT_RGB666_1X24_CPADHI,
> > MEDIA_BUS_FMT_RBG888_1X24,
> > };
> >
> > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > >> This will have all the available input formats, but finally which video data input format chosen?
> > Since dsi->format = MIPI_DSI_FMT_RGB888 is used does it chooses
> > MEDIA_BUS_FMT_RBG888_1X24 by the drm pipeline
> >
> > On Wed, Aug 12, 2020 at 6:48 PM Laurent Pinchart
> > <[email protected]> wrote:
> > >
> > > Hi Vinay,
> > >
> > > On Wed, Aug 12, 2020 at 06:07:52PM +0530, Vinay Simha B N wrote:
> > > > On Wed, Aug 12, 2020 at 3:24 PM Laurent Pinchart wrote:
> > > > > On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> > > > > > - bus formats read from drm_bridge_state.output_bus_cfg.format
> > > > > > and .atomic_get_input_bus_fmts() instead of connector
> > > > > >
> > > > > > Signed-off-by: Vinay Simha BN <[email protected]>
> > > > > >
> > > > > > ---
> > > > > > v1:
> > > > > > * Laurent Pinchart review comments incorporated
> > > > > > drm_bridge_state.output_bus_cfg.format
> > > > > > instead of connector
> > > > > > ---
> > > > > > drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
> > > > > > 1 file changed, 59 insertions(+), 17 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> > > > > > index 7da15cd..5d8714a 100644
> > > > > > --- a/drivers/gpu/drm/bridge/tc358775.c
> > > > > > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > > > > > @@ -271,6 +271,13 @@ struct tc_data {
> > > > > > struct gpio_desc *stby_gpio;
> > > > > > u8 lvds_link; /* single-link or dual-link */
> > > > > > u8 bpc;
> > > > > > + u32 output_bus_fmt;
> > > > > > +};
> > > > > > +
> > > > > > +static const u32 tc_lvds_out_bus_fmts[] = {
> > > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > > > > + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > > > > };
> > > > > >
> > > > > > static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> > > > > > @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> > > > > > ret, addr);
> > > > > > }
> > > > > >
> > > > > > -/* helper function to access bus_formats */
> > > > > > -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> > > > > > -{
> > > > > > - struct drm_device *dev = encoder->dev;
> > > > > > - struct drm_connector *connector;
> > > > > > -
> > > > > > - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > > > > - if (connector->encoder == encoder)
> > > > > > - return connector;
> > > > > > -
> > > > > > - return NULL;
> > > > > > -}
> > > > > > -
> > > > > > static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > > {
> > > > > > struct tc_data *tc = bridge_to_tc(bridge);
> > > > > > @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > > u32 val = 0;
> > > > > > u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> > > > > > struct drm_display_mode *mode;
> > > > > > - struct drm_connector *connector = get_connector(bridge->encoder);
> > > > > >
> > > > > > mode = &bridge->encoder->crtc->state->adjusted_mode;
> > > > > >
> > > > > > @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > > d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
> > > > > >
> > > > > > dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> > > > > > - connector->display_info.bus_formats[0],
> > > > > > + tc->output_bus_fmt,
> > > > > > tc->bpc);
> > > > > > /*
> > > > > > * Default hardware register settings of tc358775 configured
> > > > > > * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
> > > > > > */
> > > > > > - if (connector->display_info.bus_formats[0] ==
> > > > > > - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > > + if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > > /* VESA-24 */
> > > > > > d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> > > > > > d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> > > > > > @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> > > > > > + struct drm_bridge_state *bridge_state,
> > > > > > + struct drm_crtc_state *crtc_state,
> > > > > > + struct drm_connector_state *conn_state)
> > > > > > +{
> > > > > > + struct tc_data *tc = bridge_to_tc(bridge);
> > > > > > +
> > > > > > + tc->output_bus_fmt = bridge_state->output_bus_cfg.format;
> > > > >
> > > > > .atomic_check() isn't allowed to modify the device state, neither the
> > > > > hardware state nor the software state in drm_bridge or tc_data. You can
> > > > > instead access the bridge state directly in tc_bridge_enable(), with
> > > > >
> > > > > struct drm_bridge_state *state =
> > > > > drm_priv_to_bridge_state(bridge->base.state);
> > > >
> > > > Currently the driver is picking up from the dts panel
> > > > (data-mapping = "vesa-24";) or jeida-24 or jeida-18.
> > > >
> > > > Does state->output_bus_cfg.format get set from the data-mapping?
> > >
> > > It should. The drm_panel should take care of that. In
> > > panel_simple_get_non_edid_modes(), it calls
> > >
> > > if (panel->desc->bus_format)
> > > drm_display_info_set_bus_formats(&connector->display_info,
> > > &panel->desc->bus_format, 1);
> > >
> > > to initialize the bus format in display_info. Then, the DRM bridge
> > > helper drm_atomic_bridge_chain_select_bus_fmts() retrieves the output
> > > format by calling .atomic_get_output_bus_fmts() if implemented by the
> > > last bridge in the chain, or directly from the connector display_info.
> > > The last bridge in the chain is a DRM panel bridge, and doesn't
> > > implement .atomic_get_output_bus_fmts(), so the format from display_info
> > > is used, and is stored in the output_bus_cfg.format field of this bridge
> > > in select_bus_fmt_recursive().
> > >
> > > If something doesn't work according to the plan, I can help you
> > > debugging.
> > >
> > > > > > +
> > > > > > + dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static u32 *
> > > > > > +tc_bridge_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 = NULL;
> > > > > > + int i;
> > > > >
> > > > > i only takes positive values, so it can be an unsigned int.
> > > > >
> > > > > > +
> > > > > > + *num_input_fmts = 0;
> > > > > > +
> > > > > > + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > > > > + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> > > > > > + *num_input_fmts = 1;
> > > > > > + input_fmts = kcalloc(*num_input_fmts,
> > > > > > + sizeof(*input_fmts),
> > > > > > + GFP_KERNEL);
> > > > > > + if (!input_fmts)
> > > > > > + return NULL;
> > > > > > +
> > > > > > + input_fmts[0] = output_fmt;
> > > > >
> > > > > I don't think this is right, the input of the bridge isn't LVDS, is it ?
> > > >
> > > > Input to the bridge is DSI, format is already set
> > > >
> > > > dsi->format = MIPI_DSI_FMT_RGB888;
> > > >
> > > > enum mipi_dsi_pixel_format {
> > > > MIPI_DSI_FMT_RGB888,
> > > > MIPI_DSI_FMT_RGB666,
> > > > MIPI_DSI_FMT_RGB666_PACKED,
> > > > MIPI_DSI_FMT_RGB565,
> > > > };
> > > > include/drm/drm_mipi_dsi.h
> > > >
> > > > Why do we require this atomic_get_input_bus_fmts?
> > > >
> > > > Do i need to implement both atomic_get_input_bus_fmts and
> > > > atomic_get_output_bus_fmts?
> > >
> > > .atomic_get_output_bus_fmts() is only need for the last bridge in the
> > > chain, and is not mandatory when that bridge supports a single format.
> > > As this bridge can't be last (if the output is connect to a panel, there
> > > will be a drm_bridge wrapping the drm_panel), you don't have to
> > > implement that operation.
> > >
> > > .atomic_get_input_bus_fmts() is used to negotiate formats along the
> > > pipeline. The helps the DRM bridge helpers figure out what formats are
> > > possible, with the help of bridges that must report what input formats
> > > are compatible with a given output format. The DRM bridge helpers will
> > > take care of the rest.
> > >
> > > So, for this bridge, the input and output formats are decoupled. The
> > > bridge can output any of the three supported LVDS formats, regardless of
> > > what format it gets at its input. You should thus verify that the output
> > > format you receive in this function is supported (and return NULL if it
> > > isn't), and then return the list of supported input formats. If you
> > > don't implement .atomic_get_input_bus_fmts(), then the DRM bridge
> > > helpers will consider that the input and output formats are the same,
> > > and will set the output format of the previous bridge to, for example,
> > > MEDIA_BUS_FMT_RGB666_1X7X3_SPWG. It may work if the previous bridge
> > > doesn't care about its output format, but if it does, then it will be
> > > puzzled, as the previous bridge outputs DSI, not LVDS.
> > >
> > > > > As far as I can tell, the hardware support transcoding any of the
> > > > > supported input formats (RGB565, RGB666 or RGB888) to any of the
> > > > > supported output formats. How about the following ?
> > > > >
> > > > > static const u32 tc_lvds_in_bus_fmts[] = {
> > > > > MEDIA_BUS_FMT_RGB565_1X16,
> > > > > MEDIA_BUS_FMT_RGB666_1X18,
> > > > > MEDIA_BUS_FMT_RBG888_1X24,
> > > > > };
> > > > >
> > > > > ...
> > > > >
> > > > > u32 *input_fmts;
> > > > > unsigned int i;
> > > > >
> > > > > *num_input_fmts = 0;
> > > > >
> > > > > for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > > > if (output_fmt == tc_lvds_out_bus_fmts[i])
> > > > > break;
> > > > > }
> > > > >
> > > > > if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
> > > > > return NULL;
> > > > >
> > > > > input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> > > > > GFP_KERNEL);
> > > > > if (!input_fmts)
> > > > > return NULL;
> > > > >
> > > > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > > > >
> > > > > *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > > > > return input_fmts;
> > > > >
> > > > > > +
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + return input_fmts;
> > > > > > +}
> > > > > > +
> > > > > > static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > > > enum drm_bridge_attach_flags flags)
> > > > > > {
> > > > > > @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > > > }
> > > > > >
> > > > > > static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
> > > > > > + .atomic_check = tc_bridge_atomic_check,
> > > > > > .attach = tc_bridge_attach,
> > > > > > .pre_enable = tc_bridge_pre_enable,
> > > > > > .enable = tc_bridge_enable,
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >
> >
> >
> > --
> > regards,
> > vinaysimha
>
>
>
> --
> regards,
> vinaysimha



--
regards,
vinaysimha

2020-08-25 14:30:45

by Vinay Simha B N

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

laurent,

Please review or give some feedback.

On Thu, Aug 13, 2020 at 9:09 PM Vinay Simha B N <[email protected]> wrote:
>
> laurent,
>
> The code sequence was a problem. *num_inputs_fmts =
> ARRAY_SIZE(tc_lvds_in_bus_fmts); should come first and then allocate
> the kcalloc.
>
> input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> GFP_KERNEL);
> ..
> for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> input_fmts[i] = tc_lvds_in_bus_fmts[i];
>
> *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
>
> So, internally in the drm pipeline get set the input format based on
> the output formats?
>
> On Wed, Aug 12, 2020 at 10:45 PM Vinay Simha B N <[email protected]> wrote:
> >
> > laurent,
> >
> > if i add the .atomic_get_input_bus_fmts =
> > tc_bridge_get_input_bus_fmts, with the implementation suggested,
> > system does not boot fully, the reason is, we capture all the
> > supported input formats, but not sure where to set the final input
> > format. Please suggest.
> >
> > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> >
> > *num_input_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> >
> > On Wed, Aug 12, 2020 at 8:25 PM Vinay Simha B N <[email protected]> wrote:
> > >
> > > laurent,
> > >
> > > Video data input format : RGB666 loosely packed 24 bits per pixel
> > > Can we use MEDIA_BUS_FMT_RGB666_1X24_CPADHI? There was no information
> > > wrt CPADHI or for loosely packed
> > >
> > > static const u32 tc_lvds_in_bus_fmts[] = {
> > > MEDIA_BUS_FMT_RGB565_1X16,
> > > MEDIA_BUS_FMT_RGB666_1X18,
> > > MEDIA_BUS_FMT_RGB666_1X24_CPADHI,
> > > MEDIA_BUS_FMT_RBG888_1X24,
> > > };
> > >
> > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > > >> This will have all the available input formats, but finally which video data input format chosen?
> > > Since dsi->format = MIPI_DSI_FMT_RGB888 is used does it chooses
> > > MEDIA_BUS_FMT_RBG888_1X24 by the drm pipeline
> > >
> > > On Wed, Aug 12, 2020 at 6:48 PM Laurent Pinchart
> > > <[email protected]> wrote:
> > > >
> > > > Hi Vinay,
> > > >
> > > > On Wed, Aug 12, 2020 at 06:07:52PM +0530, Vinay Simha B N wrote:
> > > > > On Wed, Aug 12, 2020 at 3:24 PM Laurent Pinchart wrote:
> > > > > > On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> > > > > > > - bus formats read from drm_bridge_state.output_bus_cfg.format
> > > > > > > and .atomic_get_input_bus_fmts() instead of connector
> > > > > > >
> > > > > > > Signed-off-by: Vinay Simha BN <[email protected]>
> > > > > > >
> > > > > > > ---
> > > > > > > v1:
> > > > > > > * Laurent Pinchart review comments incorporated
> > > > > > > drm_bridge_state.output_bus_cfg.format
> > > > > > > instead of connector
> > > > > > > ---
> > > > > > > drivers/gpu/drm/bridge/tc358775.c | 76 ++++++++++++++++++++++++++++++---------
> > > > > > > 1 file changed, 59 insertions(+), 17 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> > > > > > > index 7da15cd..5d8714a 100644
> > > > > > > --- a/drivers/gpu/drm/bridge/tc358775.c
> > > > > > > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > > > > > > @@ -271,6 +271,13 @@ struct tc_data {
> > > > > > > struct gpio_desc *stby_gpio;
> > > > > > > u8 lvds_link; /* single-link or dual-link */
> > > > > > > u8 bpc;
> > > > > > > + u32 output_bus_fmt;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const u32 tc_lvds_out_bus_fmts[] = {
> > > > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > > > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > > > > > + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > > > > > };
> > > > > > >
> > > > > > > static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> > > > > > > @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> > > > > > > ret, addr);
> > > > > > > }
> > > > > > >
> > > > > > > -/* helper function to access bus_formats */
> > > > > > > -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> > > > > > > -{
> > > > > > > - struct drm_device *dev = encoder->dev;
> > > > > > > - struct drm_connector *connector;
> > > > > > > -
> > > > > > > - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > > > > > > - if (connector->encoder == encoder)
> > > > > > > - return connector;
> > > > > > > -
> > > > > > > - return NULL;
> > > > > > > -}
> > > > > > > -
> > > > > > > static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > > > {
> > > > > > > struct tc_data *tc = bridge_to_tc(bridge);
> > > > > > > @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > > > u32 val = 0;
> > > > > > > u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> > > > > > > struct drm_display_mode *mode;
> > > > > > > - struct drm_connector *connector = get_connector(bridge->encoder);
> > > > > > >
> > > > > > > mode = &bridge->encoder->crtc->state->adjusted_mode;
> > > > > > >
> > > > > > > @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > > > d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
> > > > > > >
> > > > > > > dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> > > > > > > - connector->display_info.bus_formats[0],
> > > > > > > + tc->output_bus_fmt,
> > > > > > > tc->bpc);
> > > > > > > /*
> > > > > > > * Default hardware register settings of tc358775 configured
> > > > > > > * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
> > > > > > > */
> > > > > > > - if (connector->display_info.bus_formats[0] ==
> > > > > > > - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > > > + if (tc->output_bus_fmt == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > > > /* VESA-24 */
> > > > > > > d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> > > > > > > d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> > > > > > > @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > > > +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> > > > > > > + struct drm_bridge_state *bridge_state,
> > > > > > > + struct drm_crtc_state *crtc_state,
> > > > > > > + struct drm_connector_state *conn_state)
> > > > > > > +{
> > > > > > > + struct tc_data *tc = bridge_to_tc(bridge);
> > > > > > > +
> > > > > > > + tc->output_bus_fmt = bridge_state->output_bus_cfg.format;
> > > > > >
> > > > > > .atomic_check() isn't allowed to modify the device state, neither the
> > > > > > hardware state nor the software state in drm_bridge or tc_data. You can
> > > > > > instead access the bridge state directly in tc_bridge_enable(), with
> > > > > >
> > > > > > struct drm_bridge_state *state =
> > > > > > drm_priv_to_bridge_state(bridge->base.state);
> > > > >
> > > > > Currently the driver is picking up from the dts panel
> > > > > (data-mapping = "vesa-24";) or jeida-24 or jeida-18.
> > > > >
> > > > > Does state->output_bus_cfg.format get set from the data-mapping?
> > > >
> > > > It should. The drm_panel should take care of that. In
> > > > panel_simple_get_non_edid_modes(), it calls
> > > >
> > > > if (panel->desc->bus_format)
> > > > drm_display_info_set_bus_formats(&connector->display_info,
> > > > &panel->desc->bus_format, 1);
> > > >
> > > > to initialize the bus format in display_info. Then, the DRM bridge
> > > > helper drm_atomic_bridge_chain_select_bus_fmts() retrieves the output
> > > > format by calling .atomic_get_output_bus_fmts() if implemented by the
> > > > last bridge in the chain, or directly from the connector display_info.
> > > > The last bridge in the chain is a DRM panel bridge, and doesn't
> > > > implement .atomic_get_output_bus_fmts(), so the format from display_info
> > > > is used, and is stored in the output_bus_cfg.format field of this bridge
> > > > in select_bus_fmt_recursive().
> > > >
> > > > If something doesn't work according to the plan, I can help you
> > > > debugging.
> > > >
> > > > > > > +
> > > > > > > + dev_dbg(tc->dev, "output_bus_fmt %04x\n", tc->output_bus_fmt);
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static u32 *
> > > > > > > +tc_bridge_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 = NULL;
> > > > > > > + int i;
> > > > > >
> > > > > > i only takes positive values, so it can be an unsigned int.
> > > > > >
> > > > > > > +
> > > > > > > + *num_input_fmts = 0;
> > > > > > > +
> > > > > > > + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > > > > > + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> > > > > > > + *num_input_fmts = 1;
> > > > > > > + input_fmts = kcalloc(*num_input_fmts,
> > > > > > > + sizeof(*input_fmts),
> > > > > > > + GFP_KERNEL);
> > > > > > > + if (!input_fmts)
> > > > > > > + return NULL;
> > > > > > > +
> > > > > > > + input_fmts[0] = output_fmt;
> > > > > >
> > > > > > I don't think this is right, the input of the bridge isn't LVDS, is it ?
> > > > >
> > > > > Input to the bridge is DSI, format is already set
> > > > >
> > > > > dsi->format = MIPI_DSI_FMT_RGB888;
> > > > >
> > > > > enum mipi_dsi_pixel_format {
> > > > > MIPI_DSI_FMT_RGB888,
> > > > > MIPI_DSI_FMT_RGB666,
> > > > > MIPI_DSI_FMT_RGB666_PACKED,
> > > > > MIPI_DSI_FMT_RGB565,
> > > > > };
> > > > > include/drm/drm_mipi_dsi.h
> > > > >
> > > > > Why do we require this atomic_get_input_bus_fmts?
> > > > >
> > > > > Do i need to implement both atomic_get_input_bus_fmts and
> > > > > atomic_get_output_bus_fmts?
> > > >
> > > > .atomic_get_output_bus_fmts() is only need for the last bridge in the
> > > > chain, and is not mandatory when that bridge supports a single format.
> > > > As this bridge can't be last (if the output is connect to a panel, there
> > > > will be a drm_bridge wrapping the drm_panel), you don't have to
> > > > implement that operation.
> > > >
> > > > .atomic_get_input_bus_fmts() is used to negotiate formats along the
> > > > pipeline. The helps the DRM bridge helpers figure out what formats are
> > > > possible, with the help of bridges that must report what input formats
> > > > are compatible with a given output format. The DRM bridge helpers will
> > > > take care of the rest.
> > > >
> > > > So, for this bridge, the input and output formats are decoupled. The
> > > > bridge can output any of the three supported LVDS formats, regardless of
> > > > what format it gets at its input. You should thus verify that the output
> > > > format you receive in this function is supported (and return NULL if it
> > > > isn't), and then return the list of supported input formats. If you
> > > > don't implement .atomic_get_input_bus_fmts(), then the DRM bridge
> > > > helpers will consider that the input and output formats are the same,
> > > > and will set the output format of the previous bridge to, for example,
> > > > MEDIA_BUS_FMT_RGB666_1X7X3_SPWG. It may work if the previous bridge
> > > > doesn't care about its output format, but if it does, then it will be
> > > > puzzled, as the previous bridge outputs DSI, not LVDS.
> > > >
> > > > > > As far as I can tell, the hardware support transcoding any of the
> > > > > > supported input formats (RGB565, RGB666 or RGB888) to any of the
> > > > > > supported output formats. How about the following ?
> > > > > >
> > > > > > static const u32 tc_lvds_in_bus_fmts[] = {
> > > > > > MEDIA_BUS_FMT_RGB565_1X16,
> > > > > > MEDIA_BUS_FMT_RGB666_1X18,
> > > > > > MEDIA_BUS_FMT_RBG888_1X24,
> > > > > > };
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > u32 *input_fmts;
> > > > > > unsigned int i;
> > > > > >
> > > > > > *num_input_fmts = 0;
> > > > > >
> > > > > > for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ; ++i) {
> > > > > > if (output_fmt == tc_lvds_out_bus_fmts[i])
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
> > > > > > return NULL;
> > > > > >
> > > > > > input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> > > > > > GFP_KERNEL);
> > > > > > if (!input_fmts)
> > > > > > return NULL;
> > > > > >
> > > > > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > > > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > > > > >
> > > > > > *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > > > > > return input_fmts;
> > > > > >
> > > > > > > +
> > > > > > > + break;
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > + return input_fmts;
> > > > > > > +}
> > > > > > > +
> > > > > > > static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > > > > enum drm_bridge_attach_flags flags)
> > > > > > > {
> > > > > > > @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > > > > }
> > > > > > >
> > > > > > > static const struct drm_bridge_funcs tc_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 = tc_bridge_get_input_bus_fmts,
> > > > > > > + .atomic_check = tc_bridge_atomic_check,
> > > > > > > .attach = tc_bridge_attach,
> > > > > > > .pre_enable = tc_bridge_pre_enable,
> > > > > > > .enable = tc_bridge_enable,
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Laurent Pinchart
> > >
> > >
> > >
> > > --
> > > regards,
> > > vinaysimha
> >
> >
> >
> > --
> > regards,
> > vinaysimha
>
>
>
> --
> regards,
> vinaysimha



--
regards,
vinaysimha

2020-09-08 17:58:58

by Laurent Pinchart

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

Hi Vinay,

On Tue, Sep 08, 2020 at 11:22:48PM +0530, Vinay Simha B N wrote:
> laurent,
>
> Please review or give some feedback.

I'm sorry, I have very little time these days :-( Maybe Neil can provide
feedback ?

> On Tue, Aug 25, 2020 at 7:57 PM Vinay Simha B N <[email protected]> wrote:
>
> > laurent,
> >
> > Please review or give some feedback.
> >
> > On Thu, Aug 13, 2020 at 9:09 PM Vinay Simha B N <[email protected]>
> > wrote:
> > >
> > > laurent,
> > >
> > > The code sequence was a problem. *num_inputs_fmts =
> > > ARRAY_SIZE(tc_lvds_in_bus_fmts); should come first and then allocate
> > > the kcalloc.
> > >
> > > input_fmts = kcalloc(*num_input_fmts, ARRAY_SIZE(tc_lvds_in_bus_fmts),
> > > GFP_KERNEL);
> > > ..
> > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > >
> > > *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > >
> > > So, internally in the drm pipeline get set the input format based on
> > > the output formats?
> > >
> > > On Wed, Aug 12, 2020 at 10:45 PM Vinay Simha B N <[email protected]>
> > wrote:
> > > >
> > > > laurent,
> > > >
> > > > if i add the .atomic_get_input_bus_fmts =
> > > > tc_bridge_get_input_bus_fmts, with the implementation suggested,
> > > > system does not boot fully, the reason is, we capture all the
> > > > supported input formats, but not sure where to set the final input
> > > > format. Please suggest.
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > > >
> > > > *num_input_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > > >
> > > > On Wed, Aug 12, 2020 at 8:25 PM Vinay Simha B N <[email protected]>
> > wrote:
> > > > >
> > > > > laurent,
> > > > >
> > > > > Video data input format : RGB666 loosely packed 24 bits per pixel
> > > > > Can we use MEDIA_BUS_FMT_RGB666_1X24_CPADHI? There was no information
> > > > > wrt CPADHI or for loosely packed
> > > > >
> > > > > static const u32 tc_lvds_in_bus_fmts[] = {
> > > > > MEDIA_BUS_FMT_RGB565_1X16,
> > > > > MEDIA_BUS_FMT_RGB666_1X18,
> > > > > MEDIA_BUS_FMT_RGB666_1X24_CPADHI,
> > > > > MEDIA_BUS_FMT_RBG888_1X24,
> > > > > };
> > > > >
> > > > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > > > > >> This will have all the available input formats, but finally which
> > video data input format chosen?
> > > > > Since dsi->format = MIPI_DSI_FMT_RGB888 is used does it chooses
> > > > > MEDIA_BUS_FMT_RBG888_1X24 by the drm pipeline
> > > > >
> > > > > On Wed, Aug 12, 2020 at 6:48 PM Laurent Pinchart
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Hi Vinay,
> > > > > >
> > > > > > On Wed, Aug 12, 2020 at 06:07:52PM +0530, Vinay Simha B N wrote:
> > > > > > > On Wed, Aug 12, 2020 at 3:24 PM Laurent Pinchart wrote:
> > > > > > > > On Wed, Aug 12, 2020 at 12:55:50PM +0530, Vinay Simha BN wrote:
> > > > > > > > > - bus formats read from
> > drm_bridge_state.output_bus_cfg.format
> > > > > > > > > and .atomic_get_input_bus_fmts() instead of connector
> > > > > > > > >
> > > > > > > > > Signed-off-by: Vinay Simha BN <[email protected]>
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > > v1:
> > > > > > > > > * Laurent Pinchart review comments incorporated
> > > > > > > > > drm_bridge_state.output_bus_cfg.format
> > > > > > > > > instead of connector
> > > > > > > > > ---
> > > > > > > > > drivers/gpu/drm/bridge/tc358775.c | 76
> > ++++++++++++++++++++++++++++++---------
> > > > > > > > > 1 file changed, 59 insertions(+), 17 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358775.c
> > b/drivers/gpu/drm/bridge/tc358775.c
> > > > > > > > > index 7da15cd..5d8714a 100644
> > > > > > > > > --- a/drivers/gpu/drm/bridge/tc358775.c
> > > > > > > > > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > > > > > > > > @@ -271,6 +271,13 @@ struct tc_data {
> > > > > > > > > struct gpio_desc *stby_gpio;
> > > > > > > > > u8 lvds_link; /* single-link or
> > dual-link */
> > > > > > > > > u8 bpc;
> > > > > > > > > + u32 output_bus_fmt;
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +static const u32 tc_lvds_out_bus_fmts[] = {
> > > > > > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > > > > > > > > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > > > > > > > + MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > static inline struct tc_data *bridge_to_tc(struct
> > drm_bridge *b)
> > > > > > > > > @@ -359,19 +366,6 @@ static void d2l_write(struct i2c_client
> > *i2c, u16 addr, u32 val)
> > > > > > > > > ret, addr);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > -/* helper function to access bus_formats */
> > > > > > > > > -static struct drm_connector *get_connector(struct
> > drm_encoder *encoder)
> > > > > > > > > -{
> > > > > > > > > - struct drm_device *dev = encoder->dev;
> > > > > > > > > - struct drm_connector *connector;
> > > > > > > > > -
> > > > > > > > > - list_for_each_entry(connector,
> > &dev->mode_config.connector_list, head)
> > > > > > > > > - if (connector->encoder == encoder)
> > > > > > > > > - return connector;
> > > > > > > > > -
> > > > > > > > > - return NULL;
> > > > > > > > > -}
> > > > > > > > > -
> > > > > > > > > static void tc_bridge_enable(struct drm_bridge *bridge)
> > > > > > > > > {
> > > > > > > > > struct tc_data *tc = bridge_to_tc(bridge);
> > > > > > > > > @@ -380,7 +374,6 @@ static void tc_bridge_enable(struct
> > drm_bridge *bridge)
> > > > > > > > > u32 val = 0;
> > > > > > > > > u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
> > > > > > > > > struct drm_display_mode *mode;
> > > > > > > > > - struct drm_connector *connector =
> > get_connector(bridge->encoder);
> > > > > > > > >
> > > > > > > > > mode = &bridge->encoder->crtc->state->adjusted_mode;
> > > > > > > > >
> > > > > > > > > @@ -451,14 +444,13 @@ static void tc_bridge_enable(struct
> > drm_bridge *bridge)
> > > > > > > > > d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) |
> > LV_PHY0_ND(6));
> > > > > > > > >
> > > > > > > > > dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
> > > > > > > > > - connector->display_info.bus_formats[0],
> > > > > > > > > + tc->output_bus_fmt,
> > > > > > > > > tc->bpc);
> > > > > > > > > /*
> > > > > > > > > * Default hardware register settings of tc358775
> > configured
> > > > > > > > > * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24
> > format
> > > > > > > > > */
> > > > > > > > > - if (connector->display_info.bus_formats[0] ==
> > > > > > > > > - MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > > > > > + if (tc->output_bus_fmt ==
> > MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> > > > > > > > > /* VESA-24 */
> > > > > > > > > d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0,
> > LVI_R1, LVI_R2, LVI_R3));
> > > > > > > > > d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4,
> > LVI_R7, LVI_R5, LVI_G0));
> > > > > > > > > @@ -590,6 +582,51 @@ static int tc358775_parse_dt(struct
> > device_node *np, struct tc_data *tc)
> > > > > > > > > return 0;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static int tc_bridge_atomic_check(struct drm_bridge *bridge,
> > > > > > > > > + struct drm_bridge_state
> > *bridge_state,
> > > > > > > > > + struct drm_crtc_state
> > *crtc_state,
> > > > > > > > > + struct drm_connector_state
> > *conn_state)
> > > > > > > > > +{
> > > > > > > > > + struct tc_data *tc = bridge_to_tc(bridge);
> > > > > > > > > +
> > > > > > > > > + tc->output_bus_fmt =
> > bridge_state->output_bus_cfg.format;
> > > > > > > >
> > > > > > > > .atomic_check() isn't allowed to modify the device state,
> > neither the
> > > > > > > > hardware state nor the software state in drm_bridge or
> > tc_data. You can
> > > > > > > > instead access the bridge state directly in
> > tc_bridge_enable(), with
> > > > > > > >
> > > > > > > > struct drm_bridge_state *state =
> > > > > > > > drm_priv_to_bridge_state(bridge->base.state);
> > > > > > >
> > > > > > > Currently the driver is picking up from the dts panel
> > > > > > > (data-mapping = "vesa-24";) or jeida-24 or jeida-18.
> > > > > > >
> > > > > > > Does state->output_bus_cfg.format get set from the data-mapping?
> > > > > >
> > > > > > It should. The drm_panel should take care of that. In
> > > > > > panel_simple_get_non_edid_modes(), it calls
> > > > > >
> > > > > > if (panel->desc->bus_format)
> > > > > >
> > drm_display_info_set_bus_formats(&connector->display_info,
> > > > > >
> > &panel->desc->bus_format, 1);
> > > > > >
> > > > > > to initialize the bus format in display_info. Then, the DRM bridge
> > > > > > helper drm_atomic_bridge_chain_select_bus_fmts() retrieves the
> > output
> > > > > > format by calling .atomic_get_output_bus_fmts() if implemented by
> > the
> > > > > > last bridge in the chain, or directly from the connector
> > display_info.
> > > > > > The last bridge in the chain is a DRM panel bridge, and doesn't
> > > > > > implement .atomic_get_output_bus_fmts(), so the format from
> > display_info
> > > > > > is used, and is stored in the output_bus_cfg.format field of this
> > bridge
> > > > > > in select_bus_fmt_recursive().
> > > > > >
> > > > > > If something doesn't work according to the plan, I can help you
> > > > > > debugging.
> > > > > >
> > > > > > > > > +
> > > > > > > > > + dev_dbg(tc->dev, "output_bus_fmt %04x\n",
> > tc->output_bus_fmt);
> > > > > > > > > +
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static u32 *
> > > > > > > > > +tc_bridge_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 = NULL;
> > > > > > > > > + int i;
> > > > > > > >
> > > > > > > > i only takes positive values, so it can be an unsigned int.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > + *num_input_fmts = 0;
> > > > > > > > > +
> > > > > > > > > + for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ;
> > ++i) {
> > > > > > > > > + if (output_fmt == tc_lvds_out_bus_fmts[i]) {
> > > > > > > > > + *num_input_fmts = 1;
> > > > > > > > > + input_fmts = kcalloc(*num_input_fmts,
> > > > > > > > > +
> > sizeof(*input_fmts),
> > > > > > > > > + GFP_KERNEL);
> > > > > > > > > + if (!input_fmts)
> > > > > > > > > + return NULL;
> > > > > > > > > +
> > > > > > > > > + input_fmts[0] = output_fmt;
> > > > > > > >
> > > > > > > > I don't think this is right, the input of the bridge isn't
> > LVDS, is it ?
> > > > > > >
> > > > > > > Input to the bridge is DSI, format is already set
> > > > > > >
> > > > > > > dsi->format = MIPI_DSI_FMT_RGB888;
> > > > > > >
> > > > > > > enum mipi_dsi_pixel_format {
> > > > > > > MIPI_DSI_FMT_RGB888,
> > > > > > > MIPI_DSI_FMT_RGB666,
> > > > > > > MIPI_DSI_FMT_RGB666_PACKED,
> > > > > > > MIPI_DSI_FMT_RGB565,
> > > > > > > };
> > > > > > > include/drm/drm_mipi_dsi.h
> > > > > > >
> > > > > > > Why do we require this atomic_get_input_bus_fmts?
> > > > > > >
> > > > > > > Do i need to implement both atomic_get_input_bus_fmts and
> > > > > > > atomic_get_output_bus_fmts?
> > > > > >
> > > > > > .atomic_get_output_bus_fmts() is only need for the last bridge in
> > the
> > > > > > chain, and is not mandatory when that bridge supports a single
> > format.
> > > > > > As this bridge can't be last (if the output is connect to a panel,
> > there
> > > > > > will be a drm_bridge wrapping the drm_panel), you don't have to
> > > > > > implement that operation.
> > > > > >
> > > > > > .atomic_get_input_bus_fmts() is used to negotiate formats along the
> > > > > > pipeline. The helps the DRM bridge helpers figure out what formats
> > are
> > > > > > possible, with the help of bridges that must report what input
> > formats
> > > > > > are compatible with a given output format. The DRM bridge helpers
> > will
> > > > > > take care of the rest.
> > > > > >
> > > > > > So, for this bridge, the input and output formats are decoupled.
> > The
> > > > > > bridge can output any of the three supported LVDS formats,
> > regardless of
> > > > > > what format it gets at its input. You should thus verify that the
> > output
> > > > > > format you receive in this function is supported (and return NULL
> > if it
> > > > > > isn't), and then return the list of supported input formats. If you
> > > > > > don't implement .atomic_get_input_bus_fmts(), then the DRM bridge
> > > > > > helpers will consider that the input and output formats are the
> > same,
> > > > > > and will set the output format of the previous bridge to, for
> > example,
> > > > > > MEDIA_BUS_FMT_RGB666_1X7X3_SPWG. It may work if the previous bridge
> > > > > > doesn't care about its output format, but if it does, then it will
> > be
> > > > > > puzzled, as the previous bridge outputs DSI, not LVDS.
> > > > > >
> > > > > > > > As far as I can tell, the hardware support transcoding any of
> > the
> > > > > > > > supported input formats (RGB565, RGB666 or RGB888) to any of
> > the
> > > > > > > > supported output formats. How about the following ?
> > > > > > > >
> > > > > > > > static const u32 tc_lvds_in_bus_fmts[] = {
> > > > > > > > MEDIA_BUS_FMT_RGB565_1X16,
> > > > > > > > MEDIA_BUS_FMT_RGB666_1X18,
> > > > > > > > MEDIA_BUS_FMT_RBG888_1X24,
> > > > > > > > };
> > > > > > > >
> > > > > > > > ...
> > > > > > > >
> > > > > > > > u32 *input_fmts;
> > > > > > > > unsigned int i;
> > > > > > > >
> > > > > > > > *num_input_fmts = 0;
> > > > > > > >
> > > > > > > > for (i = 0 ; i < ARRAY_SIZE(tc_lvds_out_bus_fmts) ;
> > ++i) {
> > > > > > > > if (output_fmt == tc_lvds_out_bus_fmts[i])
> > > > > > > > break;
> > > > > > > > }
> > > > > > > >
> > > > > > > > if (i == ARRAY_SIZE(tc_lvds_out_bus_fmts))
> > > > > > > > return NULL;
> > > > > > > >
> > > > > > > > input_fmts = kcalloc(*num_input_fmts,
> > ARRAY_SIZE(tc_lvds_in_bus_fmts),
> > > > > > > > GFP_KERNEL);
> > > > > > > > if (!input_fmts)
> > > > > > > > return NULL;
> > > > > > > >
> > > > > > > > for (i = 0; i < ARRAY_SIZE(tc_lvds_in_bus_fmts); ++i)
> > > > > > > > input_fmts[i] = tc_lvds_in_bus_fmts[i];
> > > > > > > >
> > > > > > > > *num_inputs_fmts = ARRAY_SIZE(tc_lvds_in_bus_fmts);
> > > > > > > > return input_fmts;
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > + break;
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + return input_fmts;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > static int tc_bridge_attach(struct drm_bridge *bridge,
> > > > > > > > > enum drm_bridge_attach_flags flags)
> > > > > > > > > {
> > > > > > > > > @@ -639,6 +676,11 @@ static int tc_bridge_attach(struct
> > drm_bridge *bridge,
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > static const struct drm_bridge_funcs tc_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 =
> > tc_bridge_get_input_bus_fmts,
> > > > > > > > > + .atomic_check = tc_bridge_atomic_check,
> > > > > > > > > .attach = tc_bridge_attach,
> > > > > > > > > .pre_enable = tc_bridge_pre_enable,
> > > > > > > > > .enable = tc_bridge_enable,

--
Regards,

Laurent Pinchart

2020-10-17 10:39:04

by Sam Ravnborg

Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read

Hi Vinay

On Tue, Sep 08, 2020 at 08:57:07PM +0300, Laurent Pinchart wrote:
> Hi Vinay,
>
> On Tue, Sep 08, 2020 at 11:22:48PM +0530, Vinay Simha B N wrote:
> > laurent,
> >
> > Please review or give some feedback.
>
> I'm sorry, I have very little time these days :-( Maybe Neil can provide
> feedback ?

I have lost the original patch - if this is still pending could you then
please resend.

Thanks,

Sam