Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 12 Aug 2020 16:18:18 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Vinay Simha B N <simhavcs@gmail.com>
Cc:     Andrzej Hajda <a.hajda@samsung.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        "open list:DRM DRIVERS" <dri-devel@lists.freedesktop.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read
Message-ID: <20200812131818.GI6057@pendragon.ideasonboard.com>
References: <1597217150-22911-1-git-send-email-simhavcs@gmail.com>
 <20200812095418.GG6057@pendragon.ideasonboard.com>
 <CAGWqDJ4i=t4Noi7wjGDDhUYkB_uuQ6A-WiMrh1ErKRi2HU9t9w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAGWqDJ4i=t4Noi7wjGDDhUYkB_uuQ6A-WiMrh1ErKRi2HU9t9w@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 <simhavcs@gmail.com>
> > >
> > > ---
> > >  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