From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 76FE2EBE for ; Fri, 26 Apr 2024 03:03:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714100597; cv=none; b=K/VZ0FgHoNsWhyPrEdd7UVjAMQylommDWRqI4tXg307LQloc7Xr6UJ9gu/b1fuAIKT6V3y5TA69f8xp8e/E7ag/26FV0p6UdN95H8H/nSePbPg7rTOHyx+AVDaM+vSQ4e3fmdWgJ7vdya63XpYNB2K8fldp+PxmWcCif5yE/CuM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714100597; c=relaxed/simple; bh=vsLI7TVcpwAvcajVp83qswpofMP2+9uWtTzz/ql0IgM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=AM+j4jvQTx8TlX1+gx4+qUAKW0U5CdeWXeGK7VyRoFSB2qzIbgeDUF7iERNm5otZLXOFc8JOzoD1ZhZF8iW3tW3CmemBpd4Rz/IY4tCO3VvxTyK+dv5KNxPA1nA59HzzU58tZJDdJScwabwEL7ynvvDe3DA+RYM3zmiTQaHmLlI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=oiYO3qrZ; arc=none smtp.client-ip=209.85.167.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="oiYO3qrZ" Received: by mail-lf1-f43.google.com with SMTP id 2adb3069b0e04-518a56cdc03so1901312e87.1 for ; Thu, 25 Apr 2024 20:03:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1714100590; x=1714705390; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=eerUw8HV77CQtEM68opKn2k+UEnTk4gJflML9gefhwE=; b=oiYO3qrZe3e8yCjLyKYMhoTGJ6opNuH1FG6seRyzPt6wLY6CcQxQFq5gLU+00S5H8y P3luXtqve7tPciSuq1UQM+aGWnlGj7jvCz0BhXstBnoX6VBqGfZXyov2dbZdviYrPQ9F K8V5wV69RqItf0zPp3saRvbbxz8R0w7gkULHeyBu5/OipNnO9ksJkNSTVBUtucvmRN5E ZFsdbn7pZS84ONymlKy8Zf/h6pC3IVDelZ7BHfZNMOX1fZvZ0MvGl9QaeP3Svxdv86oc GrTlVcxNutNqBUdJplJC3LYLcQLuWjyqWDbFIw5hUbOWGjNHiOGhRISGUnfc5o8ML/aJ EGlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714100590; x=1714705390; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=eerUw8HV77CQtEM68opKn2k+UEnTk4gJflML9gefhwE=; b=aaDaSPl4LdCsgQkvOpLscQo1ZO5juuOnhuFMq9kMDppAAlkVDjruRuHzUiTnKteTnw BoZO/4xjqYK56rWiIlzBb1fgRT3QX1/XCZWFqK9BV6gpxc5OeFUGxjUjcCaLslmlGYVH IyQiURTwr7vUD/IRbrOBgMH1G1o2QOjQT78VsrE8Qv5VIPJRUxxWHCzksVoALq0YHqKY fH+yNoFIrexP3U2/tTZ4Jl2gaPi7I5q7U/QZY46H9F7cLK8wvDoB9A9pjI4197g4F7/b +a7Qjgwj44Du7vw+vxusJjQGKwFjoNGLcJ90BVHK1ZQHm2SEwhV53H2SmyrdUYAiWRfm ZjDA== X-Forwarded-Encrypted: i=1; AJvYcCVSrApkyH7FTaJYZq0k926BYHKpDtUz/J5npuXpjO98e8o8zN7wqXPa98LoF+/38zfNnkKDKfFdnfKzvNIptDsANKMsQkj77urJpiip X-Gm-Message-State: AOJu0YyUac0Gz7EZH70BHB2YzA+gTfFWTPtNECmpYA3ArnnvFco2+g4o v7WxmeUIoP33KWKWMoOjw+3xEZMpTa3UvWA1qnlRBaQRd4r5a01M39mNaBJHCoA= X-Google-Smtp-Source: AGHT+IF7fC3rK4d2pUEe78+3175lW5ei7plG847JDb4Ai7SjzrmEwSTqyN+4QmSXUWusfc3etJMfTQ== X-Received: by 2002:ac2:4e03:0:b0:516:cf0a:9799 with SMTP id e3-20020ac24e03000000b00516cf0a9799mr800810lfr.64.1714100589585; Thu, 25 Apr 2024 20:03:09 -0700 (PDT) Received: from eriador.lumag.spb.ru (dzdbxzyyyyyyyyyyybcwt-3.rev.dnainternet.fi. [2001:14ba:a0c3:3a00::8a5]) by smtp.gmail.com with ESMTPSA id a13-20020ac2520d000000b00516b07d95c0sm2980341lfl.217.2024.04.25.20.03.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Apr 2024 20:03:08 -0700 (PDT) Date: Fri, 26 Apr 2024 06:03:07 +0300 From: Dmitry Baryshkov To: Doug Anderson Cc: Jani Nikula , dri-devel@lists.freedesktop.org, Javier Martinez Canillas , Neil Armstrong , linus.walleij@linaro.org, Cong Yang , lvzhaoxiong@huaqin.corp-partner.google.com, Hsin-Yi Wang , Sam Ravnborg , Daniel Vetter , David Airlie , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq() Message-ID: References: <20240424172017.1.Id15fae80582bc74a0d4f1338987fa375738f45b9@changeid> <87pludq2g0.fsf@intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Thu, Apr 25, 2024 at 10:04:49AM -0700, Doug Anderson wrote: > Hi, > > On Thu, Apr 25, 2024 at 1:19 AM Jani Nikula wrote: > > > > > @@ -279,6 +281,8 @@ enum mipi_dsi_dcs_tear_mode { > > > > > > ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, > > > const void *data, size_t len); > > > +ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi, > > > + const void *data, size_t len); > > > ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd, > > > const void *data, size_t len); > > > ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data, > > > @@ -317,14 +321,10 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, > > > #define mipi_dsi_generic_write_seq(dsi, seq...) \ > > > do { \ > > > static const u8 d[] = { seq }; \ > > > - struct device *dev = &dsi->dev; \ > > > int ret; \ > > > - ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d)); \ > > > - if (ret < 0) { \ > > > - dev_err_ratelimited(dev, "transmit data failed: %d\n", \ > > > - ret); \ > > > + ret = mipi_dsi_generic_write_chatty(dsi, d, ARRAY_SIZE(d)); \ > > > + if (ret < 0) \ > > > return ret; \ > > > - } \ > > > } while (0) Reading the thread makes me wonder whether we should be going into slightly other direction: Add __must_check() to mipi_dsi_ writing functions, #define mipi_dsi_dcs_whatever_write(dsi, cmd, seq...) \ ({ \ static const u8 d[] = { cmd, seq }; \ mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ }) Then in panel drivers we actually have to explicitly handle the return code (either by dropping to the error label or by just returning an error). > > > > The one thing that I've always disliked about these macros (even if I've > > never actually used them myself) is that they hide control flow from the > > caller, i.e. return directly. You don't see that in the code, it's not > > documented, and if you wanted to do better error handling yourself, > > you're out of luck. > > Yeah, I agree that it's not the cleanest. That being said, it is > existing code and making the existing code less bloated seems worth > doing. > > I'd also say that it feels worth it to have _some_ solution so that > the caller doesn't need to write error handling after every single cmd > sent. If we get rid of / discourage these macros that's either going > to end us up with ugly/verbose code or it's going to encourage people > to totally skip error handling. IMO neither of those are wonderful > solutions. > > While thinking about this there were a few ideas I came up with. None > of them are amazing, but probably they are better than the hidden > "return" like this. Perhaps we could mark the current function as > "deprecated" and pick one of these depending on what others opinions > are: > > 1. Use "goto" and force the caller to give a goto target for error handling. > > This is based on an idea that Dmitry came up with, but made a little > more explicit. Example usage: > > int ret; > > ret = 0; > mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETSPCCMD, 0xcd, > some_cmd_failed); > mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETMIPI, 0x84, > some_cmd_failed); > mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETSPCCMD, 0x3f, > some_cmd_failed); > mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETVDC, 0x1b, 0x04, > some_cmd_failed); > > ... > > some_cmd_failed: > pr_err("Commands failed to write: %d", ret); > return ret; > } > > One downside here is that you can't easily tell which command failed > to put it in the error message. A variant of this idea (1a?) could be > to hoist the print back into the write command. I'd want to pick one > or the other. I guess my preference would be to hoist the print into > the write command and if someone really doesn't want the print then > they call mipi_dsi_dcs_write_buffer() directly. Do we really care, which command has failed? I mean, usually either all DSI transfers work, or we have an issue with the DSI host. > > --- > > 2. Accept that a slightly less efficient handling of the error case > and perhaps a less intuitive API, but avoid the goto. > > Essentially you could pass in "ret" and have the function be a no-op > if an error is already present. Something like this: > > void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, > const void *data, size_t len, int *accum_ret) > { > if (*accum_ret) > return; > > *accum_ret = mipi_dsi_dcs_write_buffer(dsi, data, len); > } > > ...and then the caller: > > int ret; > > ret = 0; > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0xcd, &ret); > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETMIPI, 0x84, &ret); > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0x3f, &ret); > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETVDC, 0x1b, 0x04, &ret); > if (ret) > goto some_cmd_failed; > > This has similar properties to solution #1. > > --- > > 3. Accept that callers don't want to error handling but just need a print. > > I'm not 100% sure we want to encourage this. On the one hand it's > unlikely anyone is really going to be able to reliably recover super > properly from an error midway through a big long command sequence. On > the other hand, this means we can't pass the error back to the caller. > In theory the caller _could_ try to handle errors by resetting / power > cycling things, so that's a real downside. > > Example usage: > > mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETSPCCMD, 0xcd); > mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETMIPI, 0x84); > mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETSPCCMD, 0x3f); > mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETVDC, 0x1b, 0x04); > > --- > > I think I'd lean towards #1a (user passes goto label and we include > the error print in the helper), but I'd personally be happy with any > of #1 or #2. I don't love #3. > > > Be that as it may, the combo of ratelimited error printing and return on > > errors does not make much sense to me. If there's something to print, > > you bail out, that's it. I suspect we never hit the ratelimit. > > Yeah, I'm in favor of removing the ratelimit. > > > > You might even want to try *only* changing the ratelimited printing to a > > regular error message, and see if the compiler can combine the logging > > to a single exit point in the callers. Ratelimited it obviously can't > > because every single one of them is unique. > > It wasn't quite as good. Comparing the "after" solution (AKA applying > $SUBJECT patch) vs. _not_ taking $SUBJECT patch and instead changing > dev_err_ratelimited() to dev_err(). > > $ scripts/bloat-o-meter \ > .../after/panel-novatek-nt36672e.ko \ > .../noratelimit/panel-novatek-nt36672e.ko > add/remove: 0/0 grow/shrink: 1/0 up/down: 3404/0 (3404) > Function old new delta > nt36672e_1080x2408_60hz_init 7260 10664 +3404 > Total: Before=11669, After=15073, chg +29.17% > > ...so $SUBJECT patch is still better. > > --- > > Where does that leave us? IMO: > > a) If others agree, we should land $SUBJECT patch. It doesn't change > the behavior at all and gives big savings. It adds an extra function > hop, but presumably the fact that we have to fetch _a lot_ less stuff > from RAM might mean we still get better performance (likely it doesn't > matter anyway since this is not hotpath code). > > b) Atop this patch, we should consider changing dev_err_ratelimited() > to dev_err(). It doesn't seem to make lots of sense to me to ratelimit > this error. > > c) Atop this patch, we should consider making the two existing macros > "deprecated" in favor of a new macro that makes the control flow more > obvious. > > How does that sound to folks? > > -Doug -- With best wishes Dmitry