From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 17BC7C433DF for ; Wed, 12 Aug 2020 17:15:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D98EC20675 for ; Wed, 12 Aug 2020 17:15:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="LFi2sEFi" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726573AbgHLRPy (ORCPT ); Wed, 12 Aug 2020 13:15:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43932 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726456AbgHLRPx (ORCPT ); Wed, 12 Aug 2020 13:15:53 -0400 Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4F08CC061383 for ; Wed, 12 Aug 2020 10:15:53 -0700 (PDT) Received: by mail-io1-xd42.google.com with SMTP id v6so3593837iow.11 for ; Wed, 12 Aug 2020 10:15:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4FHPwIDokTxaV01WqdBixhHARN/0PwLwli+q7UO4rYQ=; b=LFi2sEFi7HoDKEDbKjtKUvMmpAVwD3RZAlhQQxtjdZlRr9uaMcyk9uzMr6hDBWPFYS LKh6Iyni4oTGtKnCLVYBnDe9M3DVmimVgdXyA25VX6eTIDPeUOi+qQV/IOvmyN9BAi/n eH926zlEVPVNhUykbFoSvSkV57Y6GoE7ftrJcPKkqbeuf2v7iR+5pkD30aqXQ5be44hq xbm/cWY7SZKpdeCSEMgT8nF8iDYOyD3HMn3ldJGPScodjCfS0rgN6cW6elVRRmHZMIUh tEcAPjQy2e/zKkDezCVEN7/SyN+ApT9HKacIULXmgZmaUuXhCc4FQl4Sr/ugjCTBCVjY dE4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4FHPwIDokTxaV01WqdBixhHARN/0PwLwli+q7UO4rYQ=; b=V9Oz86F6UMcB+WqdlWDnqeAk8EXH9IDynOEmMqkU3nGUE6fjlLWyEy+j4vzQjUwqHt Sh5+s8t9XDgKQQq7r9t3m/WuzpjVe5RHXYZ9SKNAuct+r4+MYu/VPY295kNoPOC/pde6 taGJSQjKo/8xVLCoG+rOWSsrc1MlbaDhn4UdbA/LrfSKAYxiG74eD8/IZ99eVx6lTdrv 2daNnnIT7Uy0rBwCdxUwTE0+v3jlLRD9EX4pKkur21IKrl//v7gPMTHXbsYc+3aEn8dX aB6SkBv06odMBZkGMQXweq+zV9e/gPgvS9KAFGoXLCPjX11Wnp8HItMXUJLDKczqr/a1 FZBQ== X-Gm-Message-State: AOAM530APTYkSy9qhNCMWfTgJclFtvPkpz/z0/EEZdM8xPveD5G2y8WU VIj1ZJrvMbiyx8iRbS72lFcOAJEyN5qXFxgoabM= X-Google-Smtp-Source: ABdhPJzZJ+wkwSRVJFSSsAhzXMyiwZ1JniTfmBYMAjd7g0Peah0qCY9sKBQb0K7WMrnmUxjqhH8Hl66dKjnpOZ9UgFA= X-Received: by 2002:a6b:e31a:: with SMTP id u26mr781847ioc.162.1597252552271; Wed, 12 Aug 2020 10:15:52 -0700 (PDT) MIME-Version: 1.0 References: <1597217150-22911-1-git-send-email-simhavcs@gmail.com> <20200812095418.GG6057@pendragon.ideasonboard.com> <20200812131818.GI6057@pendragon.ideasonboard.com> In-Reply-To: From: Vinay Simha B N Date: Wed, 12 Aug 2020 22:45:40 +0530 Message-ID: Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read To: Laurent Pinchart Cc: Andrzej Hajda , Neil Armstrong , Jonas Karlman , Jernej Skrabec , David Airlie , Daniel Vetter , "open list:DRM DRIVERS" , open list Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 > 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 > > > > > > > > > > --- > > > > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0AE74C433DF for ; Thu, 13 Aug 2020 07:13:02 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C69E420716 for ; Thu, 13 Aug 2020 07:13:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="LFi2sEFi" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C69E420716 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 10B9A6E483; Thu, 13 Aug 2020 07:12:34 +0000 (UTC) Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4A3E96E8F3 for ; Wed, 12 Aug 2020 17:15:53 +0000 (UTC) Received: by mail-io1-xd42.google.com with SMTP id b17so3656569ion.7 for ; Wed, 12 Aug 2020 10:15:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4FHPwIDokTxaV01WqdBixhHARN/0PwLwli+q7UO4rYQ=; b=LFi2sEFi7HoDKEDbKjtKUvMmpAVwD3RZAlhQQxtjdZlRr9uaMcyk9uzMr6hDBWPFYS LKh6Iyni4oTGtKnCLVYBnDe9M3DVmimVgdXyA25VX6eTIDPeUOi+qQV/IOvmyN9BAi/n eH926zlEVPVNhUykbFoSvSkV57Y6GoE7ftrJcPKkqbeuf2v7iR+5pkD30aqXQ5be44hq xbm/cWY7SZKpdeCSEMgT8nF8iDYOyD3HMn3ldJGPScodjCfS0rgN6cW6elVRRmHZMIUh tEcAPjQy2e/zKkDezCVEN7/SyN+ApT9HKacIULXmgZmaUuXhCc4FQl4Sr/ugjCTBCVjY dE4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4FHPwIDokTxaV01WqdBixhHARN/0PwLwli+q7UO4rYQ=; b=lYTeh/Y1FhQI96FBawgkLGg7ADE7iY6o2lM7J/vyf+UytAigcKrn+kqSgapvRr1L2U L21j6l/j+t+Ozl9vBzsHn8tmPsxVGh2rX308c1ksGjRrd43haYuo/+DYW/JUjaAhmlPZ eQfKdq2kpbGoEUxTg4erMDVC7qBCgdnK/y5Q2bbalQWsNUv6ZkAftXZA0zEqqs7ypL3/ nkxF0/SScwAAQJIseDjzioDCOqi5DUH/TsugI2g3Rdu/Xz+4KoIJ3Zom8veugB/0ZUF7 tPGTcQ1E/EpOm5ZUDotgmyjux+x3nWeirz0WF1RQHlrA14RGfH9Y6ctGpgJhsdtTwaqx OdKw== X-Gm-Message-State: AOAM533H+zl4xXqDk5/SwwaMe2sbTThweV3CnW5e3TzT1/+tD7ZWIOOK 6Tu86TFyS3SGLyRu7QxMbdBama3lkPEhxUoECEI= X-Google-Smtp-Source: ABdhPJzZJ+wkwSRVJFSSsAhzXMyiwZ1JniTfmBYMAjd7g0Peah0qCY9sKBQb0K7WMrnmUxjqhH8Hl66dKjnpOZ9UgFA= X-Received: by 2002:a6b:e31a:: with SMTP id u26mr781847ioc.162.1597252552271; Wed, 12 Aug 2020 10:15:52 -0700 (PDT) MIME-Version: 1.0 References: <1597217150-22911-1-git-send-email-simhavcs@gmail.com> <20200812095418.GG6057@pendragon.ideasonboard.com> <20200812131818.GI6057@pendragon.ideasonboard.com> In-Reply-To: From: Vinay Simha B N Date: Wed, 12 Aug 2020 22:45:40 +0530 Message-ID: Subject: Re: [PATCH] drm/bridge/tc358775: Fixes bus formats read To: Laurent Pinchart X-Mailman-Approved-At: Thu, 13 Aug 2020 07:12:31 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jernej Skrabec , Jonas Karlman , David Airlie , Neil Armstrong , open list , "open list:DRM DRIVERS" , Andrzej Hajda Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 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 > 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 > > > > > > > > > > --- > > > > > 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel