From: Gustavo Sousa <gustavo.sousa@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
<intel-gfx@lists.freedesktop.org>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Move hotplug inversion logic into separate helper
Date: Tue, 20 Sep 2022 14:04:33 -0300 [thread overview]
Message-ID: <20220920170433.yacfjnsrtz32ggim@gjsousa-mobl2> (raw)
In-Reply-To: <87v8pio6li.fsf@intel.com>
Hi, Jani.
On Tue, Sep 20, 2022 at 10:19:53AM +0300, Jani Nikula wrote:
> On Mon, 19 Sep 2022, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> > Make the code more readable, which will be more apparent as new
> > platforms with different hotplug inversion needs are added.
> >
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 25 ++++++++++++++++---------
> > 1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index de06f293e173..c53d21ae197f 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3263,6 +3263,21 @@ static void cherryview_irq_reset(struct drm_i915_private *dev_priv)
> > spin_unlock_irq(&dev_priv->irq_lock);
> > }
> >
> > +static void setup_hotplug_inversion(struct drm_i915_private *dev_priv)
> > +{
> > + u32 invert_bits;
> > +
> > + if (HAS_PCH_DG1(dev_priv))
> > + invert_bits = INVERT_DDIA_HPD |
> > + INVERT_DDIB_HPD |
> > + INVERT_DDIC_HPD |
> > + INVERT_DDID_HPD;
>
> Nitpick, the indentation will be off compared to automated indentation.
What do you mean by automated indentation?
>
> > + else
> > + return;
> > +
> > + intel_uncore_rmw(&dev_priv->uncore, SOUTH_CHICKEN1, 0, invert_bits);
> > +}
> > +
> > static u32 ibx_hotplug_enables(struct drm_i915_private *i915,
> > enum hpd_pin pin)
> > {
> > @@ -3413,15 +3428,7 @@ static u32 gen11_hotplug_enables(struct drm_i915_private *i915,
> >
> > static void dg1_hpd_irq_setup(struct drm_i915_private *dev_priv)
> > {
> > - u32 val;
> > -
> > - val = intel_uncore_read(&dev_priv->uncore, SOUTH_CHICKEN1);
> > - val |= (INVERT_DDIA_HPD |
> > - INVERT_DDIB_HPD |
> > - INVERT_DDIC_HPD |
> > - INVERT_DDID_HPD);
> > - intel_uncore_write(&dev_priv->uncore, SOUTH_CHICKEN1, val);
> > -
> > + setup_hotplug_inversion(dev_priv);
>
> Since you're already in a platform specific function here, seems a bit
> odd to call a new generic function that needs to have another if ladder
> platform check. What are we gaining here? The end result is
> de-duplicating just one line of intel_uncore_rmw(). I'm not convinced.
It is true that the proposed refactor repeats a platform check, but the proposed
refactor has its benefits. As more platforms with hotplug inversion needs are
added (e.g. MTL), we will have a common place for the logic of hotplug
inversion. That arguably makes the code more readable and makes future refactors
easier when we need split a function that has become too complex due to platform
checks.
To make that last point clearer, I am quoting Lucas' (copied here as well)
comment (which was what convinced me) from a discussion regarding the advantage
of using such a helper:
that is what helpers are for, so you don't have to open code it in every
platform-fork of the function that needs it. See how the various
"Sequences to initialize display" are done in the driver... When we are
extending it to a future platform, if the change is small enough we just
add e few if/else in the same function. But it doesn't take too long for
those functions to become unreadable if there are several branches the
code path may take. So then we "fork" the function for a new platform,
but reuse the helpers doing the individual steps.
--
Gustavo Sousa
next prev parent reply other threads:[~2022-09-20 17:04 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-19 14:56 [Intel-gfx] [PATCH] drm/i915: Move hotplug inversion logic into separate helper Gustavo Sousa
2022-09-19 22:58 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2022-09-20 6:47 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2022-09-20 7:01 ` [Intel-gfx] [PATCH] " Lucas De Marchi
2022-09-20 16:27 ` Gustavo Sousa
2022-09-21 9:59 ` Jani Nikula
2022-09-20 7:19 ` Jani Nikula
2022-09-20 17:04 ` Gustavo Sousa [this message]
2022-09-20 17:56 ` Lucas De Marchi
2022-09-21 10:23 ` Jani Nikula
2022-09-21 8:21 ` Jani Nikula
2022-09-21 12:05 ` Gustavo Sousa
2022-09-22 8:17 ` Jani Nikula
2022-09-22 10:53 ` Gustavo Sousa
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=20220920170433.yacfjnsrtz32ggim@gjsousa-mobl2 \
--to=gustavo.sousa@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=lucas.demarchi@intel.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.