dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>,
	Jonas Karlman <jonas@kwiboo.se>,
	Neil Armstrong <narmstrong@baylibre.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Stephen Boyd <swboyd@chromium.org>,
	linux-renesas-soc@vger.kernel.org,
	Andrzej Hajda <a.hajda@samsung.com>
Subject: Re: [RFC PATCH 04/11] drm/bridge: ti-sn65dsi86: Use bitmask to store valid rates
Date: Tue, 23 Mar 2021 14:08:55 -0700	[thread overview]
Message-ID: <CAD=FV=UDd9LC-sMEk0hn10roeM+Cz6VNekcZomkQXLhfw0-4wA@mail.gmail.com> (raw)
In-Reply-To: <20210322030128.2283-5-laurent.pinchart+renesas@ideasonboard.com>

Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> The valid rates are stored in an array of 8 booleans. Replace it with a
> bitmask to save space.

I'm curious: do you have evidence that this does anything useful? I
guess you're expecting it to save .text space, right? Stack usage and
execution time differences should be irrelevant--it's not in a
critical section and the difference should be tiny anyway. As far as
.text segment goes, it's not obvious to me that the compiler will use
fewer instructions to manipulate bits compared to booleans.

Doing a super simple "ls -ah" on vmlinux (unstripped):

Before: 224820232 bytes
After: 224820376 bytes

...so your change made it _bigger_.   OK, so running "strip
--strip-debug" on those:

Before: 26599464 bytes
After: 26599464 bytes

...so exactly the same. I tried finding some evidence using "readelf -ah":

Before:
  [ 2] .text             PROGBITS         ffffffc010010000  00020000
       0000000000b03508  0000000000000000 WAX       0     0     65536
  [ 3] .rodata           PROGBITS         ffffffc010b20000  00b30000
       00000000002e84b3  0000000000000000 WAMS       0     0     4096

After:
  [ 2] .text             PROGBITS         ffffffc010010000  00020000
       0000000000b03508  0000000000000000 WAX       0     0     65536
  [ 3] .rodata           PROGBITS         ffffffc010b20000  00b30000
       00000000002e84b3  0000000000000000 WAMS       0     0     4096

Maybe you have some evidence showing an improvement? Ah, OK. I
disassembled ti_sn_bridge_enable() and your patch saves 12 bytes, but
I guess maybe alignment washes it out in reality...


In terms of readability / conventions, I personally find this change a
bit of a wash. I mean, I guess I originally implemented it as an array
and I thought that was the most readable, but I like bitfields fine
too. If everyone loves it then I won't object, but to me it feels like
touching lines of code for something that's personal preference. ;-)


> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c45420a50e73..1d1be791d5ba 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -557,9 +557,9 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
>         return i;
>  }
>
> -static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,
> -                                         bool rate_valid[])
> +static unsigned int ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata)
>  {
> +       unsigned int valid_rates = 0;
>         unsigned int rate_per_200khz;
>         unsigned int rate_mhz;
>         u8 dpcd_val;
> @@ -599,13 +599,13 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,
>                              j < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
>                              j++) {
>                                 if (ti_sn_bridge_dp_rate_lut[j] == rate_mhz)
> -                                       rate_valid[j] = true;
> +                                       valid_rates |= BIT(j);
>                         }
>                 }
>
>                 for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); i++) {
> -                       if (rate_valid[i])
> -                               return;
> +                       if (valid_rates & BIT(i))
> +                               return valid_rates;
>                 }
>                 DRM_DEV_ERROR(pdata->dev,
>                               "No matching eDP rates in table; falling back\n");
> @@ -627,15 +627,17 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,
>                               (int)dpcd_val);
>                 fallthrough;
>         case DP_LINK_BW_5_4:
> -               rate_valid[7] = 1;
> +               valid_rates |= BIT(7);
>                 fallthrough;
>         case DP_LINK_BW_2_7:
> -               rate_valid[4] = 1;
> +               valid_rates |= BIT(4);
>                 fallthrough;
>         case DP_LINK_BW_1_62:
> -               rate_valid[1] = 1;
> +               valid_rates |= BIT(1);
>                 break;
>         }
> +
> +       return valid_rates;
>  }
>
>  static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
> @@ -753,8 +755,8 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
>  static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>  {
>         struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -       bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)] = { };
>         const char *last_err_str = "No supported DP rate";
> +       unsigned int valid_rates;
>         int dp_rate_idx;
>         unsigned int val;
>         int ret = -EINVAL;
> @@ -793,13 +795,13 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>         regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
>                            val);
>
> -       ti_sn_bridge_read_valid_rates(pdata, rate_valid);
> +       valid_rates = ti_sn_bridge_read_valid_rates(pdata);
>
>         /* Train until we run out of rates */
>         for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
>              dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
>              dp_rate_idx++) {
> -               if (!rate_valid[dp_rate_idx])
> +               if (!(valid_rates & BIT(dp_rate_idx)))
>                         continue;
>
>                 ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);

In any case, since it does save 12 bytes:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  parent reply	other threads:[~2021-03-23 21:09 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-22  3:01 [RFC PATCH 00/11] drm/bridge: ti-sn65dsi86: Support DisplayPort mode Laurent Pinchart
2021-03-22  3:01 ` [RFC PATCH 01/11] dt-bindings: drm/bridge: ti-sn65dsi8: Make enable GPIO optional Laurent Pinchart
2021-03-22 10:29   ` Jagan Teki
2021-03-23  7:10   ` Stephen Boyd
2021-03-23 21:08   ` Doug Anderson
2021-03-27 16:42   ` Rob Herring
2021-03-22  3:01 ` [RFC PATCH 02/11] drm/bridge: ti-sn65dsi86: " Laurent Pinchart
2021-03-22 10:29   ` Jagan Teki
2021-03-23  7:10   ` Stephen Boyd
2021-03-23 21:08   ` Doug Anderson
2021-03-22  3:01 ` [RFC PATCH 03/11] drm/bridge: ti-sn65dsi86: Unregister AUX adapter in remove() Laurent Pinchart
2021-03-23  7:10   ` Stephen Boyd
2021-03-23 21:08   ` Doug Anderson
2021-03-23 21:41     ` Laurent Pinchart
2021-03-23 22:55       ` Doug Anderson
2021-03-23 23:02         ` Laurent Pinchart
2021-03-26  0:43           ` Doug Anderson
2021-03-26  1:01             ` Laurent Pinchart
2021-03-22  3:01 ` [RFC PATCH 04/11] drm/bridge: ti-sn65dsi86: Use bitmask to store valid rates Laurent Pinchart
2021-03-23  7:11   ` Stephen Boyd
2021-03-23 21:08   ` Doug Anderson [this message]
2021-03-23 21:45     ` Laurent Pinchart
2021-03-23 22:45       ` Doug Anderson
2021-03-24  8:47     ` Geert Uytterhoeven
2021-03-22  3:01 ` [RFC PATCH 05/11] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge Laurent Pinchart
2021-03-22 10:19   ` Jagan Teki
2021-03-23  7:14   ` Stephen Boyd
2021-03-23 21:50     ` Laurent Pinchart
2021-03-24 22:44   ` Doug Anderson
2021-03-26  1:06     ` Laurent Pinchart
2021-03-22  3:01 ` [RFC PATCH 06/11] drm/bridge: ti-sn65dsi86: Group code in sections Laurent Pinchart
2021-03-23  7:14   ` Stephen Boyd
2021-03-24 22:44   ` Doug Anderson
2021-03-22  3:01 ` [RFC PATCH 07/11] drm/bridge: ti-sn65dsi86: Split connector creation to a function Laurent Pinchart
2021-03-23  7:15   ` Stephen Boyd
2021-03-24 22:44   ` Doug Anderson
2021-03-22  3:01 ` [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations Laurent Pinchart
2021-03-23  7:15   ` Stephen Boyd
2021-03-24 22:46   ` Doug Anderson
2021-03-26  1:40     ` Laurent Pinchart
2021-03-22  3:01 ` [RFC PATCH 09/11] drm/bridge: ti-sn65dsi86: Make connector creation optional Laurent Pinchart
2021-03-22  3:01 ` [RFC PATCH 10/11] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode Laurent Pinchart
2021-03-24 22:47   ` Doug Anderson
2021-06-23 13:59     ` Laurent Pinchart
2022-02-23 18:04       ` Kieran Bingham
2022-02-23 18:20         ` Doug Anderson
2022-03-04 15:49           ` Laurent Pinchart
2021-03-22  3:01 ` [RFC PATCH 11/11] drm/bridge: ti-sn65dsi86: Support hotplug detection Laurent Pinchart
2021-03-23  7:21   ` Stephen Boyd
2021-03-24 22:47   ` Doug Anderson
2021-06-23 23:25     ` Laurent Pinchart
2021-06-23 23:51       ` Doug Anderson
2022-02-23 17:43         ` Kieran Bingham
2022-02-23 18:25           ` Doug Anderson
2022-03-04 15:45             ` Kieran Bingham
2022-03-04 16:30               ` Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD=FV=UDd9LC-sMEk0hn10roeM+Cz6VNekcZomkQXLhfw0-4wA@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=a.hajda@samsung.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=laurent.pinchart+renesas@ideasonboard.com \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=narmstrong@baylibre.com \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).