Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote:
> On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
> > > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:

...

> > > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
> > > need to fork the __GENMASK() implementation on the 2 sides of the ifdef
> > > since I think the GENMASK_INPUT_CHECK() should be the one covering the
> > > input checks. However to make it common we'd need to solve 2 problems:
> > > the casts and the sizeof. The sizeof can be passed as arg to
> > > __GENMASK(), however the casts I think would need a __CAST_U8(x)
> > > or the like and sprinkle it everywhere, which would hurt readability.
> > > Not pretty. Or go back to the original submission and make it less
> > > horrible :-/
> > 
> > I'm wondering if we can use _Generic() approach here.
> 
> in assembly?

Yes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
> On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
> > > On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> > > > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi 
> > > > >  wrote:

...

> I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
> need to fork the __GENMASK() implementation on the 2 sides of the ifdef
> since I think the GENMASK_INPUT_CHECK() should be the one covering the
> input checks. However to make it common we'd need to solve 2 problems:
> the casts and the sizeof. The sizeof can be passed as arg to
> __GENMASK(), however the casts I think would need a __CAST_U8(x)
> or the like and sprinkle it everywhere, which would hurt readability.
> Not pretty. Or go back to the original submission and make it less
> horrible :-/

I'm wondering if we can use _Generic() approach here.

...

> > #define GENMASK_INPUT_CHECK(h, l) 0
> > +#define __GENMASK(t, h, l) \
> > +   ((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h
> 
> humn... this builds, but does it work if GENMASK_ULL() is used in
> assembly? That BITS_PER_LONG does not match the type width.

UL()/ULL() macros are not just for fun.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/12] drm/i915: Indicate which pipe failed the fastset check overall

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 05:35:51PM +0200, Jani Nikula wrote:
> On Mon, 26 Feb 2024, Andy Shevchenko  
> wrote:
> > On Mon, Feb 26, 2024 at 04:57:58PM +0200, Jani Nikula wrote:
> >> On Fri, 23 Feb 2024, Ville Syrjälä  wrote:
> >> > On Thu, Feb 22, 2024 at 04:46:12PM -0500, Rodrigo Vivi wrote:

...

> >> > I think the proper solution would be to have actually
> >> > sensible conversion specifiers in the format string.
> >> > So instead of % we'd have something
> >> > more like %{drm_crtc} (or whatever color you want to throw
> >> > on that particular bikeshed).
> >> 
> >> Personally I suck at remembering even the standard printf conversion
> >> specifiers, let alone all the kernel extensions. I basically have to
> >> look them up every time. I'd really love some %{name} format for named
> >> pointer things. And indeed preferrably without the %p. Just %{name}.
> >
> > It will become something like %{name[:subextensions]}, where subextensions
> > is what we now have with different letters/numbers after %pX (X is a letter
> > which you proposed to have written as name AFAIU).
> 
> Thanks, I appreciate it, a lot!

Oh, I meant "can" rather than "will".

> But could you perhaps try to go with just clean %{name} only instead of
> adding [:subextensions] right away, please?
> 
> I presume the suggestion comes from an implementation detail, and I
> guess it would be handy to reuse the current implementation for
> subextension.
> 
> For example, %pb -> %{bitmap} and %pbl -> %{bitmap:l}. But really I
> think the better option would be for the latter to become, say,
> %{bitmap-list}. The goal here is to make them easy to remember and
> understand, without resorting to looking up the documentation!

Okay, so it seems you have something in mind, perhaps you can submit a draft
of the list of those "names"?

> >> And then we could discuss adding support for drm specific things. I
> >> guess one downside is that the functions to do this would have to be in
> >> vsprintf.c instead of drm. Unless we add some code in drm for this
> >> that's always built-in.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/12] drm/i915: Indicate which pipe failed the fastset check overall

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 04:57:58PM +0200, Jani Nikula wrote:
> On Fri, 23 Feb 2024, Ville Syrjälä  wrote:
> > On Thu, Feb 22, 2024 at 04:46:12PM -0500, Rodrigo Vivi wrote:

...

> > I think the proper solution would be to have actually
> > sensible conversion specifiers in the format string.
> > So instead of % we'd have something
> > more like %{drm_crtc} (or whatever color you want to throw
> > on that particular bikeshed).
> 
> Personally I suck at remembering even the standard printf conversion
> specifiers, let alone all the kernel extensions. I basically have to
> look them up every time. I'd really love some %{name} format for named
> pointer things. And indeed preferrably without the %p. Just %{name}.

It will become something like %{name[:subextensions]}, where subextensions
is what we now have with different letters/numbers after %pX (X is a letter
which you proposed to have written as name AFAIU).

> And then we could discuss adding support for drm specific things. I
> guess one downside is that the functions to do this would have to be in
> vsprintf.c instead of drm. Unless we add some code in drm for this
> that's always built-in.

-- 
With Best Regards,
Andy Shevchenko

Re: Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:

...

> +#define __GENMASK(t, h, l) \
> + ((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h

What's wrong on using the UL/ULL() macros?

Also it would be really good to avoid bifurcation of the implementations of
__GENMASK() for both cases.

...

> -#define __GENMASK(h, l) \
> - (((~UL(0)) - (UL(1) << (l)) + 1) & \
> -  (~UL(0) >> (BITS_PER_LONG - 1 - (h

This at bare minimum can be left untouched for asm case, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Wed, Feb 21, 2024 at 11:06:10PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote:
> > On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
> >  wrote:

...

> > Excuse me, it seems a c from gitlab didn't work as expected.
> 
> No problem, it's clear that the patch has not been properly tested and
> obviously wrong. Has to be dropped. If somebody wants that, it will
> be material for v6.10 cycle.

...which makes me think that bitmap(bitops) lack of assembly test case(s).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote:
> On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
>  wrote:

...

> Excuse me, it seems a c from gitlab didn't work as expected.

No problem, it's clear that the patch has not been properly tested and
obviously wrong. Has to be dropped. If somebody wants that, it will
be material for v6.10 cycle.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi  wrote:

...

> > +#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE)

Can sizeof() be used in assembly?

...

> > -#define __GENMASK(h, l) \
> > -   (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > -(~UL(0) >> (BITS_PER_LONG - 1 - (h
> > -#define GENMASK(h, l) \
> > -   (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))

> > +#define __GENMASK(t, h, l) \
> > +   (GENMASK_INPUT_CHECK(h, l) + \
> > +(((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > +((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)

Nevertheless, the use ~0ULL is not proper assembly, this broke initial
implementation using UL() / ULL().


> > -#define __GENMASK_ULL(h, l) \
> > -   (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > -(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h
> > -#define GENMASK_ULL(h, l) \
> > -   (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))

Ditto.

> > +#define GENMASK(h, l)  __GENMASK(unsigned long,  h, l)
> > +#define GENMASK_ULL(h, l)  __GENMASK(unsigned long long, h, l)
> > +#define GENMASK_U8(h, l)   __GENMASK(u8,  h, l)
> > +#define GENMASK_U16(h, l)  __GENMASK(u16, h, l)
> > +#define GENMASK_U32(h, l)  __GENMASK(u32, h, l)
> > +#define GENMASK_U64(h, l)  __GENMASK(u64, h, l)
> 
> This breaks drm-tip on arm64 architecture:
> 
> arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
> 465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)'
> 474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)'
> 484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Thu, Feb 08, 2024 at 08:48:59PM +0100, Andi Shyti wrote:
> On Wed, Feb 07, 2024 at 11:45:19PM -0800, Lucas De Marchi wrote:

...

> > Signed-off-by: Yury Norov 
> > Signed-off-by: Lucas De Marchi 
> > Acked-by: Jani Nikula 
> 
> Lucas' SoB should be at the bottom here. In any case, nice patch:

And it's at the bottom (among SoB lines), there is no violation with Submitting
Patches.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH] drm/i915/dsi: Use devm_gpiod_get() for all GPIOs

[Andy Shevchenko]

On Fri, Dec 01, 2023 at 05:11:30PM +0100, Hans de Goede wrote:
> soc_gpio_set_value() already uses devm_gpiod_get(), lets be consistent
> and use devm_gpiod_get() for all GPIOs.
> 
> This allows removing the intel_dsi_vbt_gpio_cleanup() function,
> which only function was to put the GPIO-descriptors.

Fine by me, but I'm not to judge if it is a right approach or not.
In my series I have reused existing call... Anyway, FWIW,
Reviewed-by: Andy Shevchenko 

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v1 1/1] drm/i915/display: Don't use "proxy" headers

[Andy Shevchenko]

The driver uses math.h and not util_macros.h.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_snps_phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c 
b/drivers/gpu/drm/i915/display/intel_snps_phy.c
index ce5a73a4cc89..bc61e736f9b3 100644
--- a/drivers/gpu/drm/i915/display/intel_snps_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c
@@ -3,7 +3,7 @@
  * Copyright © 2019 Intel Corporation
  */
 
-#include 
+#include 
 
 #include "i915_reg.h"
 #include "intel_ddi.h"
-- 
2.43.0.rc1.1.gbec44491f096

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

[Andy Shevchenko]

On Thu, Jan 12, 2017 at 12:23:16PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 11, 2017 at 04:51:16PM -0800, Ben Widawsky wrote:

...

> > +   memcpy(plane->modifiers, format_modifiers,
> > +  format_modifier_count * sizeof(format_modifiers[0]));
> 
> Looks all right since we do the same for formats anyway. But it did
> occur to me (twice at least) that a kmemdup_array() might a nice thing
> to have for things like this. But that's a separate topic.

JFYI:
https://lore.kernel.org/all/20231017052322.2636-2-kkar...@nvidia.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/dsi: 4th attempt to get rid of IOSF GPIO (rev2)

[Andy Shevchenko]

On Wed, Nov 22, 2023 at 12:55:05PM +0200, Jani Nikula wrote:
> On Tue, 21 Nov 2023, "Illipilli, TejasreeX"  
> wrote:
> > Hi ,
> >
> > https://patchwork.freedesktop.org/series/125977/
> 
> Thanks, I guess, but now what? There are no shards results but the
> series is not in the shards queue either [1].
> 
> I don't know what to do.

Tell me if anything I can help with.

To me sounds like CI doesn't like the series because of those checkpatch
warnings... But I'm not familiar at all with that, I might be very well
mistaken.

> [1] https://intel-gfx-ci.01.org/queue/index.html#fullshards-queue

> > -Original Message-
> > From: Jani Nikula  
> > Sent: Thursday, November 16, 2023 10:29 PM
> > To: LGCI Bug Filing ; Andy Shevchenko 
> > 
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/dsi: 4th attempt 
> > to get rid of IOSF GPIO (rev2)
> >
> > On Thu, 16 Nov 2023, Patchwork  wrote:
> >> == Series Details ==
> >>
> >> Series: drm/i915/dsi: 4th attempt to get rid of IOSF GPIO (rev2)
> >> URL   : https://patchwork.freedesktop.org/series/125977/
> >> State : failure
> >>
> >> == Summary ==
> >>
> >> CI Bug Log - changes from CI_DRM_13883 -> Patchwork_125977v2 
> >> 
> >>
> >> Summary
> >> ---
> >>
> >>   **FAILURE**
> >>
> >>   Serious unknown changes coming with Patchwork_125977v2 absolutely need 
> >> to be
> >>   verified manually.
> >>   
> >>   If you think the reported changes have nothing to do with the changes
> >>   introduced in Patchwork_125977v2, please notify your bug team 
> >> (lgci.bug.fil...@intel.com) to allow them
> >>   to document this new failure mode, which will reduce false positives in 
> >> CI.
> >
> > The reported issue is unrelated to the series.
> >
> > Please consider adding
> >
> > Reply-To: lgci.bug.fil...@intel.com
> >
> > message header to these status mails, so the right mail gets added 
> > automatically.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/dsi: 4th attempt to get rid of IOSF GPIO (rev2)

[Andy Shevchenko]

On Wed, Nov 22, 2023 at 07:17:48PM +0200, Jani Nikula wrote:
> On Wed, 22 Nov 2023, "Saarinen, Jani"  wrote:
> >> From: Musial, Ewelina 
> >> Sent: Wednesday, November 22, 2023 4:25 PM

...

> >> But this list had only series which were in queue here https://intel-gfx-
> >> ci.01.org/queue/index.html and today I was checking exactly how queue for
> >> shards is created and there are jobs which are not displayed there.
> >> Directly in Jenkins we do have multiple more jobs than in this queue only
> >> and I also killed them. I was discussing exactly this case today with
> >> Michał and he pointed out that in explanation below queues we do have
> >> highlighted: Due to technical limitation this is just an approximation of
> >> the queue. It is good for assessing the length of the queue, but should
> >> not be considered as completely accurate.

> > OK. So could have been in the list but not sure. 
> 
> Okay, timeout.
> 
> I just pushed the series. I trust Hans' testing here, considering the
> likely platform impact of the series and CI coverage of said platforms.
> 
> Thanks for the patches and review.

Thank you, Jani, Hans, Ville and others!

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [rft, PATCH v4 00/16] drm/i915/dsi: 4th attempt to get rid of IOSF GPIO

[Andy Shevchenko]

On Thu, Nov 16, 2023 at 09:58:06AM +0100, Hans de Goede wrote:
> On 11/3/23 21:18, Andy Shevchenko wrote:
> > DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
> > talking to GPIO IP behind the actual driver's back. A second attempt
> > to fix that is here.
> > 
> > If I understood correctly, my approach should work in the similar way as
> > the current IOSF GPIO.
> > 
> > Hans, I believe you have some devices that use this piece of code,
> > is it possible to give a test run on (one of) them?
> 
> Ok, this now has been testen on both a BYT and a CHT device which
> actually use GPIO controls in their MIPI sequences so this
> series is:
> 
> Tested-by: Hans de Goede 
> 
> And the code of the entire series also looks good to me:
> 
> Reviewed-by: Hans de Goede 
> 
> for the series.

Good news so far, thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [rft, PATCH v4 00/16] drm/i915/dsi: 4th attempt to get rid of IOSF GPIO

[Andy Shevchenko]

On Thu, Nov 16, 2023 at 12:15:03PM +0200, Jani Nikula wrote:
> On Thu, 16 Nov 2023, Hans de Goede  wrote:
> > Ok, this now has been testen on both a BYT and a CHT device which
> > actually use GPIO controls in their MIPI sequences so this
> > series is:
> >
> > Tested-by: Hans de Goede 
> >
> > And the code of the entire series also looks good to me:
> >
> > Reviewed-by: Hans de Goede 
> >
> > for the series.
> 
> Thanks Andy & Hans!
> 
> I'll merge this once the test results are in. The BAT results have been
> a bit flaky recently, so needed to do a rerun.
> 
> That said, I'm not sure if we have any hardware in CI that would
> actually exercise the modifications, so in that sense I trust Hans'
> testing much more.

Thank you!
Should I fix checkpatch warnings CI reported about?

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v4 16/16] drm/i915/iosf: Drop unused APIs

[Andy Shevchenko]

Drop unused vlv_iosf_sb_read() and vlv_iosf_sb_write().

Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/vlv_sideband.c | 17 -
 drivers/gpu/drm/i915/vlv_sideband.h |  3 ---
 2 files changed, 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/vlv_sideband.c 
b/drivers/gpu/drm/i915/vlv_sideband.c
index b98dec3ad817..13b644958e38 100644
--- a/drivers/gpu/drm/i915/vlv_sideband.c
+++ b/drivers/gpu/drm/i915/vlv_sideband.c
@@ -166,23 +166,6 @@ u32 vlv_nc_read(struct drm_i915_private *i915, u8 addr)
return val;
 }
 
-u32 vlv_iosf_sb_read(struct drm_i915_private *i915, u8 port, u32 reg)
-{
-   u32 val = 0;
-
-   vlv_sideband_rw(i915, PCI_DEVFN(0, 0), port,
-   SB_CRRDDA_NP, reg, );
-
-   return val;
-}
-
-void vlv_iosf_sb_write(struct drm_i915_private *i915,
-  u8 port, u32 reg, u32 val)
-{
-   vlv_sideband_rw(i915, PCI_DEVFN(0, 0), port,
-   SB_CRWRDA_NP, reg, );
-}
-
 u32 vlv_cck_read(struct drm_i915_private *i915, u32 reg)
 {
u32 val = 0;
diff --git a/drivers/gpu/drm/i915/vlv_sideband.h 
b/drivers/gpu/drm/i915/vlv_sideband.h
index 9ce283d96b80..8b4495e14bce 100644
--- a/drivers/gpu/drm/i915/vlv_sideband.h
+++ b/drivers/gpu/drm/i915/vlv_sideband.h
@@ -26,9 +26,6 @@ enum {
 };
 
 void vlv_iosf_sb_get(struct drm_i915_private *i915, unsigned long ports);
-u32 vlv_iosf_sb_read(struct drm_i915_private *i915, u8 port, u32 reg);
-void vlv_iosf_sb_write(struct drm_i915_private *i915,
-  u8 port, u32 reg, u32 val);
 void vlv_iosf_sb_put(struct drm_i915_private *i915, unsigned long ports);
 
 static inline void vlv_bunit_get(struct drm_i915_private *i915)
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 07/16] drm/i915/dsi: Get rid of redundant 'else'

[Andy Shevchenko]

In the snippets like the following

if (...)
return / goto / break / continue ...;
else
...

the 'else' is redundant. Get rid of it.

Reviewed-by: Andi Shyti 
Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 58 ++--
 1 file changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 290a112f1b63..4ed5ede9ec5b 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -142,7 +142,7 @@ static enum port intel_dsi_seq_port_to_port(struct 
intel_dsi *intel_dsi,
if (seq_port) {
if (intel_dsi->ports & BIT(PORT_B))
return PORT_B;
-   else if (intel_dsi->ports & BIT(PORT_C))
+   if (intel_dsi->ports & BIT(PORT_C))
return PORT_C;
}
 
@@ -670,8 +670,8 @@ static const char *sequence_name(enum mipi_seq seq_id)
 {
if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
return seq_name[seq_id];
-   else
-   return "(unknown)";
+
+   return "(unknown)";
 }
 
 static void intel_dsi_vbt_exec(struct intel_dsi *intel_dsi,
@@ -865,36 +865,34 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 
panel_id)
 * multiply by 100 to preserve remainder
 */
if (intel_dsi->video_mode == BURST_MODE) {
-   if (mipi_config->target_burst_mode_freq) {
-   u32 bitrate = intel_dsi_bitrate(intel_dsi);
+   u32 bitrate;
 
-   /*
-* Sometimes the VBT contains a slightly lower clock,
-* then the bitrate we have calculated, in this case
-* just replace it with the calculated bitrate.
-*/
-   if (mipi_config->target_burst_mode_freq < bitrate &&
-   intel_fuzzy_clock_check(
-   mipi_config->target_burst_mode_freq,
-   bitrate))
-   mipi_config->target_burst_mode_freq = bitrate;
-
-   if (mipi_config->target_burst_mode_freq < bitrate) {
-   drm_err(_priv->drm,
-   "Burst mode freq is less than 
computed\n");
-   return false;
-   }
-
-   burst_mode_ratio = DIV_ROUND_UP(
-   mipi_config->target_burst_mode_freq * 100,
-   bitrate);
-
-   intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * 
burst_mode_ratio, 100);
-   } else {
-   drm_err(_priv->drm,
-   "Burst mode target is not set\n");
+   if (mipi_config->target_burst_mode_freq == 0) {
+   drm_err(_priv->drm, "Burst mode target is not 
set\n");
return false;
}
+
+   bitrate = intel_dsi_bitrate(intel_dsi);
+
+   /*
+* Sometimes the VBT contains a slightly lower clock, then
+* the bitrate we have calculated, in this case just replace it
+* with the calculated bitrate.
+*/
+   if (mipi_config->target_burst_mode_freq < bitrate &&
+   intel_fuzzy_clock_check(mipi_config->target_burst_mode_freq,
+   bitrate))
+   mipi_config->target_burst_mode_freq = bitrate;
+
+   if (mipi_config->target_burst_mode_freq < bitrate) {
+   drm_err(_priv->drm, "Burst mode freq is less than 
computed\n");
+   return false;
+   }
+
+   burst_mode_ratio =
+   DIV_ROUND_UP(mipi_config->target_burst_mode_freq * 100, 
bitrate);
+
+   intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * 
burst_mode_ratio, 100);
} else
burst_mode_ratio = 100;
 
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 13/16] drm/i915/dsi: Prepare soc_gpio_set_value() to distinguish GPIO communities

[Andy Shevchenko]

Currently soc_gpio_set_value() supports only a single indexing for GPIO pin.
For CHV case, for example, we will need to distinguish community based index
from the one that VBT is using. Introduce an additional parameter to
soc_gpio_set_value() and its callers.

Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 552bc6564d79..b1736c1301ea 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -206,8 +206,8 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
-static void soc_gpio_set_value(struct intel_connector *connector, const char 
*con_id,
-  u8 gpio_index, bool value)
+static void soc_gpio_set_value(struct intel_connector *connector, u8 
gpio_index,
+  const char *con_id, u8 idx, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
/* XXX: this table is a quick ugly hack. */
@@ -217,8 +217,7 @@ static void soc_gpio_set_value(struct intel_connector 
*connector, const char *co
if (gpio_desc) {
gpiod_set_value(gpio_desc, value);
} else {
-   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-con_id, gpio_index,
+   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev, con_id, idx,
 value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
if (IS_ERR(gpio_desc)) {
drm_err(_priv->drm,
@@ -232,8 +231,8 @@ static void soc_gpio_set_value(struct intel_connector 
*connector, const char *co
 }
 
 static void soc_opaque_gpio_set_value(struct intel_connector *connector,
- const char *chip, const char *con_id,
- u8 gpio_index, bool value)
+ u8 gpio_index, const char *chip,
+ const char *con_id, u8 idx, bool value)
 {
struct gpiod_lookup_table *lookup;
 
@@ -243,11 +242,11 @@ static void soc_opaque_gpio_set_value(struct 
intel_connector *connector,
 
lookup->dev_id = ":00:02.0";
lookup->table[0] =
-   GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index, 
GPIO_ACTIVE_HIGH);
+   GPIO_LOOKUP_IDX(chip, idx, con_id, idx, GPIO_ACTIVE_HIGH);
 
gpiod_add_lookup_table(lookup);
 
-   soc_gpio_set_value(connector, con_id, gpio_index, value);
+   soc_gpio_set_value(connector, gpio_index, con_id, idx, value);
 
gpiod_remove_lookup_table(lookup);
kfree(lookup);
@@ -271,7 +270,8 @@ static void vlv_gpio_set_value(struct intel_connector 
*connector,
}
}
 
-   soc_opaque_gpio_set_value(connector, "INT33FC:01", "Panel N", 
gpio_index, value);
+   soc_opaque_gpio_set_value(connector, gpio_index,
+ "INT33FC:01", "Panel N", gpio_index, value);
 }
 
 static void chv_gpio_set_value(struct intel_connector *connector,
@@ -331,7 +331,7 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
 static void bxt_gpio_set_value(struct intel_connector *connector,
   u8 gpio_index, bool value)
 {
-   soc_gpio_set_value(connector, NULL, gpio_index, value);
+   soc_gpio_set_value(connector, gpio_index, NULL, gpio_index, value);
 }
 
 enum {
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 08/16] drm/i915/dsi: Replace check with a (missing) MIPI sequence name

[Andy Shevchenko]

Names of the MIPI sequence steps are sequential and defined, no
need to check for the gaps. However in seq_name the MIPI_SEQ_END
is missing. Add it there, and drop unneeded NULL check in
sequence_name().

Reviewed-by: Andi Shyti 
Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 4ed5ede9ec5b..d270437217b3 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -653,6 +653,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
  */
 
 static const char * const seq_name[] = {
+   [MIPI_SEQ_END] = "MIPI_SEQ_END",
[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
@@ -668,7 +669,7 @@ static const char * const seq_name[] = {
 
 static const char *sequence_name(enum mipi_seq seq_id)
 {
-   if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
+   if (seq_id < ARRAY_SIZE(seq_name))
return seq_name[seq_id];
 
return "(unknown)";
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 14/16] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
cherryview: prevent concurrent access to GPIO controllers") for
the details. Taking all this into consideration replace the hack
with proper GPIO APIs being used.

Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +---
 1 file changed, 10 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index b1736c1301ea..9c6946ccb193 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
 #define CHV_GPIO_IDX_START_SW  100
 #define CHV_GPIO_IDX_START_SE  198
 
-#define CHV_VBT_MAX_PINS_PER_FMLY  15
-
-#define CHV_GPIO_PAD_CFG0(f, i)(0x4400 + (f) * 0x400 + (i) * 8)
-#define  CHV_GPIO_GPIOEN   (1 << 15)
-#define  CHV_GPIO_GPIOCFG_GPIO (0 << 8)
-#define  CHV_GPIO_GPIOCFG_GPO  (1 << 8)
-#define  CHV_GPIO_GPIOCFG_GPI  (2 << 8)
-#define  CHV_GPIO_GPIOCFG_HIZ  (3 << 8)
-#define  CHV_GPIO_GPIOTXSTATE(state)   ((!!(state)) << 1)
-
-#define CHV_GPIO_PAD_CFG1(f, i)(0x4400 + (f) * 0x400 + (i) * 8 
+ 4)
-#define  CHV_GPIO_CFGLOCK  (1 << 31)
-
 /* ICL DSI Display GPIO Pins */
 #define  ICL_GPIO_DDSP_HPD_A   0
 #define  ICL_GPIO_L_VDDEN_11
@@ -278,23 +265,21 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
   u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   u16 cfg0, cfg1;
-   u16 family_num;
-   u8 port;
 
if (connector->panel.vbt.dsi.seq_version >= 3) {
if (gpio_index >= CHV_GPIO_IDX_START_SE) {
/* XXX: it's unclear whether 255->57 is part of SE. */
-   gpio_index -= CHV_GPIO_IDX_START_SE;
-   port = CHV_IOSF_PORT_GPIO_SE;
+   soc_opaque_gpio_set_value(connector, gpio_index, 
"INT33FF:03", "Panel SE",
+ gpio_index - 
CHV_GPIO_IDX_START_SE, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
-   gpio_index -= CHV_GPIO_IDX_START_SW;
-   port = CHV_IOSF_PORT_GPIO_SW;
+   soc_opaque_gpio_set_value(connector, gpio_index, 
"INT33FF:00", "Panel SW",
+ gpio_index - 
CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
-   gpio_index -= CHV_GPIO_IDX_START_E;
-   port = CHV_IOSF_PORT_GPIO_E;
+   soc_opaque_gpio_set_value(connector, gpio_index, 
"INT33FF:02", "Panel E",
+ gpio_index - 
CHV_GPIO_IDX_START_E, value);
} else {
-   port = CHV_IOSF_PORT_GPIO_N;
+   soc_opaque_gpio_set_value(connector, gpio_index, 
"INT33FF:01", "Panel N",
+ gpio_index - 
CHV_GPIO_IDX_START_N, value);
}
} else {
/* XXX: The spec is unclear about CHV GPIO on seq v2 */
@@ -311,21 +296,9 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
return;
}
 
-   port = CHV_IOSF_PORT_GPIO_N;
+   soc_opaque_gpio_set_value(connector, gpio_index, "INT33FF:01", 
"Panel N",
+ gpio_index - CHV_GPIO_IDX_START_N, 
value);
}
-
-   family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
-   gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
-
-   cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
-   cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
-
-   vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-   vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
-   vlv_iosf_sb_write(dev_priv, port, cfg0,
- CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
- CHV_GPIO_GPIOTXSTATE(value));
-   vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
 static void bxt_gpio_set_value(struct intel_connector *connector,
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 11/16] drm/i915/dsi: Extract common soc_gpio_set_value() helper

[Andy Shevchenko]

Extract a common soc_gpio_set_value() helper that may be used by a few SoCs.

Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 46 +++-
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 0f9da0168a7b..9847a92fdfc3 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -243,6 +243,31 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
+static void soc_gpio_set_value(struct intel_connector *connector, const char 
*con_id,
+  u8 gpio_index, bool value)
+{
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   /* XXX: this table is a quick ugly hack. */
+   static struct gpio_desc *soc_gpio_table[U8_MAX + 1];
+   struct gpio_desc *gpio_desc = soc_gpio_table[gpio_index];
+
+   if (gpio_desc) {
+   gpiod_set_value(gpio_desc, value);
+   } else {
+   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
+con_id, gpio_index,
+value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
+   if (IS_ERR(gpio_desc)) {
+   drm_err(_priv->drm,
+   "GPIO index %u request failed (%pe)\n",
+   gpio_index, gpio_desc);
+   return;
+   }
+
+   soc_gpio_table[gpio_index] = gpio_desc;
+   }
+}
+
 static void vlv_gpio_set_value(struct intel_connector *connector,
   u8 gpio_source, u8 gpio_index, bool value)
 {
@@ -348,26 +373,7 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
 static void bxt_gpio_set_value(struct intel_connector *connector,
   u8 gpio_index, bool value)
 {
-   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   /* XXX: this table is a quick ugly hack. */
-   static struct gpio_desc *bxt_gpio_table[U8_MAX + 1];
-   struct gpio_desc *gpio_desc = bxt_gpio_table[gpio_index];
-
-   if (!gpio_desc) {
-   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-NULL, gpio_index,
-value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
-   if (IS_ERR_OR_NULL(gpio_desc)) {
-   drm_err(_priv->drm,
-   "GPIO index %u request failed (%ld)\n",
-   gpio_index, PTR_ERR(gpio_desc));
-   return;
-   }
-
-   bxt_gpio_table[gpio_index] = gpio_desc;
-   }
-
-   gpiod_set_value(gpio_desc, value);
+   soc_gpio_set_value(connector, NULL, gpio_index, value);
 }
 
 enum {
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 15/16] drm/i915/dsi: Combine checks in mipi_exec_gpio()

[Andy Shevchenko]

For a couple of cases the branches call the same bxt_gpio_set_value().
As Ville suggested they can be combined by dropping the DISPLAY_VER()
check from Gen 11 to Gen 9. Do it that way.

Suggested-by: Ville Syrjälä 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 9c6946ccb193..275d0218394c 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -420,14 +420,12 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
 
if (native)
icl_native_gpio_set_value(i915, gpio_number, value);
-   else if (DISPLAY_VER(i915) >= 11)
+   else if (DISPLAY_VER(i915) >= 9)
bxt_gpio_set_value(connector, gpio_index, value);
else if (IS_VALLEYVIEW(i915))
vlv_gpio_set_value(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(i915))
chv_gpio_set_value(connector, gpio_source, gpio_number, value);
-   else
-   bxt_gpio_set_value(connector, gpio_index, value);
 
return data + size;
 }
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 09/16] drm/i915/dsi: Remove GPIO lookup table at the end of intel_dsi_vbt_gpio_init()

[Andy Shevchenko]

From: Hans de Goede 

To properly deal with GPIOs used in MIPI panel sequences a temporary
GPIO lookup will be used. Since there can only be 1 GPIO lookup table
for the ":00:02.0" device this will not work if the GPIO lookup
table used by intel_dsi_vbt_gpio_init() is still registered.

After getting the "backlight" and "panel" GPIOs the lookup table
registered by intel_dsi_vbt_gpio_init() is no longer necessary,
remove it so that another temporary lookup-table for the ":00:02.0"
device can be added.

Signed-off-by: Hans de Goede 
Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 25 +++-
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index d270437217b3..8e6beef90e5e 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -955,6 +955,7 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, 
bool panel_is_on)
struct intel_connector *connector = intel_dsi->attached_connector;
struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
+   struct gpiod_lookup_table *gpiod_lookup_table = NULL;
bool want_backlight_gpio = false;
bool want_panel_gpio = false;
struct pinctrl *pinctrl;
@@ -962,12 +963,12 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, 
bool panel_is_on)
 
if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
mipi_config->pwm_blc == PPS_BLC_PMIC) {
-   gpiod_add_lookup_table(_panel_gpio_table);
+   gpiod_lookup_table = _panel_gpio_table;
want_panel_gpio = true;
}
 
if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
-   gpiod_add_lookup_table(_panel_gpio_table);
+   gpiod_lookup_table = _panel_gpio_table;
want_panel_gpio = true;
want_backlight_gpio = true;
 
@@ -984,6 +985,9 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, 
bool panel_is_on)
"Failed to set pinmux to PWM\n");
}
 
+   if (gpiod_lookup_table)
+   gpiod_add_lookup_table(gpiod_lookup_table);
+
if (want_panel_gpio) {
intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
if (IS_ERR(intel_dsi->gpio_panel)) {
@@ -1002,15 +1006,13 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi 
*intel_dsi, bool panel_is_on)
intel_dsi->gpio_backlight = NULL;
}
}
+
+   if (gpiod_lookup_table)
+   gpiod_remove_lookup_table(gpiod_lookup_table);
 }
 
 void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
 {
-   struct drm_device *dev = intel_dsi->base.base.dev;
-   struct drm_i915_private *dev_priv = to_i915(dev);
-   struct intel_connector *connector = intel_dsi->attached_connector;
-   struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
-
if (intel_dsi->gpio_panel) {
gpiod_put(intel_dsi->gpio_panel);
intel_dsi->gpio_panel = NULL;
@@ -1020,13 +1022,4 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi 
*intel_dsi)
gpiod_put(intel_dsi->gpio_backlight);
intel_dsi->gpio_backlight = NULL;
}
-
-   if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
-   mipi_config->pwm_blc == PPS_BLC_PMIC)
-   gpiod_remove_lookup_table(_panel_gpio_table);
-
-   if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
-   pinctrl_unregister_mappings(soc_pwm_pinctrl_map);
-   gpiod_remove_lookup_table(_panel_gpio_table);
-   }
 }
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [rft, PATCH v4 00/16] drm/i915/dsi: 4th attempt to get rid of IOSF GPIO

[Andy Shevchenko]

DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
talking to GPIO IP behind the actual driver's back. A second attempt
to fix that is here.

If I understood correctly, my approach should work in the similar way as
the current IOSF GPIO.

Hans, I believe you have some devices that use this piece of code,
is it possible to give a test run on (one of) them?

In v4:
- fixed compile time errors in patch 14 (Hans, LKP)
- fixed cover letter Subject
- added patch 15 (as suggested by Ville)
- added Ack tag (Jani)

In v3:
- incorporated series by Jani
- incorporated couple of precursor patches by Hans
- added Rb tag for used to be first three patches (Andi)
- rebased on top of the above changes
- fixed indexing for multi-community devices, such as Cherry View

In v2:
- added a few cleanup patches
- reworked to use dynamic GPIO lookup tables
- converted CHV as well

Andy Shevchenko (9):
  drm/i915/dsi: Replace while(1) with one with clear exit condition
  drm/i915/dsi: Get rid of redundant 'else'
  drm/i915/dsi: Replace check with a (missing) MIPI sequence name
  drm/i915/dsi: Extract common soc_gpio_set_value() helper
  drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back
  drm/i915/dsi: Prepare soc_gpio_set_value() to distinguish GPIO
communities
  drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
  drm/i915/dsi: Combine checks in mipi_exec_gpio()
  drm/i915/iosf: Drop unused APIs

Hans de Goede (2):
  drm/i915/dsi: Remove GPIO lookup table at the end of
intel_dsi_vbt_gpio_init()
  drm/i915/dsi: Fix wrong initial value for GPIOs in
bxt_gpio_set_value()

Jani Nikula (5):
  drm/i915/dsi: assume BXT gpio works for non-native GPIO
  drm/i915/dsi: switch mipi_exec_gpio() from dev_priv to i915
  drm/i915/dsi: clarify GPIO exec sequence
  drm/i915/dsi: rename platform specific *_exec_gpio() to
*_gpio_set_value()
  drm/i915/dsi: bxt/icl GPIO set value do not need gpio source

 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 355 +++
 drivers/gpu/drm/i915/vlv_sideband.c  |  17 -
 drivers/gpu/drm/i915/vlv_sideband.h  |   3 -
 3 files changed, 136 insertions(+), 239 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 06/16] drm/i915/dsi: Replace while(1) with one with clear exit condition

[Andy Shevchenko]

Move existing condition to while(), so it will be clear on what
circumstances the loop is successfully finishing.

Reviewed-by: Andi Shyti 
Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 4af43cf3cee0..290a112f1b63 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -702,13 +702,10 @@ static void intel_dsi_vbt_exec(struct intel_dsi 
*intel_dsi,
if (connector->panel.vbt.dsi.seq_version >= 3)
data += 4;
 
-   while (1) {
+   while (*data != MIPI_SEQ_ELEM_END) {
u8 operation_byte = *data++;
u8 operation_size = 0;
 
-   if (operation_byte == MIPI_SEQ_ELEM_END)
-   break;
-
if (operation_byte < ARRAY_SIZE(exec_elem))
mipi_elem_exec = exec_elem[operation_byte];
else
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 12/16] drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back

[Andy Shevchenko]

It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 40ecab551232 ("pinctrl:
baytrail: Really serialize all register accesses") for the details.
Taking all this into consideration replace the hack with proper
GPIO APIs being used.

Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 98 ++--
 1 file changed, 28 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 9847a92fdfc3..552bc6564d79 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -55,43 +55,6 @@
 #define MIPI_VIRTUAL_CHANNEL_SHIFT 1
 #define MIPI_PORT_SHIFT3
 
-/* base offsets for gpio pads */
-#define VLV_GPIO_NC_0_HV_DDI0_HPD  0x4130
-#define VLV_GPIO_NC_1_HV_DDI0_DDC_SDA  0x4120
-#define VLV_GPIO_NC_2_HV_DDI0_DDC_SCL  0x4110
-#define VLV_GPIO_NC_3_PANEL0_VDDEN 0x4140
-#define VLV_GPIO_NC_4_PANEL0_BKLTEN0x4150
-#define VLV_GPIO_NC_5_PANEL0_BKLTCTL   0x4160
-#define VLV_GPIO_NC_6_HV_DDI1_HPD  0x4180
-#define VLV_GPIO_NC_7_HV_DDI1_DDC_SDA  0x4190
-#define VLV_GPIO_NC_8_HV_DDI1_DDC_SCL  0x4170
-#define VLV_GPIO_NC_9_PANEL1_VDDEN 0x4100
-#define VLV_GPIO_NC_10_PANEL1_BKLTEN   0x40E0
-#define VLV_GPIO_NC_11_PANEL1_BKLTCTL  0x40F0
-
-#define VLV_GPIO_PCONF0(base_offset)   (base_offset)
-#define VLV_GPIO_PAD_VAL(base_offset)  ((base_offset) + 8)
-
-struct gpio_map {
-   u16 base_offset;
-   bool init;
-};
-
-static struct gpio_map vlv_gpio_table[] = {
-   { VLV_GPIO_NC_0_HV_DDI0_HPD },
-   { VLV_GPIO_NC_1_HV_DDI0_DDC_SDA },
-   { VLV_GPIO_NC_2_HV_DDI0_DDC_SCL },
-   { VLV_GPIO_NC_3_PANEL0_VDDEN },
-   { VLV_GPIO_NC_4_PANEL0_BKLTEN },
-   { VLV_GPIO_NC_5_PANEL0_BKLTCTL },
-   { VLV_GPIO_NC_6_HV_DDI1_HPD },
-   { VLV_GPIO_NC_7_HV_DDI1_DDC_SDA },
-   { VLV_GPIO_NC_8_HV_DDI1_DDC_SCL },
-   { VLV_GPIO_NC_9_PANEL1_VDDEN },
-   { VLV_GPIO_NC_10_PANEL1_BKLTEN },
-   { VLV_GPIO_NC_11_PANEL1_BKLTCTL },
-};
-
 struct i2c_adapter_lookup {
u16 slave_addr;
struct intel_dsi *intel_dsi;
@@ -268,52 +231,47 @@ static void soc_gpio_set_value(struct intel_connector 
*connector, const char *co
}
 }
 
+static void soc_opaque_gpio_set_value(struct intel_connector *connector,
+ const char *chip, const char *con_id,
+ u8 gpio_index, bool value)
+{
+   struct gpiod_lookup_table *lookup;
+
+   lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
+   if (!lookup)
+   return;
+
+   lookup->dev_id = ":00:02.0";
+   lookup->table[0] =
+   GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index, 
GPIO_ACTIVE_HIGH);
+
+   gpiod_add_lookup_table(lookup);
+
+   soc_gpio_set_value(connector, con_id, gpio_index, value);
+
+   gpiod_remove_lookup_table(lookup);
+   kfree(lookup);
+}
+
 static void vlv_gpio_set_value(struct intel_connector *connector,
   u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   struct gpio_map *map;
-   u16 pconf0, padval;
-   u32 tmp;
-   u8 port;
 
-   if (gpio_index >= ARRAY_SIZE(vlv_gpio_table)) {
-   drm_dbg_kms(_priv->drm, "unknown gpio index %u\n",
-   gpio_index);
-   return;
-   }
-
-   map = _gpio_table[gpio_index];
-
-   if (connector->panel.vbt.dsi.seq_version >= 3) {
-   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
-   port = IOSF_PORT_GPIO_NC;
-   } else {
-   if (gpio_source == 0) {
-   port = IOSF_PORT_GPIO_NC;
-   } else if (gpio_source == 1) {
+   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
+   if (connector->panel.vbt.dsi.seq_version < 3) {
+   if (gpio_source == 1) {
drm_dbg_kms(_priv->drm, "SC gpio not supported\n");
return;
-   } else {
+   }
+   if (gpio_source > 1) {
drm_dbg_kms(_priv->drm,
"unknown gpio source %u\n", gpio_source);
return;
}
}
 
-   pconf0 = VLV_GPIO_PCONF0(map->base_offset);
-   padval = VLV_GPIO_PAD_VAL(map->base_offset);
-
-   vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-   if (!map->init) {
-   /* FIXME: remove constant below */
-   vlv_iosf_sb_write(dev_priv, port, pconf0, 0x2000CC00);
-  

[Intel-gfx] [PATCH v4 05/16] drm/i915/dsi: bxt/icl GPIO set value do not need gpio source

[Andy Shevchenko]

From: Jani Nikula 

Drop the unused parameter.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index f977d63a0ad4..4af43cf3cee0 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -346,7 +346,7 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
 }
 
 static void bxt_gpio_set_value(struct intel_connector *connector,
-  u8 gpio_source, u8 gpio_index, bool value)
+  u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
/* XXX: this table is a quick ugly hack. */
@@ -486,13 +486,13 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(i915, gpio_number, value);
else if (DISPLAY_VER(i915) >= 11)
-   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_index, value);
else if (IS_VALLEYVIEW(i915))
vlv_gpio_set_value(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(i915))
chv_gpio_set_value(connector, gpio_source, gpio_number, value);
else
-   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_index, value);
 
return data + size;
 }
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 01/16] drm/i915/dsi: assume BXT gpio works for non-native GPIO

[Andy Shevchenko]

From: Jani Nikula 

Purely a guess. Drop the nop function.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 24b2cbcfc1ef..b2c0cc11f8c1 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -372,14 +372,6 @@ static void bxt_exec_gpio(struct intel_connector 
*connector,
gpiod_set_value(gpio_desc, value);
 }
 
-static void icl_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
-{
-   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-
-   drm_dbg_kms(_priv->drm, "Skipping ICL GPIO element execution\n");
-}
-
 enum {
MIPI_RESET_1 = 0,
MIPI_AVDD_EN_1,
@@ -491,7 +483,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(dev_priv, gpio_number, value);
else if (DISPLAY_VER(dev_priv) >= 11)
-   icl_exec_gpio(connector, gpio_source, gpio_index, value);
+   bxt_exec_gpio(connector, gpio_source, gpio_index, value);
else if (IS_VALLEYVIEW(dev_priv))
vlv_exec_gpio(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(dev_priv))
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 10/16] drm/i915/dsi: Fix wrong initial value for GPIOs in bxt_gpio_set_value()

[Andy Shevchenko]

From: Hans de Goede 

Fix wrong initial value for GPIOs in bxt_gpio_set_value().

Signed-off-by: Hans de Goede 
Signed-off-by: Andy Shevchenko 
Acked-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8e6beef90e5e..0f9da0168a7b 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -356,9 +356,7 @@ static void bxt_gpio_set_value(struct intel_connector 
*connector,
if (!gpio_desc) {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
 NULL, gpio_index,
-value ? GPIOD_OUT_LOW :
-GPIOD_OUT_HIGH);
-
+value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
if (IS_ERR_OR_NULL(gpio_desc)) {
drm_err(_priv->drm,
"GPIO index %u request failed (%ld)\n",
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 04/16] drm/i915/dsi: rename platform specific *_exec_gpio() to *_gpio_set_value()

[Andy Shevchenko]

From: Jani Nikula 

The lowest level functions are about setting GPIO values, not about
executing any sequences anymore.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 11073efe26c0..f977d63a0ad4 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -243,8 +243,8 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
-static void vlv_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
+static void vlv_gpio_set_value(struct intel_connector *connector,
+  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct gpio_map *map;
@@ -291,8 +291,8 @@ static void vlv_exec_gpio(struct intel_connector *connector,
vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
-static void chv_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
+static void chv_gpio_set_value(struct intel_connector *connector,
+  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
u16 cfg0, cfg1;
@@ -345,8 +345,8 @@ static void chv_exec_gpio(struct intel_connector *connector,
vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
-static void bxt_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
+static void bxt_gpio_set_value(struct intel_connector *connector,
+  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
/* XXX: this table is a quick ugly hack. */
@@ -486,13 +486,13 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(i915, gpio_number, value);
else if (DISPLAY_VER(i915) >= 11)
-   bxt_exec_gpio(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
else if (IS_VALLEYVIEW(i915))
-   vlv_exec_gpio(connector, gpio_source, gpio_number, value);
+   vlv_gpio_set_value(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(i915))
-   chv_exec_gpio(connector, gpio_source, gpio_number, value);
+   chv_gpio_set_value(connector, gpio_source, gpio_number, value);
else
-   bxt_exec_gpio(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
 
return data + size;
 }
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 03/16] drm/i915/dsi: clarify GPIO exec sequence

[Andy Shevchenko]

From: Jani Nikula 

With the various sequence versions and pointer increments interleaved,
it's a bit hard to decipher what's going on. Add separate paths for
different sequence versions.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 31 +++-
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8b962f2ac475..11073efe26c0 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -456,26 +456,29 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
struct drm_device *dev = intel_dsi->base.base.dev;
struct drm_i915_private *i915 = to_i915(dev);
struct intel_connector *connector = intel_dsi->attached_connector;
-   u8 gpio_source, gpio_index = 0, gpio_number;
+   u8 gpio_source = 0, gpio_index = 0, gpio_number;
bool value;
+   int size;
bool native = DISPLAY_VER(i915) >= 11;
 
-   if (connector->panel.vbt.dsi.seq_version >= 3)
-   gpio_index = *data++;
+   if (connector->panel.vbt.dsi.seq_version >= 3) {
+   size = 3;
 
-   gpio_number = *data++;
+   gpio_index = data[0];
+   gpio_number = data[1];
+   value = data[2] & BIT(0);
 
-   /* gpio source in sequence v2 only */
-   if (connector->panel.vbt.dsi.seq_version == 2)
-   gpio_source = (*data >> 1) & 3;
-   else
-   gpio_source = 0;
+   if (connector->panel.vbt.dsi.seq_version >= 4 && data[2] & 
BIT(1))
+   native = false;
+   } else {
+   size = 2;
 
-   if (connector->panel.vbt.dsi.seq_version >= 4 && *data & BIT(1))
-   native = false;
+   gpio_number = data[0];
+   value = data[1] & BIT(0);
 
-   /* pull up/down */
-   value = *data++ & 1;
+   if (connector->panel.vbt.dsi.seq_version == 2)
+   gpio_source = (data[1] >> 1) & 3;
+   }
 
drm_dbg_kms(>drm, "GPIO index %u, number %u, source %u, native 
%s, set to %s\n",
gpio_index, gpio_number, gpio_source, str_yes_no(native), 
str_on_off(value));
@@ -491,7 +494,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
else
bxt_exec_gpio(connector, gpio_source, gpio_index, value);
 
-   return data;
+   return data + size;
 }
 
 #ifdef CONFIG_ACPI
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v4 02/16] drm/i915/dsi: switch mipi_exec_gpio() from dev_priv to i915

[Andy Shevchenko]

From: Jani Nikula 

Follow the contemporary conventions.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index b2c0cc11f8c1..8b962f2ac475 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -454,11 +454,11 @@ static void icl_native_gpio_set_value(struct 
drm_i915_private *dev_priv,
 static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 {
struct drm_device *dev = intel_dsi->base.base.dev;
-   struct drm_i915_private *dev_priv = to_i915(dev);
+   struct drm_i915_private *i915 = to_i915(dev);
struct intel_connector *connector = intel_dsi->attached_connector;
u8 gpio_source, gpio_index = 0, gpio_number;
bool value;
-   bool native = DISPLAY_VER(dev_priv) >= 11;
+   bool native = DISPLAY_VER(i915) >= 11;
 
if (connector->panel.vbt.dsi.seq_version >= 3)
gpio_index = *data++;
@@ -477,16 +477,16 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
/* pull up/down */
value = *data++ & 1;
 
-   drm_dbg_kms(_priv->drm, "GPIO index %u, number %u, source %u, 
native %s, set to %s\n",
+   drm_dbg_kms(>drm, "GPIO index %u, number %u, source %u, native 
%s, set to %s\n",
gpio_index, gpio_number, gpio_source, str_yes_no(native), 
str_on_off(value));
 
if (native)
-   icl_native_gpio_set_value(dev_priv, gpio_number, value);
-   else if (DISPLAY_VER(dev_priv) >= 11)
+   icl_native_gpio_set_value(i915, gpio_number, value);
+   else if (DISPLAY_VER(i915) >= 11)
bxt_exec_gpio(connector, gpio_source, gpio_index, value);
-   else if (IS_VALLEYVIEW(dev_priv))
+   else if (IS_VALLEYVIEW(i915))
vlv_exec_gpio(connector, gpio_source, gpio_number, value);
-   else if (IS_CHERRYVIEW(dev_priv))
+   else if (IS_CHERRYVIEW(i915))
chv_exec_gpio(connector, gpio_source, gpio_number, value);
else
bxt_exec_gpio(connector, gpio_source, gpio_index, value);
-- 
2.40.0.1.gaa8946217a0b

Re: [Intel-gfx] [PATCH v3 01/15] drm/i915/dsi: assume BXT gpio works for non-native GPIO

[Andy Shevchenko]

On Thu, Nov 02, 2023 at 07:10:09PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 02, 2023 at 05:12:14PM +0200, Andy Shevchenko wrote:

...

> > if (native)
> > icl_native_gpio_set_value(dev_priv, gpio_number, value);
> > else if (DISPLAY_VER(dev_priv) >= 11)
> > -   icl_exec_gpio(connector, gpio_source, gpio_index, value);
> > +   bxt_exec_gpio(connector, gpio_source, gpio_index, value);
> 
> We could just drop this whole branch since we end up in bxt_exec_gpio()
> in the end anyway. Or we drop the final else and make this one check for
> DISPLAY_VER >=9.

Looking at the code, I'm not sure how we can get rid of it, but the second
option is feasible.

> > else if (IS_VALLEYVIEW(dev_priv))
> > vlv_exec_gpio(connector, gpio_source, gpio_number, value);
> > else if (IS_CHERRYVIEW(dev_priv))

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v3 15/15] drm/i915/iosf: Drop unused APIs

[Andy Shevchenko]

Drop unused vlv_iosf_sb_read() and vlv_iosf_sb_write().

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/vlv_sideband.c | 17 -
 drivers/gpu/drm/i915/vlv_sideband.h |  3 ---
 2 files changed, 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/vlv_sideband.c 
b/drivers/gpu/drm/i915/vlv_sideband.c
index b98dec3ad817..13b644958e38 100644
--- a/drivers/gpu/drm/i915/vlv_sideband.c
+++ b/drivers/gpu/drm/i915/vlv_sideband.c
@@ -166,23 +166,6 @@ u32 vlv_nc_read(struct drm_i915_private *i915, u8 addr)
return val;
 }
 
-u32 vlv_iosf_sb_read(struct drm_i915_private *i915, u8 port, u32 reg)
-{
-   u32 val = 0;
-
-   vlv_sideband_rw(i915, PCI_DEVFN(0, 0), port,
-   SB_CRRDDA_NP, reg, );
-
-   return val;
-}
-
-void vlv_iosf_sb_write(struct drm_i915_private *i915,
-  u8 port, u32 reg, u32 val)
-{
-   vlv_sideband_rw(i915, PCI_DEVFN(0, 0), port,
-   SB_CRWRDA_NP, reg, );
-}
-
 u32 vlv_cck_read(struct drm_i915_private *i915, u32 reg)
 {
u32 val = 0;
diff --git a/drivers/gpu/drm/i915/vlv_sideband.h 
b/drivers/gpu/drm/i915/vlv_sideband.h
index 9ce283d96b80..8b4495e14bce 100644
--- a/drivers/gpu/drm/i915/vlv_sideband.h
+++ b/drivers/gpu/drm/i915/vlv_sideband.h
@@ -26,9 +26,6 @@ enum {
 };
 
 void vlv_iosf_sb_get(struct drm_i915_private *i915, unsigned long ports);
-u32 vlv_iosf_sb_read(struct drm_i915_private *i915, u8 port, u32 reg);
-void vlv_iosf_sb_write(struct drm_i915_private *i915,
-  u8 port, u32 reg, u32 val);
 void vlv_iosf_sb_put(struct drm_i915_private *i915, unsigned long ports);
 
 static inline void vlv_bunit_get(struct drm_i915_private *i915)
-- 
2.40.0.1.gaa8946217a0b

Re: [Intel-gfx] [PATCH v3 14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

On Thu, Nov 02, 2023 at 04:47:41PM +0100, Hans de Goede wrote:
> On 11/2/23 16:12, Andy Shevchenko wrote:

...

> > +   soc_exec_opaque_gpio(connector, gpio_index, 
> > "INT33FF:03", "Panel SE",
> > +gpio_index - 
> > CHV_GPIO_IDX_START_SW, value);
> 
> The "gpio_index - CHV_GPIO_IDX_START_SW" here needs to be "gpio_index - 
> CHV_GPIO_IDX_START_SE".
> 
> Also this patch needs s/soc_exec_opaque_gpio/soc_opaque_gpio_set_value/ to 
> compile ...

Ah, indeed. I looks like I run the test build, but forgot to look into the 
result. :-(

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v3 11/15] drm/i915/dsi: Extract common soc_gpio_set_value() helper

[Andy Shevchenko]

Extract a common soc_gpio_set_value() helper that may be used by a few SoCs.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 46 +++-
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 0f9da0168a7b..9847a92fdfc3 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -243,6 +243,31 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
+static void soc_gpio_set_value(struct intel_connector *connector, const char 
*con_id,
+  u8 gpio_index, bool value)
+{
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   /* XXX: this table is a quick ugly hack. */
+   static struct gpio_desc *soc_gpio_table[U8_MAX + 1];
+   struct gpio_desc *gpio_desc = soc_gpio_table[gpio_index];
+
+   if (gpio_desc) {
+   gpiod_set_value(gpio_desc, value);
+   } else {
+   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
+con_id, gpio_index,
+value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
+   if (IS_ERR(gpio_desc)) {
+   drm_err(_priv->drm,
+   "GPIO index %u request failed (%pe)\n",
+   gpio_index, gpio_desc);
+   return;
+   }
+
+   soc_gpio_table[gpio_index] = gpio_desc;
+   }
+}
+
 static void vlv_gpio_set_value(struct intel_connector *connector,
   u8 gpio_source, u8 gpio_index, bool value)
 {
@@ -348,26 +373,7 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
 static void bxt_gpio_set_value(struct intel_connector *connector,
   u8 gpio_index, bool value)
 {
-   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   /* XXX: this table is a quick ugly hack. */
-   static struct gpio_desc *bxt_gpio_table[U8_MAX + 1];
-   struct gpio_desc *gpio_desc = bxt_gpio_table[gpio_index];
-
-   if (!gpio_desc) {
-   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-NULL, gpio_index,
-value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
-   if (IS_ERR_OR_NULL(gpio_desc)) {
-   drm_err(_priv->drm,
-   "GPIO index %u request failed (%ld)\n",
-   gpio_index, PTR_ERR(gpio_desc));
-   return;
-   }
-
-   bxt_gpio_table[gpio_index] = gpio_desc;
-   }
-
-   gpiod_set_value(gpio_desc, value);
+   soc_gpio_set_value(connector, NULL, gpio_index, value);
 }
 
 enum {
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 12/15] drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back

[Andy Shevchenko]

It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 40ecab551232 ("pinctrl:
baytrail: Really serialize all register accesses") for the details.
Taking all this into consideration replace the hack with proper
GPIO APIs being used.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 98 ++--
 1 file changed, 28 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 9847a92fdfc3..552bc6564d79 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -55,43 +55,6 @@
 #define MIPI_VIRTUAL_CHANNEL_SHIFT 1
 #define MIPI_PORT_SHIFT3
 
-/* base offsets for gpio pads */
-#define VLV_GPIO_NC_0_HV_DDI0_HPD  0x4130
-#define VLV_GPIO_NC_1_HV_DDI0_DDC_SDA  0x4120
-#define VLV_GPIO_NC_2_HV_DDI0_DDC_SCL  0x4110
-#define VLV_GPIO_NC_3_PANEL0_VDDEN 0x4140
-#define VLV_GPIO_NC_4_PANEL0_BKLTEN0x4150
-#define VLV_GPIO_NC_5_PANEL0_BKLTCTL   0x4160
-#define VLV_GPIO_NC_6_HV_DDI1_HPD  0x4180
-#define VLV_GPIO_NC_7_HV_DDI1_DDC_SDA  0x4190
-#define VLV_GPIO_NC_8_HV_DDI1_DDC_SCL  0x4170
-#define VLV_GPIO_NC_9_PANEL1_VDDEN 0x4100
-#define VLV_GPIO_NC_10_PANEL1_BKLTEN   0x40E0
-#define VLV_GPIO_NC_11_PANEL1_BKLTCTL  0x40F0
-
-#define VLV_GPIO_PCONF0(base_offset)   (base_offset)
-#define VLV_GPIO_PAD_VAL(base_offset)  ((base_offset) + 8)
-
-struct gpio_map {
-   u16 base_offset;
-   bool init;
-};
-
-static struct gpio_map vlv_gpio_table[] = {
-   { VLV_GPIO_NC_0_HV_DDI0_HPD },
-   { VLV_GPIO_NC_1_HV_DDI0_DDC_SDA },
-   { VLV_GPIO_NC_2_HV_DDI0_DDC_SCL },
-   { VLV_GPIO_NC_3_PANEL0_VDDEN },
-   { VLV_GPIO_NC_4_PANEL0_BKLTEN },
-   { VLV_GPIO_NC_5_PANEL0_BKLTCTL },
-   { VLV_GPIO_NC_6_HV_DDI1_HPD },
-   { VLV_GPIO_NC_7_HV_DDI1_DDC_SDA },
-   { VLV_GPIO_NC_8_HV_DDI1_DDC_SCL },
-   { VLV_GPIO_NC_9_PANEL1_VDDEN },
-   { VLV_GPIO_NC_10_PANEL1_BKLTEN },
-   { VLV_GPIO_NC_11_PANEL1_BKLTCTL },
-};
-
 struct i2c_adapter_lookup {
u16 slave_addr;
struct intel_dsi *intel_dsi;
@@ -268,52 +231,47 @@ static void soc_gpio_set_value(struct intel_connector 
*connector, const char *co
}
 }
 
+static void soc_opaque_gpio_set_value(struct intel_connector *connector,
+ const char *chip, const char *con_id,
+ u8 gpio_index, bool value)
+{
+   struct gpiod_lookup_table *lookup;
+
+   lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
+   if (!lookup)
+   return;
+
+   lookup->dev_id = ":00:02.0";
+   lookup->table[0] =
+   GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index, 
GPIO_ACTIVE_HIGH);
+
+   gpiod_add_lookup_table(lookup);
+
+   soc_gpio_set_value(connector, con_id, gpio_index, value);
+
+   gpiod_remove_lookup_table(lookup);
+   kfree(lookup);
+}
+
 static void vlv_gpio_set_value(struct intel_connector *connector,
   u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   struct gpio_map *map;
-   u16 pconf0, padval;
-   u32 tmp;
-   u8 port;
 
-   if (gpio_index >= ARRAY_SIZE(vlv_gpio_table)) {
-   drm_dbg_kms(_priv->drm, "unknown gpio index %u\n",
-   gpio_index);
-   return;
-   }
-
-   map = _gpio_table[gpio_index];
-
-   if (connector->panel.vbt.dsi.seq_version >= 3) {
-   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
-   port = IOSF_PORT_GPIO_NC;
-   } else {
-   if (gpio_source == 0) {
-   port = IOSF_PORT_GPIO_NC;
-   } else if (gpio_source == 1) {
+   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
+   if (connector->panel.vbt.dsi.seq_version < 3) {
+   if (gpio_source == 1) {
drm_dbg_kms(_priv->drm, "SC gpio not supported\n");
return;
-   } else {
+   }
+   if (gpio_source > 1) {
drm_dbg_kms(_priv->drm,
"unknown gpio source %u\n", gpio_source);
return;
}
}
 
-   pconf0 = VLV_GPIO_PCONF0(map->base_offset);
-   padval = VLV_GPIO_PAD_VAL(map->base_offset);
-
-   vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-   if (!map->init) {
-   /* FIXME: remove constant below */
-   vlv_iosf_sb_write(dev_priv, port, pconf0, 0x2000CC00);
-   map->init = 

[Intel-gfx] [PATCH v3 03/15] drm/i915/dsi: clarify GPIO exec sequence

[Andy Shevchenko]

From: Jani Nikula 

With the various sequence versions and pointer increments interleaved,
it's a bit hard to decipher what's going on. Add separate paths for
different sequence versions.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 31 +++-
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8b962f2ac475..11073efe26c0 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -456,26 +456,29 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
struct drm_device *dev = intel_dsi->base.base.dev;
struct drm_i915_private *i915 = to_i915(dev);
struct intel_connector *connector = intel_dsi->attached_connector;
-   u8 gpio_source, gpio_index = 0, gpio_number;
+   u8 gpio_source = 0, gpio_index = 0, gpio_number;
bool value;
+   int size;
bool native = DISPLAY_VER(i915) >= 11;
 
-   if (connector->panel.vbt.dsi.seq_version >= 3)
-   gpio_index = *data++;
+   if (connector->panel.vbt.dsi.seq_version >= 3) {
+   size = 3;
 
-   gpio_number = *data++;
+   gpio_index = data[0];
+   gpio_number = data[1];
+   value = data[2] & BIT(0);
 
-   /* gpio source in sequence v2 only */
-   if (connector->panel.vbt.dsi.seq_version == 2)
-   gpio_source = (*data >> 1) & 3;
-   else
-   gpio_source = 0;
+   if (connector->panel.vbt.dsi.seq_version >= 4 && data[2] & 
BIT(1))
+   native = false;
+   } else {
+   size = 2;
 
-   if (connector->panel.vbt.dsi.seq_version >= 4 && *data & BIT(1))
-   native = false;
+   gpio_number = data[0];
+   value = data[1] & BIT(0);
 
-   /* pull up/down */
-   value = *data++ & 1;
+   if (connector->panel.vbt.dsi.seq_version == 2)
+   gpio_source = (data[1] >> 1) & 3;
+   }
 
drm_dbg_kms(>drm, "GPIO index %u, number %u, source %u, native 
%s, set to %s\n",
gpio_index, gpio_number, gpio_source, str_yes_no(native), 
str_on_off(value));
@@ -491,7 +494,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
else
bxt_exec_gpio(connector, gpio_source, gpio_index, value);
 
-   return data;
+   return data + size;
 }
 
 #ifdef CONFIG_ACPI
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
cherryview: prevent concurrent access to GPIO controllers") for
the details. Taking all this into consideration replace the hack
with proper GPIO APIs being used.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +---
 1 file changed, 10 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index b1736c1301ea..ffc65c943b11 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
 #define CHV_GPIO_IDX_START_SW  100
 #define CHV_GPIO_IDX_START_SE  198
 
-#define CHV_VBT_MAX_PINS_PER_FMLY  15
-
-#define CHV_GPIO_PAD_CFG0(f, i)(0x4400 + (f) * 0x400 + (i) * 8)
-#define  CHV_GPIO_GPIOEN   (1 << 15)
-#define  CHV_GPIO_GPIOCFG_GPIO (0 << 8)
-#define  CHV_GPIO_GPIOCFG_GPO  (1 << 8)
-#define  CHV_GPIO_GPIOCFG_GPI  (2 << 8)
-#define  CHV_GPIO_GPIOCFG_HIZ  (3 << 8)
-#define  CHV_GPIO_GPIOTXSTATE(state)   ((!!(state)) << 1)
-
-#define CHV_GPIO_PAD_CFG1(f, i)(0x4400 + (f) * 0x400 + (i) * 8 
+ 4)
-#define  CHV_GPIO_CFGLOCK  (1 << 31)
-
 /* ICL DSI Display GPIO Pins */
 #define  ICL_GPIO_DDSP_HPD_A   0
 #define  ICL_GPIO_L_VDDEN_11
@@ -278,23 +265,21 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
   u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   u16 cfg0, cfg1;
-   u16 family_num;
-   u8 port;
 
if (connector->panel.vbt.dsi.seq_version >= 3) {
if (gpio_index >= CHV_GPIO_IDX_START_SE) {
/* XXX: it's unclear whether 255->57 is part of SE. */
-   gpio_index -= CHV_GPIO_IDX_START_SE;
-   port = CHV_IOSF_PORT_GPIO_SE;
+   soc_exec_opaque_gpio(connector, gpio_index, 
"INT33FF:03", "Panel SE",
+gpio_index - 
CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
-   gpio_index -= CHV_GPIO_IDX_START_SW;
-   port = CHV_IOSF_PORT_GPIO_SW;
+   soc_exec_opaque_gpio(connector, gpio_index, 
"INT33FF:00", "Panel SW",
+gpio_index - 
CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
-   gpio_index -= CHV_GPIO_IDX_START_E;
-   port = CHV_IOSF_PORT_GPIO_E;
+   soc_exec_opaque_gpio(connector, gpio_index, 
"INT33FF:02", "Panel E",
+gpio_index - CHV_GPIO_IDX_START_E, 
value);
} else {
-   port = CHV_IOSF_PORT_GPIO_N;
+   soc_exec_opaque_gpio(connector, gpio_index, 
"INT33FF:01", "Panel N",
+gpio_index - CHV_GPIO_IDX_START_N, 
value);
}
} else {
/* XXX: The spec is unclear about CHV GPIO on seq v2 */
@@ -311,21 +296,9 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
return;
}
 
-   port = CHV_IOSF_PORT_GPIO_N;
+   soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", 
"Panel N",
+gpio_index - CHV_GPIO_IDX_START_N, value);
}
-
-   family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
-   gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
-
-   cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
-   cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
-
-   vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-   vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
-   vlv_iosf_sb_write(dev_priv, port, cfg0,
- CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
- CHV_GPIO_GPIOTXSTATE(value));
-   vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
 static void bxt_gpio_set_value(struct intel_connector *connector,
-- 
2.40.0.1.gaa8946217a0b

Re: [Intel-gfx] [PATCH v3 10/15] drm/i915/dsi: Fix wrong initial value for GPIOs in bxt_exec_gpio()

[Andy Shevchenko]

On Thu, Nov 02, 2023 at 05:12:23PM +0200, Andy Shevchenko wrote:
> From: Hans de Goede 
> 
> Fix wrong initial value for GPIOs in bxt_exec_gpio().

Oh, and forgot to update the function name in this patch.

In any case I would wait for Hans to confirm it works (and probably he may give
a formal Tested-by tag) and then will send v4 to be applied for real.

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v3 07/15] drm/i915/dsi: Get rid of redundant 'else'

[Andy Shevchenko]

In the snippets like the following

if (...)
return / goto / break / continue ...;
else
...

the 'else' is redundant. Get rid of it.

Reviewed-by: Andi Shyti 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 58 ++--
 1 file changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 290a112f1b63..4ed5ede9ec5b 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -142,7 +142,7 @@ static enum port intel_dsi_seq_port_to_port(struct 
intel_dsi *intel_dsi,
if (seq_port) {
if (intel_dsi->ports & BIT(PORT_B))
return PORT_B;
-   else if (intel_dsi->ports & BIT(PORT_C))
+   if (intel_dsi->ports & BIT(PORT_C))
return PORT_C;
}
 
@@ -670,8 +670,8 @@ static const char *sequence_name(enum mipi_seq seq_id)
 {
if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
return seq_name[seq_id];
-   else
-   return "(unknown)";
+
+   return "(unknown)";
 }
 
 static void intel_dsi_vbt_exec(struct intel_dsi *intel_dsi,
@@ -865,36 +865,34 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 
panel_id)
 * multiply by 100 to preserve remainder
 */
if (intel_dsi->video_mode == BURST_MODE) {
-   if (mipi_config->target_burst_mode_freq) {
-   u32 bitrate = intel_dsi_bitrate(intel_dsi);
+   u32 bitrate;
 
-   /*
-* Sometimes the VBT contains a slightly lower clock,
-* then the bitrate we have calculated, in this case
-* just replace it with the calculated bitrate.
-*/
-   if (mipi_config->target_burst_mode_freq < bitrate &&
-   intel_fuzzy_clock_check(
-   mipi_config->target_burst_mode_freq,
-   bitrate))
-   mipi_config->target_burst_mode_freq = bitrate;
-
-   if (mipi_config->target_burst_mode_freq < bitrate) {
-   drm_err(_priv->drm,
-   "Burst mode freq is less than 
computed\n");
-   return false;
-   }
-
-   burst_mode_ratio = DIV_ROUND_UP(
-   mipi_config->target_burst_mode_freq * 100,
-   bitrate);
-
-   intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * 
burst_mode_ratio, 100);
-   } else {
-   drm_err(_priv->drm,
-   "Burst mode target is not set\n");
+   if (mipi_config->target_burst_mode_freq == 0) {
+   drm_err(_priv->drm, "Burst mode target is not 
set\n");
return false;
}
+
+   bitrate = intel_dsi_bitrate(intel_dsi);
+
+   /*
+* Sometimes the VBT contains a slightly lower clock, then
+* the bitrate we have calculated, in this case just replace it
+* with the calculated bitrate.
+*/
+   if (mipi_config->target_burst_mode_freq < bitrate &&
+   intel_fuzzy_clock_check(mipi_config->target_burst_mode_freq,
+   bitrate))
+   mipi_config->target_burst_mode_freq = bitrate;
+
+   if (mipi_config->target_burst_mode_freq < bitrate) {
+   drm_err(_priv->drm, "Burst mode freq is less than 
computed\n");
+   return false;
+   }
+
+   burst_mode_ratio =
+   DIV_ROUND_UP(mipi_config->target_burst_mode_freq * 100, 
bitrate);
+
+   intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * 
burst_mode_ratio, 100);
} else
burst_mode_ratio = 100;
 
-- 
2.40.0.1.gaa8946217a0b

Re: [Intel-gfx] [rft, PATCH v3 00/15] drm/i915/dsi: 2nd attempt to get rid of IOSF GPIO

[Andy Shevchenko]

On Thu, Nov 02, 2023 at 05:12:13PM +0200, Andy Shevchenko wrote:
> DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
> talking to GPIO IP behind the actual driver's back. A second attempt
> to fix that is here.
> 
> If I understood correctly, my approach should work in the similar way as
> the current IOSF GPIO.
> 
> Hans, I believe you have some devices that use this piece of code,
> is it possible to give a test run on (one of) them?

Subject should be "3rd attempt ..." :-)

> In v3:
> - incorporated series by Jani
> - incorporated couple of precursor patches by Hans
> - added Rb tag for used to be first three patches (Andi)
> - rebased on top of the above changes
> - fixed indexing for multi-community devices, such as Cherry View
> 
> In v2:
> - added a few cleanup patches
> - reworked to use dynamic GPIO lookup tables
> - converted CHV as well


-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v3 10/15] drm/i915/dsi: Fix wrong initial value for GPIOs in bxt_exec_gpio()

[Andy Shevchenko]

From: Hans de Goede 

Fix wrong initial value for GPIOs in bxt_exec_gpio().

Signed-off-by: Hans de Goede 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8e6beef90e5e..0f9da0168a7b 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -356,9 +356,7 @@ static void bxt_gpio_set_value(struct intel_connector 
*connector,
if (!gpio_desc) {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
 NULL, gpio_index,
-value ? GPIOD_OUT_LOW :
-GPIOD_OUT_HIGH);
-
+value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
if (IS_ERR_OR_NULL(gpio_desc)) {
drm_err(_priv->drm,
"GPIO index %u request failed (%ld)\n",
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [rft, PATCH v3 00/15] drm/i915/dsi: 2nd attempt to get rid of IOSF GPIO

[Andy Shevchenko]

DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
talking to GPIO IP behind the actual driver's back. A second attempt
to fix that is here.

If I understood correctly, my approach should work in the similar way as
the current IOSF GPIO.

Hans, I believe you have some devices that use this piece of code,
is it possible to give a test run on (one of) them?

In v3:
- incorporated series by Jani
- incorporated couple of precursor patches by Hans
- added Rb tag for used to be first three patches (Andi)
- rebased on top of the above changes
- fixed indexing for multi-community devices, such as Cherry View

In v2:
- added a few cleanup patches
- reworked to use dynamic GPIO lookup tables
- converted CHV as well

Andy Shevchenko (8):
  drm/i915/dsi: Replace while(1) with one with clear exit condition
  drm/i915/dsi: Get rid of redundant 'else'
  drm/i915/dsi: Replace check with a (missing) MIPI sequence name
  drm/i915/dsi: Extract common soc_gpio_set_value() helper
  drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back
  drm/i915/dsi: Prepare soc_gpio_set_value() to distinguish GPIO
communities
  drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
  drm/i915/iosf: Drop unused APIs

Hans de Goede (2):
  drm/i915/dsi: Remove GPIO lookup table at the end of
intel_dsi_vbt_gpio_init()
  drm/i915/dsi: Fix wrong initial value for GPIOs in bxt_exec_gpio()

Jani Nikula (5):
  drm/i915/dsi: assume BXT gpio works for non-native GPIO
  drm/i915/dsi: switch mipi_exec_gpio() from dev_priv to i915
  drm/i915/dsi: clarify GPIO exec sequence
  drm/i915/dsi: rename platform specific *_exec_gpio() to
*_gpio_set_value()
  drm/i915/dsi: bxt/icl GPIO set value do not need gpio source

 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 355 +++
 drivers/gpu/drm/i915/vlv_sideband.c  |  17 -
 drivers/gpu/drm/i915/vlv_sideband.h  |   3 -
 3 files changed, 137 insertions(+), 238 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 06/15] drm/i915/dsi: Replace while(1) with one with clear exit condition

[Andy Shevchenko]

Move existing condition to while(), so it will be clear on what
circumstances the loop is successfully finishing.

Reviewed-by: Andi Shyti 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 4af43cf3cee0..290a112f1b63 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -702,13 +702,10 @@ static void intel_dsi_vbt_exec(struct intel_dsi 
*intel_dsi,
if (connector->panel.vbt.dsi.seq_version >= 3)
data += 4;
 
-   while (1) {
+   while (*data != MIPI_SEQ_ELEM_END) {
u8 operation_byte = *data++;
u8 operation_size = 0;
 
-   if (operation_byte == MIPI_SEQ_ELEM_END)
-   break;
-
if (operation_byte < ARRAY_SIZE(exec_elem))
mipi_elem_exec = exec_elem[operation_byte];
else
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 08/15] drm/i915/dsi: Replace check with a (missing) MIPI sequence name

[Andy Shevchenko]

Names of the MIPI sequence steps are sequential and defined, no
need to check for the gaps. However in seq_name the MIPI_SEQ_END
is missing. Add it there, and drop unneeded NULL check in
sequence_name().

Reviewed-by: Andi Shyti 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 4ed5ede9ec5b..d270437217b3 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -653,6 +653,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
  */
 
 static const char * const seq_name[] = {
+   [MIPI_SEQ_END] = "MIPI_SEQ_END",
[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
@@ -668,7 +669,7 @@ static const char * const seq_name[] = {
 
 static const char *sequence_name(enum mipi_seq seq_id)
 {
-   if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
+   if (seq_id < ARRAY_SIZE(seq_name))
return seq_name[seq_id];
 
return "(unknown)";
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 13/15] drm/i915/dsi: Prepare soc_gpio_set_value() to distinguish GPIO communities

[Andy Shevchenko]

Currently soc_gpio_set_value() supports only a single indexing for GPIO pin.
For CHV case, for example, we will need to distinguish community based index
from the one that VBT is using. Introduce an additional parameter to
soc_gpio_set_value() and its callers.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 552bc6564d79..b1736c1301ea 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -206,8 +206,8 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
-static void soc_gpio_set_value(struct intel_connector *connector, const char 
*con_id,
-  u8 gpio_index, bool value)
+static void soc_gpio_set_value(struct intel_connector *connector, u8 
gpio_index,
+  const char *con_id, u8 idx, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
/* XXX: this table is a quick ugly hack. */
@@ -217,8 +217,7 @@ static void soc_gpio_set_value(struct intel_connector 
*connector, const char *co
if (gpio_desc) {
gpiod_set_value(gpio_desc, value);
} else {
-   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-con_id, gpio_index,
+   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev, con_id, idx,
 value ? GPIOD_OUT_HIGH : 
GPIOD_OUT_LOW);
if (IS_ERR(gpio_desc)) {
drm_err(_priv->drm,
@@ -232,8 +231,8 @@ static void soc_gpio_set_value(struct intel_connector 
*connector, const char *co
 }
 
 static void soc_opaque_gpio_set_value(struct intel_connector *connector,
- const char *chip, const char *con_id,
- u8 gpio_index, bool value)
+ u8 gpio_index, const char *chip,
+ const char *con_id, u8 idx, bool value)
 {
struct gpiod_lookup_table *lookup;
 
@@ -243,11 +242,11 @@ static void soc_opaque_gpio_set_value(struct 
intel_connector *connector,
 
lookup->dev_id = ":00:02.0";
lookup->table[0] =
-   GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index, 
GPIO_ACTIVE_HIGH);
+   GPIO_LOOKUP_IDX(chip, idx, con_id, idx, GPIO_ACTIVE_HIGH);
 
gpiod_add_lookup_table(lookup);
 
-   soc_gpio_set_value(connector, con_id, gpio_index, value);
+   soc_gpio_set_value(connector, gpio_index, con_id, idx, value);
 
gpiod_remove_lookup_table(lookup);
kfree(lookup);
@@ -271,7 +270,8 @@ static void vlv_gpio_set_value(struct intel_connector 
*connector,
}
}
 
-   soc_opaque_gpio_set_value(connector, "INT33FC:01", "Panel N", 
gpio_index, value);
+   soc_opaque_gpio_set_value(connector, gpio_index,
+ "INT33FC:01", "Panel N", gpio_index, value);
 }
 
 static void chv_gpio_set_value(struct intel_connector *connector,
@@ -331,7 +331,7 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
 static void bxt_gpio_set_value(struct intel_connector *connector,
   u8 gpio_index, bool value)
 {
-   soc_gpio_set_value(connector, NULL, gpio_index, value);
+   soc_gpio_set_value(connector, gpio_index, NULL, gpio_index, value);
 }
 
 enum {
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 09/15] drm/i915/dsi: Remove GPIO lookup table at the end of intel_dsi_vbt_gpio_init()

[Andy Shevchenko]

From: Hans de Goede 

To properly deal with GPIOs used in MIPI panel sequences a temporary
GPIO lookup will be used. Since there can only be 1 GPIO lookup table
for the ":00:02.0" device this will not work if the GPIO lookup
table used by intel_dsi_vbt_gpio_init() is still registered.

After getting the "backlight" and "panel" GPIOs the lookup table
registered by intel_dsi_vbt_gpio_init() is no longer necessary,
remove it so that another temporary lookup-table for the ":00:02.0"
device can be added.

Signed-off-by: Hans de Goede 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 25 +++-
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index d270437217b3..8e6beef90e5e 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -955,6 +955,7 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, 
bool panel_is_on)
struct intel_connector *connector = intel_dsi->attached_connector;
struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
enum gpiod_flags flags = panel_is_on ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
+   struct gpiod_lookup_table *gpiod_lookup_table = NULL;
bool want_backlight_gpio = false;
bool want_panel_gpio = false;
struct pinctrl *pinctrl;
@@ -962,12 +963,12 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, 
bool panel_is_on)
 
if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
mipi_config->pwm_blc == PPS_BLC_PMIC) {
-   gpiod_add_lookup_table(_panel_gpio_table);
+   gpiod_lookup_table = _panel_gpio_table;
want_panel_gpio = true;
}
 
if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
-   gpiod_add_lookup_table(_panel_gpio_table);
+   gpiod_lookup_table = _panel_gpio_table;
want_panel_gpio = true;
want_backlight_gpio = true;
 
@@ -984,6 +985,9 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, 
bool panel_is_on)
"Failed to set pinmux to PWM\n");
}
 
+   if (gpiod_lookup_table)
+   gpiod_add_lookup_table(gpiod_lookup_table);
+
if (want_panel_gpio) {
intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
if (IS_ERR(intel_dsi->gpio_panel)) {
@@ -1002,15 +1006,13 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi 
*intel_dsi, bool panel_is_on)
intel_dsi->gpio_backlight = NULL;
}
}
+
+   if (gpiod_lookup_table)
+   gpiod_remove_lookup_table(gpiod_lookup_table);
 }
 
 void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
 {
-   struct drm_device *dev = intel_dsi->base.base.dev;
-   struct drm_i915_private *dev_priv = to_i915(dev);
-   struct intel_connector *connector = intel_dsi->attached_connector;
-   struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
-
if (intel_dsi->gpio_panel) {
gpiod_put(intel_dsi->gpio_panel);
intel_dsi->gpio_panel = NULL;
@@ -1020,13 +1022,4 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi 
*intel_dsi)
gpiod_put(intel_dsi->gpio_backlight);
intel_dsi->gpio_backlight = NULL;
}
-
-   if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
-   mipi_config->pwm_blc == PPS_BLC_PMIC)
-   gpiod_remove_lookup_table(_panel_gpio_table);
-
-   if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
-   pinctrl_unregister_mappings(soc_pwm_pinctrl_map);
-   gpiod_remove_lookup_table(_panel_gpio_table);
-   }
 }
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 05/15] drm/i915/dsi: bxt/icl GPIO set value do not need gpio source

[Andy Shevchenko]

From: Jani Nikula 

Drop the unused parameter.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index f977d63a0ad4..4af43cf3cee0 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -346,7 +346,7 @@ static void chv_gpio_set_value(struct intel_connector 
*connector,
 }
 
 static void bxt_gpio_set_value(struct intel_connector *connector,
-  u8 gpio_source, u8 gpio_index, bool value)
+  u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
/* XXX: this table is a quick ugly hack. */
@@ -486,13 +486,13 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(i915, gpio_number, value);
else if (DISPLAY_VER(i915) >= 11)
-   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_index, value);
else if (IS_VALLEYVIEW(i915))
vlv_gpio_set_value(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(i915))
chv_gpio_set_value(connector, gpio_source, gpio_number, value);
else
-   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_index, value);
 
return data + size;
 }
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 01/15] drm/i915/dsi: assume BXT gpio works for non-native GPIO

[Andy Shevchenko]

From: Jani Nikula 

Purely a guess. Drop the nop function.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 24b2cbcfc1ef..b2c0cc11f8c1 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -372,14 +372,6 @@ static void bxt_exec_gpio(struct intel_connector 
*connector,
gpiod_set_value(gpio_desc, value);
 }
 
-static void icl_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
-{
-   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-
-   drm_dbg_kms(_priv->drm, "Skipping ICL GPIO element execution\n");
-}
-
 enum {
MIPI_RESET_1 = 0,
MIPI_AVDD_EN_1,
@@ -491,7 +483,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(dev_priv, gpio_number, value);
else if (DISPLAY_VER(dev_priv) >= 11)
-   icl_exec_gpio(connector, gpio_source, gpio_index, value);
+   bxt_exec_gpio(connector, gpio_source, gpio_index, value);
else if (IS_VALLEYVIEW(dev_priv))
vlv_exec_gpio(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(dev_priv))
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 02/15] drm/i915/dsi: switch mipi_exec_gpio() from dev_priv to i915

[Andy Shevchenko]

From: Jani Nikula 

Follow the contemporary conventions.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index b2c0cc11f8c1..8b962f2ac475 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -454,11 +454,11 @@ static void icl_native_gpio_set_value(struct 
drm_i915_private *dev_priv,
 static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
 {
struct drm_device *dev = intel_dsi->base.base.dev;
-   struct drm_i915_private *dev_priv = to_i915(dev);
+   struct drm_i915_private *i915 = to_i915(dev);
struct intel_connector *connector = intel_dsi->attached_connector;
u8 gpio_source, gpio_index = 0, gpio_number;
bool value;
-   bool native = DISPLAY_VER(dev_priv) >= 11;
+   bool native = DISPLAY_VER(i915) >= 11;
 
if (connector->panel.vbt.dsi.seq_version >= 3)
gpio_index = *data++;
@@ -477,16 +477,16 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
/* pull up/down */
value = *data++ & 1;
 
-   drm_dbg_kms(_priv->drm, "GPIO index %u, number %u, source %u, 
native %s, set to %s\n",
+   drm_dbg_kms(>drm, "GPIO index %u, number %u, source %u, native 
%s, set to %s\n",
gpio_index, gpio_number, gpio_source, str_yes_no(native), 
str_on_off(value));
 
if (native)
-   icl_native_gpio_set_value(dev_priv, gpio_number, value);
-   else if (DISPLAY_VER(dev_priv) >= 11)
+   icl_native_gpio_set_value(i915, gpio_number, value);
+   else if (DISPLAY_VER(i915) >= 11)
bxt_exec_gpio(connector, gpio_source, gpio_index, value);
-   else if (IS_VALLEYVIEW(dev_priv))
+   else if (IS_VALLEYVIEW(i915))
vlv_exec_gpio(connector, gpio_source, gpio_number, value);
-   else if (IS_CHERRYVIEW(dev_priv))
+   else if (IS_CHERRYVIEW(i915))
chv_exec_gpio(connector, gpio_source, gpio_number, value);
else
bxt_exec_gpio(connector, gpio_source, gpio_index, value);
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v3 04/15] drm/i915/dsi: rename platform specific *_exec_gpio() to *_gpio_set_value()

[Andy Shevchenko]

From: Jani Nikula 

The lowest level functions are about setting GPIO values, not about
executing any sequences anymore.

Cc: Andy Shevchenko 
Cc: Hans de Goede 
Signed-off-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 11073efe26c0..f977d63a0ad4 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -243,8 +243,8 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
-static void vlv_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
+static void vlv_gpio_set_value(struct intel_connector *connector,
+  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct gpio_map *map;
@@ -291,8 +291,8 @@ static void vlv_exec_gpio(struct intel_connector *connector,
vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
-static void chv_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
+static void chv_gpio_set_value(struct intel_connector *connector,
+  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
u16 cfg0, cfg1;
@@ -345,8 +345,8 @@ static void chv_exec_gpio(struct intel_connector *connector,
vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
-static void bxt_exec_gpio(struct intel_connector *connector,
- u8 gpio_source, u8 gpio_index, bool value)
+static void bxt_gpio_set_value(struct intel_connector *connector,
+  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
/* XXX: this table is a quick ugly hack. */
@@ -486,13 +486,13 @@ static const u8 *mipi_exec_gpio(struct intel_dsi 
*intel_dsi, const u8 *data)
if (native)
icl_native_gpio_set_value(i915, gpio_number, value);
else if (DISPLAY_VER(i915) >= 11)
-   bxt_exec_gpio(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
else if (IS_VALLEYVIEW(i915))
-   vlv_exec_gpio(connector, gpio_source, gpio_number, value);
+   vlv_gpio_set_value(connector, gpio_source, gpio_number, value);
else if (IS_CHERRYVIEW(i915))
-   chv_exec_gpio(connector, gpio_source, gpio_number, value);
+   chv_gpio_set_value(connector, gpio_source, gpio_number, value);
else
-   bxt_exec_gpio(connector, gpio_source, gpio_index, value);
+   bxt_gpio_set_value(connector, gpio_source, gpio_index, value);
 
return data + size;
 }
-- 
2.40.0.1.gaa8946217a0b

Re: [Intel-gfx] [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

On Wed, Nov 01, 2023 at 11:20:23AM +0100, Hans de Goede wrote:
> On 11/1/23 10:32, Andy Shevchenko wrote:
> > On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
> >> On 10/31/23 17:07, Hans de Goede wrote:
> >>> On 10/24/23 18:11, Andy Shevchenko wrote:
> >>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:

...

> Note you still need the first part of my patch which is
> an unrelated bugfix:
> 
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector 
> *connector, const char *con_id,
>   } else {
>   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
>con_id, gpio_index,
> -  value ? GPIOD_OUT_LOW :
> -  GPIOD_OUT_HIGH);
> +  value ? GPIOD_OUT_HIGH : 
> GPIOD_OUT_LOW);
>   if (IS_ERR(gpio_desc)) {
>   drm_err(_priv->drm,
>   "GPIO index %u request failed (%pe)\n",

Can you attach or send a formal submission, so I can incorporate it into one
(v3) series among other changes?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 5/5] drm/i915/dsi: bxt/icl GPIO set value do not need gpio source

[Andy Shevchenko]

On Wed, Nov 01, 2023 at 12:16:20PM +0200, Jani Nikula wrote:

In mine not published yet series it will be used for some of them IIRC.
I prefer to postpone this last patch.

The rest AFAIU you want to go before any rework done.
So, I can collect these into my v3 including Hans'
change.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

On Wed, Nov 01, 2023 at 12:01:31PM +0100, Hans de Goede wrote:
> On 11/1/23 11:20, Hans de Goede wrote:

...

> Attached is this patch, this should probably be one of
> the first patches in the v3 submission.

Thanks, noted!

> Note that if you go with Ville's suggestion to preparse
> the MIPI sequences, things will change significantly
> and then the attached patch will likely be unnecessary.

I don't think so I'm for that. My task is to get rid of the poking registers
of the GPIO IPs in the kernel when driver has no clue about them.

That's why I want to do minimum in that sense with less possible invasion
into existing flow.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
> On 10/31/23 17:07, Hans de Goede wrote:
> > On 10/24/23 18:11, Andy Shevchenko wrote:
> >> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:

...

> > As for the CHT support, I have not added that to my tree yet, I would
> > prefer to directly test the correct/fixed patch.
> 
> And I hit the "jackpot" on the first device I tried and the code needed
> some fixing to actually work, so here is something to fold into v3 to
> fix things:

Thanks!

But let me first send current v3 as it quite differs to v2 in the sense
of how I do instantiate GPIO lookup tables.

Meanwhile I will look into the change you sent (and hopefully we can
incorporate something in v3 for v4).

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

On Tue, Oct 31, 2023 at 05:07:39PM +0100, Hans de Goede wrote:
> On 10/24/23 18:11, Andy Shevchenko wrote:
> > On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
> >> It's a dirty hack in the driver that pokes GPIO registers behind
> >> the driver's back. Moreoever it might be problematic as simultaneous
> >> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
> >> cherryview: prevent concurrent access to GPIO controllers") for
> >> the details. Taking all this into consideration replace the hack
> >> with proper GPIO APIs being used.
> > 
> > Ah, just realised that this won't work if it happens to request to GPIOs 
> > with
> > the same index but different communities. I will fix that in v3, but will 
> > wait
> > for Hans to test VLV and it might even work in most of the cases on CHV as 
> > it
> > seems quite unlikely that the above mentioned assertion is going to happen 
> > in
> > real life.
> 
> I have added patches 1-5 to my personal tree + a small debug patch on top
> which logs when soc_exec_opaque_gpio() actually gets called.
> 
> So these patches will now get run every time I run some tests on
> one my tablets.
> 
> I'll get back to you with testing results when I've found a device where
> the new soc_exec_opaque_gpio() actually gets called.

Thank you!

> As for the CHT support, I have not added that to my tree yet, I would
> prefer to directly test the correct/fixed patch.

Noted, I'll prepare a new version then.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
> It's a dirty hack in the driver that pokes GPIO registers behind
> the driver's back. Moreoever it might be problematic as simultaneous
> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
> cherryview: prevent concurrent access to GPIO controllers") for
> the details. Taking all this into consideration replace the hack
> with proper GPIO APIs being used.

Ah, just realised that this won't work if it happens to request to GPIOs with
the same index but different communities. I will fix that in v3, but will wait
for Hans to test VLV and it might even work in most of the cases on CHV as it
seems quite unlikely that the above mentioned assertion is going to happen in
real life.

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v2 2/7] drm/i915/dsi: Get rid of redundant 'else'

[Andy Shevchenko]

In the snippets like the following

if (...)
return / goto / break / continue ...;
else
...

the 'else' is redundant. Get rid of it.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 58 ++--
 1 file changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index a6a6f1814967..22b89e68e6de 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -142,7 +142,7 @@ static enum port intel_dsi_seq_port_to_port(struct 
intel_dsi *intel_dsi,
if (seq_port) {
if (intel_dsi->ports & BIT(PORT_B))
return PORT_B;
-   else if (intel_dsi->ports & BIT(PORT_C))
+   if (intel_dsi->ports & BIT(PORT_C))
return PORT_C;
}
 
@@ -675,8 +675,8 @@ static const char *sequence_name(enum mipi_seq seq_id)
 {
if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
return seq_name[seq_id];
-   else
-   return "(unknown)";
+
+   return "(unknown)";
 }
 
 static void intel_dsi_vbt_exec(struct intel_dsi *intel_dsi,
@@ -870,36 +870,34 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 
panel_id)
 * multiply by 100 to preserve remainder
 */
if (intel_dsi->video_mode == BURST_MODE) {
-   if (mipi_config->target_burst_mode_freq) {
-   u32 bitrate = intel_dsi_bitrate(intel_dsi);
+   u32 bitrate;
 
-   /*
-* Sometimes the VBT contains a slightly lower clock,
-* then the bitrate we have calculated, in this case
-* just replace it with the calculated bitrate.
-*/
-   if (mipi_config->target_burst_mode_freq < bitrate &&
-   intel_fuzzy_clock_check(
-   mipi_config->target_burst_mode_freq,
-   bitrate))
-   mipi_config->target_burst_mode_freq = bitrate;
-
-   if (mipi_config->target_burst_mode_freq < bitrate) {
-   drm_err(_priv->drm,
-   "Burst mode freq is less than 
computed\n");
-   return false;
-   }
-
-   burst_mode_ratio = DIV_ROUND_UP(
-   mipi_config->target_burst_mode_freq * 100,
-   bitrate);
-
-   intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * 
burst_mode_ratio, 100);
-   } else {
-   drm_err(_priv->drm,
-   "Burst mode target is not set\n");
+   if (mipi_config->target_burst_mode_freq == 0) {
+   drm_err(_priv->drm, "Burst mode target is not 
set\n");
return false;
}
+
+   bitrate = intel_dsi_bitrate(intel_dsi);
+
+   /*
+* Sometimes the VBT contains a slightly lower clock, then
+* the bitrate we have calculated, in this case just replace it
+* with the calculated bitrate.
+*/
+   if (mipi_config->target_burst_mode_freq < bitrate &&
+   intel_fuzzy_clock_check(mipi_config->target_burst_mode_freq,
+   bitrate))
+   mipi_config->target_burst_mode_freq = bitrate;
+
+   if (mipi_config->target_burst_mode_freq < bitrate) {
+   drm_err(_priv->drm, "Burst mode freq is less than 
computed\n");
+   return false;
+   }
+
+   burst_mode_ratio =
+   DIV_ROUND_UP(mipi_config->target_burst_mode_freq * 100, 
bitrate);
+
+   intel_dsi->pclk = DIV_ROUND_UP(intel_dsi->pclk * 
burst_mode_ratio, 100);
} else
burst_mode_ratio = 100;
 
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [rft, PATCH v2 0/7] drm/i915/dsi: 2nd attempt to get rid of IOSF GPIO

[Andy Shevchenko]

DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
talking to GPIO IP behind the actual driver's back. A second attempt
to fix that is here.

If I understood correctly, my approach should work in the similar way as
the current IOSF GPIO.

Hans, I believe you have some devices that use this piece of code,
is it possible to give a test run on (one of) them?

In v2:
- added a few cleanup patches
- reworked to use dynamic GPIO lookup tables
- converted CHV as well

Andy Shevchenko (7):
  drm/i915/dsi: Replace while(1) with one with clear exit condition
  drm/i915/dsi: Get rid of redundant 'else'
  drm/i915/dsi: Replace check with a (missing) MIPI sequence name
  drm/i915/dsi: Extract common soc_gpio_exec() helper
  drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back
  drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
  drm/i915/iosf: Drop unused APIs

 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 260 +++
 drivers/gpu/drm/i915/vlv_sideband.c  |  17 --
 drivers/gpu/drm/i915/vlv_sideband.h  |   3 -
 3 files changed, 96 insertions(+), 184 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v2 5/7] drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back

[Andy Shevchenko]

It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 40ecab551232 ("pinctrl:
baytrail: Really serialize all register accesses") for the details.
Taking all this into consideration replace the hack with proper
GPIO APIs being used.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 98 ++--
 1 file changed, 28 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 3fb85b6d320e..8fc82aceae14 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -55,43 +55,6 @@
 #define MIPI_VIRTUAL_CHANNEL_SHIFT 1
 #define MIPI_PORT_SHIFT3
 
-/* base offsets for gpio pads */
-#define VLV_GPIO_NC_0_HV_DDI0_HPD  0x4130
-#define VLV_GPIO_NC_1_HV_DDI0_DDC_SDA  0x4120
-#define VLV_GPIO_NC_2_HV_DDI0_DDC_SCL  0x4110
-#define VLV_GPIO_NC_3_PANEL0_VDDEN 0x4140
-#define VLV_GPIO_NC_4_PANEL0_BKLTEN0x4150
-#define VLV_GPIO_NC_5_PANEL0_BKLTCTL   0x4160
-#define VLV_GPIO_NC_6_HV_DDI1_HPD  0x4180
-#define VLV_GPIO_NC_7_HV_DDI1_DDC_SDA  0x4190
-#define VLV_GPIO_NC_8_HV_DDI1_DDC_SCL  0x4170
-#define VLV_GPIO_NC_9_PANEL1_VDDEN 0x4100
-#define VLV_GPIO_NC_10_PANEL1_BKLTEN   0x40E0
-#define VLV_GPIO_NC_11_PANEL1_BKLTCTL  0x40F0
-
-#define VLV_GPIO_PCONF0(base_offset)   (base_offset)
-#define VLV_GPIO_PAD_VAL(base_offset)  ((base_offset) + 8)
-
-struct gpio_map {
-   u16 base_offset;
-   bool init;
-};
-
-static struct gpio_map vlv_gpio_table[] = {
-   { VLV_GPIO_NC_0_HV_DDI0_HPD },
-   { VLV_GPIO_NC_1_HV_DDI0_DDC_SDA },
-   { VLV_GPIO_NC_2_HV_DDI0_DDC_SCL },
-   { VLV_GPIO_NC_3_PANEL0_VDDEN },
-   { VLV_GPIO_NC_4_PANEL0_BKLTEN },
-   { VLV_GPIO_NC_5_PANEL0_BKLTCTL },
-   { VLV_GPIO_NC_6_HV_DDI1_HPD },
-   { VLV_GPIO_NC_7_HV_DDI1_DDC_SDA },
-   { VLV_GPIO_NC_8_HV_DDI1_DDC_SCL },
-   { VLV_GPIO_NC_9_PANEL1_VDDEN },
-   { VLV_GPIO_NC_10_PANEL1_BKLTEN },
-   { VLV_GPIO_NC_11_PANEL1_BKLTCTL },
-};
-
 struct i2c_adapter_lookup {
u16 slave_addr;
struct intel_dsi *intel_dsi;
@@ -269,52 +232,47 @@ static void soc_exec_gpio(struct intel_connector 
*connector, const char *con_id,
}
 }
 
+static void soc_exec_opaque_gpio(struct intel_connector *connector,
+const char *chip, const char *con_id,
+u8 gpio_index, bool value)
+{
+   struct gpiod_lookup_table *lookup;
+
+   lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
+   if (!lookup)
+   return;
+
+   lookup->dev_id = ":00:02.0";
+   lookup->table[0] =
+   GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index, 
GPIO_ACTIVE_HIGH);
+
+   gpiod_add_lookup_table(lookup);
+
+   soc_exec_gpio(connector, con_id, gpio_index, value);
+
+   gpiod_remove_lookup_table(lookup);
+   kfree(lookup);
+}
+
 static void vlv_exec_gpio(struct intel_connector *connector,
  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   struct gpio_map *map;
-   u16 pconf0, padval;
-   u32 tmp;
-   u8 port;
 
-   if (gpio_index >= ARRAY_SIZE(vlv_gpio_table)) {
-   drm_dbg_kms(_priv->drm, "unknown gpio index %u\n",
-   gpio_index);
-   return;
-   }
-
-   map = _gpio_table[gpio_index];
-
-   if (connector->panel.vbt.dsi.seq_version >= 3) {
-   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
-   port = IOSF_PORT_GPIO_NC;
-   } else {
-   if (gpio_source == 0) {
-   port = IOSF_PORT_GPIO_NC;
-   } else if (gpio_source == 1) {
+   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
+   if (connector->panel.vbt.dsi.seq_version < 3) {
+   if (gpio_source == 1) {
drm_dbg_kms(_priv->drm, "SC gpio not supported\n");
return;
-   } else {
+   }
+   if (gpio_source > 1) {
drm_dbg_kms(_priv->drm,
"unknown gpio source %u\n", gpio_source);
return;
}
}
 
-   pconf0 = VLV_GPIO_PCONF0(map->base_offset);
-   padval = VLV_GPIO_PAD_VAL(map->base_offset);
-
-   vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-   if (!map->init) {
-   /* FIXME: remove constant below */
-   vlv_iosf_sb_write(dev_priv, port, pconf0, 0x2000CC00);
-   map->init = true;
-   }
-
-   

Re: [Intel-gfx] [rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[Andy Shevchenko]

On Wed, Oct 18, 2023 at 03:52:36PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 18, 2023 at 11:09:35AM +0200, Hans de Goede wrote:
> > On 10/18/23 07:10, Andy Shevchenko wrote:

...

> > Yes I should be able to find a device or 2 which poke GPIOs from the
> > VBT MIPI sequences. Unfortunately I don't know from the top of my head
> > which devices actually use this, so I may need to try quite a few devices
> > before finding one which actually uses this.
> > 
> > I'll try to get this series tested sometime the coming weeks,
> > depending on when I can schedule some time for this.
> 
> No hurry. maybe you simply can add into your usual tree you run on your
> devices?

FYI, I have just sent a v2, which includes CHV conversion.

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v2 4/7] drm/i915/dsi: Extract common soc_gpio_exec() helper

[Andy Shevchenko]

Extract a common soc_gpio_exec() helper that may be used by a few SoCs.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 49 +++-
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 1014051a6866..3fb85b6d320e 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -243,6 +243,32 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
+static void soc_exec_gpio(struct intel_connector *connector, const char 
*con_id,
+ u8 gpio_index, bool value)
+{
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   /* XXX: this table is a quick ugly hack. */
+   static struct gpio_desc *soc_gpio_table[U8_MAX + 1];
+   struct gpio_desc *gpio_desc = soc_gpio_table[gpio_index];
+
+   if (gpio_desc) {
+   gpiod_set_value(gpio_desc, value);
+   } else {
+   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
+con_id, gpio_index,
+value ? GPIOD_OUT_LOW :
+GPIOD_OUT_HIGH);
+   if (IS_ERR(gpio_desc)) {
+   drm_err(_priv->drm,
+   "GPIO index %u request failed (%pe)\n",
+   gpio_index, gpio_desc);
+   return;
+   }
+
+   soc_gpio_table[gpio_index] = gpio_desc;
+   }
+}
+
 static void vlv_exec_gpio(struct intel_connector *connector,
  u8 gpio_source, u8 gpio_index, bool value)
 {
@@ -348,28 +374,7 @@ static void chv_exec_gpio(struct intel_connector 
*connector,
 static void bxt_exec_gpio(struct intel_connector *connector,
  u8 gpio_source, u8 gpio_index, bool value)
 {
-   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   /* XXX: this table is a quick ugly hack. */
-   static struct gpio_desc *bxt_gpio_table[U8_MAX + 1];
-   struct gpio_desc *gpio_desc = bxt_gpio_table[gpio_index];
-
-   if (!gpio_desc) {
-   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-NULL, gpio_index,
-value ? GPIOD_OUT_LOW :
-GPIOD_OUT_HIGH);
-
-   if (IS_ERR_OR_NULL(gpio_desc)) {
-   drm_err(_priv->drm,
-   "GPIO index %u request failed (%ld)\n",
-   gpio_index, PTR_ERR(gpio_desc));
-   return;
-   }
-
-   bxt_gpio_table[gpio_index] = gpio_desc;
-   }
-
-   gpiod_set_value(gpio_desc, value);
+   soc_exec_gpio(connector, NULL, gpio_index, value);
 }
 
 static void icl_exec_gpio(struct intel_connector *connector,
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v2 6/7] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

[Andy Shevchenko]

It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
cherryview: prevent concurrent access to GPIO controllers") for
the details. Taking all this into consideration replace the hack
with proper GPIO APIs being used.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +---
 1 file changed, 10 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8fc82aceae14..a393ddaff0dd 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
 #define CHV_GPIO_IDX_START_SW  100
 #define CHV_GPIO_IDX_START_SE  198
 
-#define CHV_VBT_MAX_PINS_PER_FMLY  15
-
-#define CHV_GPIO_PAD_CFG0(f, i)(0x4400 + (f) * 0x400 + (i) * 8)
-#define  CHV_GPIO_GPIOEN   (1 << 15)
-#define  CHV_GPIO_GPIOCFG_GPIO (0 << 8)
-#define  CHV_GPIO_GPIOCFG_GPO  (1 << 8)
-#define  CHV_GPIO_GPIOCFG_GPI  (2 << 8)
-#define  CHV_GPIO_GPIOCFG_HIZ  (3 << 8)
-#define  CHV_GPIO_GPIOTXSTATE(state)   ((!!(state)) << 1)
-
-#define CHV_GPIO_PAD_CFG1(f, i)(0x4400 + (f) * 0x400 + (i) * 8 
+ 4)
-#define  CHV_GPIO_CFGLOCK  (1 << 31)
-
 /* ICL DSI Display GPIO Pins */
 #define  ICL_GPIO_DDSP_HPD_A   0
 #define  ICL_GPIO_L_VDDEN_11
@@ -279,23 +266,21 @@ static void chv_exec_gpio(struct intel_connector 
*connector,
  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   u16 cfg0, cfg1;
-   u16 family_num;
-   u8 port;
 
if (connector->panel.vbt.dsi.seq_version >= 3) {
if (gpio_index >= CHV_GPIO_IDX_START_SE) {
/* XXX: it's unclear whether 255->57 is part of SE. */
-   gpio_index -= CHV_GPIO_IDX_START_SE;
-   port = CHV_IOSF_PORT_GPIO_SE;
+   soc_exec_opaque_gpio(connector, "INT33FF:03", "Panel 
SE",
+gpio_index - 
CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
-   gpio_index -= CHV_GPIO_IDX_START_SW;
-   port = CHV_IOSF_PORT_GPIO_SW;
+   soc_exec_opaque_gpio(connector, "INT33FF:00", "Panel 
SW",
+gpio_index - 
CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
-   gpio_index -= CHV_GPIO_IDX_START_E;
-   port = CHV_IOSF_PORT_GPIO_E;
+   soc_exec_opaque_gpio(connector, "INT33FF:02", "Panel E",
+gpio_index - CHV_GPIO_IDX_START_E, 
value);
} else {
-   port = CHV_IOSF_PORT_GPIO_N;
+   soc_exec_opaque_gpio(connector, "INT33FF:01", "Panel N",
+gpio_index - CHV_GPIO_IDX_START_N, 
value);
}
} else {
/* XXX: The spec is unclear about CHV GPIO on seq v2 */
@@ -312,21 +297,9 @@ static void chv_exec_gpio(struct intel_connector 
*connector,
return;
}
 
-   port = CHV_IOSF_PORT_GPIO_N;
+   soc_exec_opaque_gpio(connector, "INT33FF:01", "Panel N",
+gpio_index - CHV_GPIO_IDX_START_N, value);
}
-
-   family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
-   gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
-
-   cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
-   cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
-
-   vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-   vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
-   vlv_iosf_sb_write(dev_priv, port, cfg0,
- CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
- CHV_GPIO_GPIOTXSTATE(value));
-   vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
 static void bxt_exec_gpio(struct intel_connector *connector,
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v2 7/7] drm/i915/iosf: Drop unused APIs

[Andy Shevchenko]

Drop unused vlv_iosf_sb_read() and vlv_iosf_sb_write().

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/vlv_sideband.c | 17 -
 drivers/gpu/drm/i915/vlv_sideband.h |  3 ---
 2 files changed, 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/vlv_sideband.c 
b/drivers/gpu/drm/i915/vlv_sideband.c
index b98dec3ad817..13b644958e38 100644
--- a/drivers/gpu/drm/i915/vlv_sideband.c
+++ b/drivers/gpu/drm/i915/vlv_sideband.c
@@ -166,23 +166,6 @@ u32 vlv_nc_read(struct drm_i915_private *i915, u8 addr)
return val;
 }
 
-u32 vlv_iosf_sb_read(struct drm_i915_private *i915, u8 port, u32 reg)
-{
-   u32 val = 0;
-
-   vlv_sideband_rw(i915, PCI_DEVFN(0, 0), port,
-   SB_CRRDDA_NP, reg, );
-
-   return val;
-}
-
-void vlv_iosf_sb_write(struct drm_i915_private *i915,
-  u8 port, u32 reg, u32 val)
-{
-   vlv_sideband_rw(i915, PCI_DEVFN(0, 0), port,
-   SB_CRWRDA_NP, reg, );
-}
-
 u32 vlv_cck_read(struct drm_i915_private *i915, u32 reg)
 {
u32 val = 0;
diff --git a/drivers/gpu/drm/i915/vlv_sideband.h 
b/drivers/gpu/drm/i915/vlv_sideband.h
index 9ce283d96b80..8b4495e14bce 100644
--- a/drivers/gpu/drm/i915/vlv_sideband.h
+++ b/drivers/gpu/drm/i915/vlv_sideband.h
@@ -26,9 +26,6 @@ enum {
 };
 
 void vlv_iosf_sb_get(struct drm_i915_private *i915, unsigned long ports);
-u32 vlv_iosf_sb_read(struct drm_i915_private *i915, u8 port, u32 reg);
-void vlv_iosf_sb_write(struct drm_i915_private *i915,
-  u8 port, u32 reg, u32 val);
 void vlv_iosf_sb_put(struct drm_i915_private *i915, unsigned long ports);
 
 static inline void vlv_bunit_get(struct drm_i915_private *i915)
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v2 3/7] drm/i915/dsi: Replace check with a (missing) MIPI sequence name

[Andy Shevchenko]

Names of the MIPI sequence steps are sequential and defined, no
need to check for the gaps. However in seq_name the MIPI_SEQ_END
is missing. Add it there, and drop unneeded NULL check in
sequence_name().

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 22b89e68e6de..1014051a6866 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -658,6 +658,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
  */
 
 static const char * const seq_name[] = {
+   [MIPI_SEQ_END] = "MIPI_SEQ_END",
[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
@@ -673,7 +674,7 @@ static const char * const seq_name[] = {
 
 static const char *sequence_name(enum mipi_seq seq_id)
 {
-   if (seq_id < ARRAY_SIZE(seq_name) && seq_name[seq_id])
+   if (seq_id < ARRAY_SIZE(seq_name))
return seq_name[seq_id];
 
return "(unknown)";
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v2 1/7] drm/i915/dsi: Replace while(1) with one with clear exit condition

[Andy Shevchenko]

Move existing condition to while(), so it will be clear on what
circumstances the loop is successfully finishing.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 24b2cbcfc1ef..a6a6f1814967 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -707,13 +707,10 @@ static void intel_dsi_vbt_exec(struct intel_dsi 
*intel_dsi,
if (connector->panel.vbt.dsi.seq_version >= 3)
data += 4;
 
-   while (1) {
+   while (*data != MIPI_SEQ_ELEM_END) {
u8 operation_byte = *data++;
u8 operation_size = 0;
 
-   if (operation_byte == MIPI_SEQ_ELEM_END)
-   break;
-
if (operation_byte < ARRAY_SIZE(exec_elem))
mipi_elem_exec = exec_elem[operation_byte];
else
-- 
2.40.0.1.gaa8946217a0b

Re: [Intel-gfx] [rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[Andy Shevchenko]

On Wed, Oct 18, 2023 at 11:09:35AM +0200, Hans de Goede wrote:
> On 10/18/23 07:10, Andy Shevchenko wrote:
> > DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
> > talking to GPIO IP behind the actual driver's back. An attempt to fix
> > that is here.
> > 
> > If I understood correctly, my approach should work in the similar way as
> > the current IOSF GPIO. 
> > 
> > Hans, I believe you have some devices that use this piece of code,
> > is it possible to give a test run on (one of) them?
> 
> Yes I should be able to find a device or 2 which poke GPIOs from the
> VBT MIPI sequences. Unfortunately I don't know from the top of my head
> which devices actually use this, so I may need to try quite a few devices
> before finding one which actually uses this.
> 
> I'll try to get this series tested sometime the coming weeks,
> depending on when I can schedule some time for this.

No hurry. maybe you simply can add into your usual tree you run on your
devices?

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v1 1/2] drm/i915/dsi: Extract common soc_gpio_exec() helper

[Andy Shevchenko]

Extract a common soc_gpio_exec() helper that may be used by a few SoCs.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 49 +++-
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 24b2cbcfc1ef..c3c3f4df9ac4 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -243,6 +243,32 @@ static const u8 *mipi_exec_delay(struct intel_dsi 
*intel_dsi, const u8 *data)
return data;
 }
 
+static void soc_exec_gpio(struct intel_connector *connector, const char 
*con_id,
+ u8 gpio_index, bool value)
+{
+   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+   /* XXX: this table is a quick ugly hack. */
+   static struct gpio_desc *soc_gpio_table[U8_MAX + 1];
+   struct gpio_desc *gpio_desc = soc_gpio_table[gpio_index];
+
+   if (gpio_desc) {
+   gpiod_set_value(gpio_desc, value);
+   } else {
+   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
+con_id, gpio_index,
+value ? GPIOD_OUT_LOW :
+GPIOD_OUT_HIGH);
+   if (IS_ERR(gpio_desc)) {
+   drm_err(_priv->drm,
+   "GPIO index %u request failed (%pe)\n",
+   gpio_index, gpio_desc);
+   return;
+   }
+
+   soc_gpio_table[gpio_index] = gpio_desc;
+   }
+}
+
 static void vlv_exec_gpio(struct intel_connector *connector,
  u8 gpio_source, u8 gpio_index, bool value)
 {
@@ -348,28 +374,7 @@ static void chv_exec_gpio(struct intel_connector 
*connector,
 static void bxt_exec_gpio(struct intel_connector *connector,
  u8 gpio_source, u8 gpio_index, bool value)
 {
-   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   /* XXX: this table is a quick ugly hack. */
-   static struct gpio_desc *bxt_gpio_table[U8_MAX + 1];
-   struct gpio_desc *gpio_desc = bxt_gpio_table[gpio_index];
-
-   if (!gpio_desc) {
-   gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-NULL, gpio_index,
-value ? GPIOD_OUT_LOW :
-GPIOD_OUT_HIGH);
-
-   if (IS_ERR_OR_NULL(gpio_desc)) {
-   drm_err(_priv->drm,
-   "GPIO index %u request failed (%ld)\n",
-   gpio_index, PTR_ERR(gpio_desc));
-   return;
-   }
-
-   bxt_gpio_table[gpio_index] = gpio_desc;
-   }
-
-   gpiod_set_value(gpio_desc, value);
+   soc_exec_gpio(connector, NULL, gpio_index, value);
 }
 
 static void icl_exec_gpio(struct intel_connector *connector,
-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[Andy Shevchenko]

DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
talking to GPIO IP behind the actual driver's back. An attempt to fix
that is here.

If I understood correctly, my approach should work in the similar way as
the current IOSF GPIO. 

Hans, I believe you have some devices that use this piece of code,
is it possible to give a test run on (one of) them?

Andy Shevchenko (2):
  drm/i915/dsi: Extract common soc_gpio_exec() helper
  drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back

 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 150 +++
 1 file changed, 58 insertions(+), 92 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[Intel-gfx] [PATCH v1 2/2] drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back

[Andy Shevchenko]

It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 40ecab551232 ("pinctrl:
baytrail: Really serialize all register accesses") for the details.
Taking all this into consideration replace the hack with proper
GPIO APIs being used.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 101 ++-
 1 file changed, 31 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 
b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index c3c3f4df9ac4..35ab4048029d 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -55,43 +55,6 @@
 #define MIPI_VIRTUAL_CHANNEL_SHIFT 1
 #define MIPI_PORT_SHIFT3
 
-/* base offsets for gpio pads */
-#define VLV_GPIO_NC_0_HV_DDI0_HPD  0x4130
-#define VLV_GPIO_NC_1_HV_DDI0_DDC_SDA  0x4120
-#define VLV_GPIO_NC_2_HV_DDI0_DDC_SCL  0x4110
-#define VLV_GPIO_NC_3_PANEL0_VDDEN 0x4140
-#define VLV_GPIO_NC_4_PANEL0_BKLTEN0x4150
-#define VLV_GPIO_NC_5_PANEL0_BKLTCTL   0x4160
-#define VLV_GPIO_NC_6_HV_DDI1_HPD  0x4180
-#define VLV_GPIO_NC_7_HV_DDI1_DDC_SDA  0x4190
-#define VLV_GPIO_NC_8_HV_DDI1_DDC_SCL  0x4170
-#define VLV_GPIO_NC_9_PANEL1_VDDEN 0x4100
-#define VLV_GPIO_NC_10_PANEL1_BKLTEN   0x40E0
-#define VLV_GPIO_NC_11_PANEL1_BKLTCTL  0x40F0
-
-#define VLV_GPIO_PCONF0(base_offset)   (base_offset)
-#define VLV_GPIO_PAD_VAL(base_offset)  ((base_offset) + 8)
-
-struct gpio_map {
-   u16 base_offset;
-   bool init;
-};
-
-static struct gpio_map vlv_gpio_table[] = {
-   { VLV_GPIO_NC_0_HV_DDI0_HPD },
-   { VLV_GPIO_NC_1_HV_DDI0_DDC_SDA },
-   { VLV_GPIO_NC_2_HV_DDI0_DDC_SCL },
-   { VLV_GPIO_NC_3_PANEL0_VDDEN },
-   { VLV_GPIO_NC_4_PANEL0_BKLTEN },
-   { VLV_GPIO_NC_5_PANEL0_BKLTCTL },
-   { VLV_GPIO_NC_6_HV_DDI1_HPD },
-   { VLV_GPIO_NC_7_HV_DDI1_DDC_SDA },
-   { VLV_GPIO_NC_8_HV_DDI1_DDC_SCL },
-   { VLV_GPIO_NC_9_PANEL1_VDDEN },
-   { VLV_GPIO_NC_10_PANEL1_BKLTEN },
-   { VLV_GPIO_NC_11_PANEL1_BKLTCTL },
-};
-
 struct i2c_adapter_lookup {
u16 slave_addr;
struct intel_dsi *intel_dsi;
@@ -269,52 +232,44 @@ static void soc_exec_gpio(struct intel_connector 
*connector, const char *con_id,
}
 }
 
+static struct gpiod_lookup_table vlv_gpio_table = {
+   .dev_id = ":00:02.0",
+   .table = {
+   GPIO_LOOKUP_IDX("INT33FC:01", 0, "Panel NC", 0, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 1, "Panel NC", 1, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 2, "Panel NC", 2, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 3, "Panel NC", 3, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 4, "Panel NC", 4, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 5, "Panel NC", 5, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 6, "Panel NC", 6, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 7, "Panel NC", 7, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 8, "Panel NC", 8, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 9, "Panel NC", 9, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 10, "Panel NC", 10, 
GPIO_ACTIVE_HIGH),
+   GPIO_LOOKUP_IDX("INT33FC:01", 11, "Panel NC", 11, 
GPIO_ACTIVE_HIGH),
+   { }
+   },
+};
+
 static void vlv_exec_gpio(struct intel_connector *connector,
  u8 gpio_source, u8 gpio_index, bool value)
 {
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-   struct gpio_map *map;
-   u16 pconf0, padval;
-   u32 tmp;
-   u8 port;
 
-   if (gpio_index >= ARRAY_SIZE(vlv_gpio_table)) {
-   drm_dbg_kms(_priv->drm, "unknown gpio index %u\n",
-   gpio_index);
-   return;
-   }
-
-   map = _gpio_table[gpio_index];
-
-   if (connector->panel.vbt.dsi.seq_version >= 3) {
-   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
-   port = IOSF_PORT_GPIO_NC;
-   } else {
-   if (gpio_source == 0) {
-   port = IOSF_PORT_GPIO_NC;
-   } else if (gpio_source == 1) {
+   /* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
+   if (connector->panel.vbt.dsi.seq_version < 3) {
+   if (gpio_source == 1) {
drm_dbg_kms(_priv->drm, "SC gpio not supported\n");

Re: [Intel-gfx] [PATCH v4 1/1] drm/i915: Move abs_diff() to math.h

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 10:24:46AM -0700, Andrew Morton wrote:
> On Thu,  3 Aug 2023 16:19:18 +0300 Andy Shevchenko 
>  wrote:

...

> > +#define abs_diff(a, b) ({  \
> > +   typeof(a) __a = (a);\
> > +   typeof(b) __b = (b);\
> > +   (void)(&__a == &__b);   \
> > +   __a > __b ? (__a - __b) : (__b - __a);  \
> > +})
> 
> Can we document it please?
> 
> Also, the open-coded type comparison could be replaced with __typecheck()?
> 
> And why the heck isn't __typecheck() in typecheck.h, to be included by
> minmax.h.
> 
> etcetera.  Sigh.  I'll grab it, but please at least send along some
> kerneldoc?

Sure and thank you!

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v4 1/1] drm/i915: Move abs_diff() to math.h

[Andy Shevchenko]

abs_diff() belongs to math.h. Move it there.
This will allow others to use it.

Reviewed-by: Jiri Slaby  # tty/serial
Acked-by: Jani Nikula 
Acked-by: Greg Kroah-Hartman 
Reviewed-by: Andi Shyti 
Reviewed-by: Philipp Zabel  # gpu/ipu-v3
Signed-off-by: Andy Shevchenko 
---
v4:
- Cc'ed to Andrew (as Jani told he is okay to route it via other tree)
- added tags
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  1 +
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  7 ---
 drivers/gpu/ipu-v3/ipu-image-convert.c| 15 +++
 drivers/tty/serial/omap-serial.c  |  7 +--
 drivers/video/fbdev/core/svgalib.c|  7 +--
 include/linux/math.h  |  7 +++
 6 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 6b2d8a1e2aa9..290e856fe9e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include 
 #include 
 
 #include "i915_reg.h"
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
index ba62eb5d7c51..04e6810954b2 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
@@ -29,13 +29,6 @@
 
 #include "intel_wakeref.h"
 
-/*FIXME: Move this to a more appropriate place. */
-#define abs_diff(a, b) ({  \
-   typeof(a) __a = (a);\
-   typeof(b) __b = (b);\
-   (void) (&__a == &__b);  \
-   __a > __b ? (__a - __b) : (__b - __a); })
-
 enum tc_port;
 struct drm_i915_private;
 struct intel_atomic_state;
diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c 
b/drivers/gpu/ipu-v3/ipu-image-convert.c
index af1612044eef..841316582ea9 100644
--- a/drivers/gpu/ipu-v3/ipu-image-convert.c
+++ b/drivers/gpu/ipu-v3/ipu-image-convert.c
@@ -7,7 +7,10 @@
 
 #include 
 #include 
+#include 
+
 #include 
+
 #include "ipu-prv.h"
 
 /*
@@ -543,7 +546,7 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
unsigned int in_pos;
unsigned int in_pos_aligned;
unsigned int in_pos_rounded;
-   unsigned int abs_diff;
+   unsigned int diff;
 
/*
 * Tiles in the right row / bottom column may not be allowed to
@@ -575,15 +578,11 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
(in_edge - in_pos_rounded) % in_burst)
continue;
 
-   if (in_pos < in_pos_aligned)
-   abs_diff = in_pos_aligned - in_pos;
-   else
-   abs_diff = in_pos - in_pos_aligned;
-
-   if (abs_diff < min_diff) {
+   diff = abs_diff(in_pos, in_pos_aligned);
+   if (diff < min_diff) {
in_seam = in_pos_rounded;
out_seam = out_pos;
-   min_diff = abs_diff;
+   min_diff = diff;
}
}
 
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 3dc14dcb01ca..0ead88c5a19a 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -222,16 +222,11 @@ static inline int calculate_baud_abs_diff(struct 
uart_port *port,
unsigned int baud, unsigned int mode)
 {
unsigned int n = port->uartclk / (mode * baud);
-   int abs_diff;
 
if (n == 0)
n = 1;
 
-   abs_diff = baud - (port->uartclk / (mode * n));
-   if (abs_diff < 0)
-   abs_diff = -abs_diff;
-
-   return abs_diff;
+   return abs_diff(baud, port->uartclk / (mode * n));
 }
 
 /*
diff --git a/drivers/video/fbdev/core/svgalib.c 
b/drivers/video/fbdev/core/svgalib.c
index 9e01322fabe3..2cba15ea 100644
--- a/drivers/video/fbdev/core/svgalib.c
+++ b/drivers/video/fbdev/core/svgalib.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -372,12 +373,6 @@ EXPORT_SYMBOL(svga_get_caps);
  *  F_VCO = (F_BASE * M) / N
  *  F_OUT = F_VCO / (2^R)
  */
-
-static inline u32 abs_diff(u32 a, u32 b)
-{
-   return (a > b) ? (a - b) : (b - a);
-}
-
 int svga_compute_pll(const struct svga_pll *pll, u32 f_wanted, u16 *m, u16 *n, 
u16 *r, int node)
 {
u16 am, an, ar;
diff --git a/include/linux/math.h b/include/linux/math.h
index 2d388650c556..336e3e3678e7 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -155,6 +155,13 @@ __STRUCT_FRACT(u32)
__builtin_types_compatible_p(typeof(x), unsigned type), \
({ signed type __x = (x); __x < 0 ? -__x : __x; }), other)
 
+#d

[Intel-gfx] [PATCH v3 1/1] drm/i915: Move abs_diff() to math.h

[Andy Shevchenko]

abs_diff() belongs to math.h. Move it there.
This will allow others to use it.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Jiri Slaby  # tty/serial
---
v3: added tag (Jiri), removed space after a cast (fdo CI)
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  1 +
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  7 ---
 drivers/gpu/ipu-v3/ipu-image-convert.c| 15 +++
 drivers/tty/serial/omap-serial.c  |  7 +--
 drivers/video/fbdev/core/svgalib.c|  7 +--
 include/linux/math.h  |  7 +++
 6 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 6b2d8a1e2aa9..290e856fe9e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include 
 #include 
 
 #include "i915_reg.h"
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
index ba62eb5d7c51..04e6810954b2 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
@@ -29,13 +29,6 @@
 
 #include "intel_wakeref.h"
 
-/*FIXME: Move this to a more appropriate place. */
-#define abs_diff(a, b) ({  \
-   typeof(a) __a = (a);\
-   typeof(b) __b = (b);\
-   (void) (&__a == &__b);  \
-   __a > __b ? (__a - __b) : (__b - __a); })
-
 enum tc_port;
 struct drm_i915_private;
 struct intel_atomic_state;
diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c 
b/drivers/gpu/ipu-v3/ipu-image-convert.c
index af1612044eef..841316582ea9 100644
--- a/drivers/gpu/ipu-v3/ipu-image-convert.c
+++ b/drivers/gpu/ipu-v3/ipu-image-convert.c
@@ -7,7 +7,10 @@
 
 #include 
 #include 
+#include 
+
 #include 
+
 #include "ipu-prv.h"
 
 /*
@@ -543,7 +546,7 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
unsigned int in_pos;
unsigned int in_pos_aligned;
unsigned int in_pos_rounded;
-   unsigned int abs_diff;
+   unsigned int diff;
 
/*
 * Tiles in the right row / bottom column may not be allowed to
@@ -575,15 +578,11 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
(in_edge - in_pos_rounded) % in_burst)
continue;
 
-   if (in_pos < in_pos_aligned)
-   abs_diff = in_pos_aligned - in_pos;
-   else
-   abs_diff = in_pos - in_pos_aligned;
-
-   if (abs_diff < min_diff) {
+   diff = abs_diff(in_pos, in_pos_aligned);
+   if (diff < min_diff) {
in_seam = in_pos_rounded;
out_seam = out_pos;
-   min_diff = abs_diff;
+   min_diff = diff;
}
}
 
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 82d35dbbfa6c..9be63a1f1f0c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -222,16 +222,11 @@ static inline int calculate_baud_abs_diff(struct 
uart_port *port,
unsigned int baud, unsigned int mode)
 {
unsigned int n = port->uartclk / (mode * baud);
-   int abs_diff;
 
if (n == 0)
n = 1;
 
-   abs_diff = baud - (port->uartclk / (mode * n));
-   if (abs_diff < 0)
-   abs_diff = -abs_diff;
-
-   return abs_diff;
+   return abs_diff(baud, port->uartclk / (mode * n));
 }
 
 /*
diff --git a/drivers/video/fbdev/core/svgalib.c 
b/drivers/video/fbdev/core/svgalib.c
index 9e01322fabe3..2cba15ea 100644
--- a/drivers/video/fbdev/core/svgalib.c
+++ b/drivers/video/fbdev/core/svgalib.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -372,12 +373,6 @@ EXPORT_SYMBOL(svga_get_caps);
  *  F_VCO = (F_BASE * M) / N
  *  F_OUT = F_VCO / (2^R)
  */
-
-static inline u32 abs_diff(u32 a, u32 b)
-{
-   return (a > b) ? (a - b) : (b - a);
-}
-
 int svga_compute_pll(const struct svga_pll *pll, u32 f_wanted, u16 *m, u16 *n, 
u16 *r, int node)
 {
u16 am, an, ar;
diff --git a/include/linux/math.h b/include/linux/math.h
index 449a29b73f6d..4459d1786f77 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -157,6 +157,13 @@ __STRUCT_FRACT(u32)
__builtin_types_compatible_p(typeof(x), unsigned type), \
({ signed type __x = (x); __x < 0 ? -__x : __x; }), other)
 
+#define abs_diff(a, b) ({  \
+   typeof(a) __a = (a);\
+   typeof(b) __b = (b);\
+   (void)(&__a ==

Re: [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/1] drm/i915: Move abs_diff() to math.h

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 02:46:35PM -, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v2,1/1] drm/i915: Move abs_diff() to math.h
> URL   : https://patchwork.freedesktop.org/series/121131/
> State : warning
> 
> == Summary ==
> 
> Error: dim checkpatch failed
> 543baaccedcc drm/i915: Move abs_diff() to math.h
> -:142: CHECK:SPACING: No space is necessary after a cast
> #142: FILE: include/linux/math.h:161:
> + (void) (&__a == &__b);  \

Ah, ah, this is existing code in the driver :-)
I will amend this in case a new version will be needed.

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v2 1/1] drm/i915: Move abs_diff() to math.h

[Andy Shevchenko]

abs_diff() belongs to math.h. Move it there.
This will allow others to use it.

Signed-off-by: Andy Shevchenko 
---
v2: better header location on ipu-v3, converted omap-serial as well
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  1 +
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  7 ---
 drivers/gpu/ipu-v3/ipu-image-convert.c| 15 +++
 drivers/tty/serial/omap-serial.c  |  7 +--
 drivers/video/fbdev/core/svgalib.c|  7 +--
 include/linux/math.h  |  6 ++
 6 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 6b2d8a1e2aa9..290e856fe9e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include 
 #include 
 
 #include "i915_reg.h"
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
index ba62eb5d7c51..04e6810954b2 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
@@ -29,13 +29,6 @@
 
 #include "intel_wakeref.h"
 
-/*FIXME: Move this to a more appropriate place. */
-#define abs_diff(a, b) ({  \
-   typeof(a) __a = (a);\
-   typeof(b) __b = (b);\
-   (void) (&__a == &__b);  \
-   __a > __b ? (__a - __b) : (__b - __a); })
-
 enum tc_port;
 struct drm_i915_private;
 struct intel_atomic_state;
diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c 
b/drivers/gpu/ipu-v3/ipu-image-convert.c
index af1612044eef..841316582ea9 100644
--- a/drivers/gpu/ipu-v3/ipu-image-convert.c
+++ b/drivers/gpu/ipu-v3/ipu-image-convert.c
@@ -7,7 +7,10 @@
 
 #include 
 #include 
+#include 
+
 #include 
+
 #include "ipu-prv.h"
 
 /*
@@ -543,7 +546,7 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
unsigned int in_pos;
unsigned int in_pos_aligned;
unsigned int in_pos_rounded;
-   unsigned int abs_diff;
+   unsigned int diff;
 
/*
 * Tiles in the right row / bottom column may not be allowed to
@@ -575,15 +578,11 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
(in_edge - in_pos_rounded) % in_burst)
continue;
 
-   if (in_pos < in_pos_aligned)
-   abs_diff = in_pos_aligned - in_pos;
-   else
-   abs_diff = in_pos - in_pos_aligned;
-
-   if (abs_diff < min_diff) {
+   diff = abs_diff(in_pos, in_pos_aligned);
+   if (diff < min_diff) {
in_seam = in_pos_rounded;
out_seam = out_pos;
-   min_diff = abs_diff;
+   min_diff = diff;
}
}
 
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 82d35dbbfa6c..9be63a1f1f0c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -222,16 +222,11 @@ static inline int calculate_baud_abs_diff(struct 
uart_port *port,
unsigned int baud, unsigned int mode)
 {
unsigned int n = port->uartclk / (mode * baud);
-   int abs_diff;
 
if (n == 0)
n = 1;
 
-   abs_diff = baud - (port->uartclk / (mode * n));
-   if (abs_diff < 0)
-   abs_diff = -abs_diff;
-
-   return abs_diff;
+   return abs_diff(baud, port->uartclk / (mode * n));
 }
 
 /*
diff --git a/drivers/video/fbdev/core/svgalib.c 
b/drivers/video/fbdev/core/svgalib.c
index 9e01322fabe3..2cba15ea 100644
--- a/drivers/video/fbdev/core/svgalib.c
+++ b/drivers/video/fbdev/core/svgalib.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -372,12 +373,6 @@ EXPORT_SYMBOL(svga_get_caps);
  *  F_VCO = (F_BASE * M) / N
  *  F_OUT = F_VCO / (2^R)
  */
-
-static inline u32 abs_diff(u32 a, u32 b)
-{
-   return (a > b) ? (a - b) : (b - a);
-}
-
 int svga_compute_pll(const struct svga_pll *pll, u32 f_wanted, u16 *m, u16 *n, 
u16 *r, int node)
 {
u16 am, an, ar;
diff --git a/include/linux/math.h b/include/linux/math.h
index 449a29b73f6d..45a21b51f183 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -157,6 +157,12 @@ __STRUCT_FRACT(u32)
__builtin_types_compatible_p(typeof(x), unsigned type), \
({ signed type __x = (x); __x < 0 ? -__x : __x; }), other)
 
+#define abs_diff(a, b) ({  \
+   typeof(a) __a = (a);\
+   typeof(b) __b = (b);\
+   (void) (&__a =

Re: [Intel-gfx] [PATCH v1 1/1] drm/i915: Move abs_diff() to math.h

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 04:42:35PM +0300, Andy Shevchenko wrote:
> abs_diff() belongs to math.h. Move it there.
> This will allow others to use it.

Sorry, forgot omap-serial case.
Will be v2 soon.

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v1 1/1] drm/i915: Move abs_diff() to math.h

[Andy Shevchenko]

abs_diff() belongs to math.h. Move it there.
This will allow others to use it.

Signed-off-by: Andy Shevchenko 
---
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  1 +
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  7 ---
 drivers/gpu/ipu-v3/ipu-image-convert.c| 14 ++
 drivers/video/fbdev/core/svgalib.c|  7 +--
 include/linux/math.h  |  6 ++
 5 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 6b2d8a1e2aa9..290e856fe9e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -21,6 +21,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include 
 #include 
 
 #include "i915_reg.h"
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h 
b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
index ba62eb5d7c51..04e6810954b2 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
@@ -29,13 +29,6 @@
 
 #include "intel_wakeref.h"
 
-/*FIXME: Move this to a more appropriate place. */
-#define abs_diff(a, b) ({  \
-   typeof(a) __a = (a);\
-   typeof(b) __b = (b);\
-   (void) (&__a == &__b);  \
-   __a > __b ? (__a - __b) : (__b - __a); })
-
 enum tc_port;
 struct drm_i915_private;
 struct intel_atomic_state;
diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c 
b/drivers/gpu/ipu-v3/ipu-image-convert.c
index af1612044eef..420992ac2ecd 100644
--- a/drivers/gpu/ipu-v3/ipu-image-convert.c
+++ b/drivers/gpu/ipu-v3/ipu-image-convert.c
@@ -8,6 +8,8 @@
 #include 
 #include 
 #include 
+#include 
+
 #include "ipu-prv.h"
 
 /*
@@ -543,7 +545,7 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
unsigned int in_pos;
unsigned int in_pos_aligned;
unsigned int in_pos_rounded;
-   unsigned int abs_diff;
+   unsigned int diff;
 
/*
 * Tiles in the right row / bottom column may not be allowed to
@@ -575,15 +577,11 @@ static void find_best_seam(struct ipu_image_convert_ctx 
*ctx,
(in_edge - in_pos_rounded) % in_burst)
continue;
 
-   if (in_pos < in_pos_aligned)
-   abs_diff = in_pos_aligned - in_pos;
-   else
-   abs_diff = in_pos - in_pos_aligned;
-
-   if (abs_diff < min_diff) {
+   diff = abs_diff(in_pos, in_pos_aligned);
+   if (diff < min_diff) {
in_seam = in_pos_rounded;
out_seam = out_pos;
-   min_diff = abs_diff;
+   min_diff = diff;
}
}
 
diff --git a/drivers/video/fbdev/core/svgalib.c 
b/drivers/video/fbdev/core/svgalib.c
index 9e01322fabe3..2cba15ea 100644
--- a/drivers/video/fbdev/core/svgalib.c
+++ b/drivers/video/fbdev/core/svgalib.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -372,12 +373,6 @@ EXPORT_SYMBOL(svga_get_caps);
  *  F_VCO = (F_BASE * M) / N
  *  F_OUT = F_VCO / (2^R)
  */
-
-static inline u32 abs_diff(u32 a, u32 b)
-{
-   return (a > b) ? (a - b) : (b - a);
-}
-
 int svga_compute_pll(const struct svga_pll *pll, u32 f_wanted, u16 *m, u16 *n, 
u16 *r, int node)
 {
u16 am, an, ar;
diff --git a/include/linux/math.h b/include/linux/math.h
index 449a29b73f6d..45a21b51f183 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -157,6 +157,12 @@ __STRUCT_FRACT(u32)
__builtin_types_compatible_p(typeof(x), unsigned type), \
({ signed type __x = (x); __x < 0 ? -__x : __x; }), other)
 
+#define abs_diff(a, b) ({  \
+   typeof(a) __a = (a);\
+   typeof(b) __b = (b);\
+   (void) (&__a == &__b);  \
+   __a > __b ? (__a - __b) : (__b - __a); })
+
 /**
  * reciprocal_scale - "scale" a value into range [0, ep_ro)
  * @val: value
-- 
2.40.0.1.gaa8946217a0b

Re: [Intel-gfx] [Intel-xe] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros

[Andy Shevchenko]

On Tue, Jun 20, 2023 at 10:25:21AM -0700, Lucas De Marchi wrote:
> On Tue, Jun 20, 2023 at 05:55:19PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 20, 2023 at 05:47:34PM +0300, Jani Nikula wrote:
> > > On Thu, 15 Jun 2023, Andy Shevchenko  
> > > wrote:
> > > > On Fri, May 12, 2023 at 02:45:19PM +0300, Jani Nikula wrote:
> > > >> On Fri, 12 May 2023, Andy Shevchenko 
> > > >>  wrote:
> > > >> > On Fri, May 12, 2023 at 02:25:18PM +0300, Jani Nikula wrote:
> > > >> >> On Fri, 12 May 2023, Andy Shevchenko 
> > > >> >>  wrote:
> > > >> >> > On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
> > > >> >> >> Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to 
> > > >> >> >> create
> > > >> >> >> masks for fixed-width types and also the corresponding BIT_U32(),
> > > >> >> >> BIT_U16() and BIT_U8().

> > > >> >> > Why?
> > > >> >>
> > > >> >> The main reason is that GENMASK() and BIT() size varies for 32/64 
> > > >> >> bit
> > > >> >> builds.
> > > >> >
> > > >> > When needed GENMASK_ULL() can be used (with respective castings 
> > > >> > perhaps)
> > > >> > and BIT_ULL(), no?
> > > >>
> > > >> How does that help with making them the same 32-bit size on both 32 and
> > > >> 64 bit builds?
> > > >
> > > > u32 x = GENMASK();
> > > > u64 y = GENMASK_ULL();
> > > >
> > > > No? Then use in your code either x or y. Note that I assume that the 
> > > > parameters
> > > > to GENMASK*() are built-time constants. Is it the case for you?
> > > 
> > > What's wrong with wanting to define macros with specific size, depending
> > > on e.g. hardware registers instead of build size?
> > 
> > Nothing, but I think the problem is smaller than it's presented.
> 
> not sure about big/small problem you are talking about. It's a problem
> for when the *device* register is a 32b fixed width, which is
> independent from the CPU you are running on. We also have registers that
> are u16 and u64. Having fixed-width GENMASK and BIT helps avoiding
> mistakes like below. Just to use one example, the diff below builds
> fine on my 64b machine, yet it's obviously wrong:
> 
>   $ git diff  diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>   index 0b414eae1683..692a0ad9a768 100644
>   --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>   +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
>   @@ -261,8 +261,8 @@ static u32 rw_with_mcr_steering_fw(struct intel_gt 
> *gt,
>* No need to save old steering reg value.
>*/
>   intel_uncore_write_fw(uncore, MTL_MCR_SELECTOR,
>   - REG_FIELD_PREP(MTL_MCR_GROUPID, 
> group) |
>   - 
> REG_FIELD_PREP(MTL_MCR_INSTANCEID, instance) |
>   + FIELD_PREP(MTL_MCR_GROUPID, 
> group) |
>   + FIELD_PREP(MTL_MCR_INSTANCEID, 
> instance) |
> (rw_flag == FW_REG_READ ? 
> GEN11_MCR_MULTICAST : 0));
>   } else if (GRAPHICS_VER(uncore->i915) >= 11) {
>   mcr_mask = GEN11_MCR_SLICE_MASK | 
> GEN11_MCR_SUBSLICE_MASK;
>   diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h 
> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>   index 718cb2c80f79..c42bc2900c6a 100644
>   --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>   +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>   @@ -80,8 +80,8 @@
>#define   GEN11_MCR_SLICE_MASK GEN11_MCR_SLICE(0xf)
>#define   GEN11_MCR_SUBSLICE(subslice) (((subslice) & 0x7) << 
> 24)
>#define   GEN11_MCR_SUBSLICE_MASK  GEN11_MCR_SUBSLICE(0x7)
>   -#define   MTL_MCR_GROUPID  REG_GENMASK(11, 8)
>   -#define   MTL_MCR_INSTANCEID   REG_GENMASK(3, 0)
>   +#define   MTL_MCR_GROUPID  GENMASK(32, 8)
>   +#define   MTL_MCR_INSTANCEID   GENMASK(3, 0)
>#define IPEIR_I965 _MMIO(0x2064)
>#define IPEHR_I965 _MMIO(0x2068)
> 
> If the driver didn't support 32b CPUs, this would even go unnoticed.

So, what does prevent you from using GENMASK_ULL()?

Another point, you may teach GENMASK() to issue a warning if hi and/or lo
bigger than BITS_PER_LONG.

I still don't see the usefulness of that churn.

> Lucas De Marchi
> 
> > And there are already header for bitfields with a lot of helpers
> > for (similar) cases if not yours.
> > 
> > > What would you use for printk format if you wanted to to print
> > > GENMASK()?
> > 
> > %lu, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros

[Andy Shevchenko]

On Tue, Jun 20, 2023 at 05:47:34PM +0300, Jani Nikula wrote:
> On Thu, 15 Jun 2023, Andy Shevchenko  
> wrote:
> > On Fri, May 12, 2023 at 02:45:19PM +0300, Jani Nikula wrote:
> >> On Fri, 12 May 2023, Andy Shevchenko  
> >> wrote:
> >> > On Fri, May 12, 2023 at 02:25:18PM +0300, Jani Nikula wrote:
> >> >> On Fri, 12 May 2023, Andy Shevchenko 
> >> >>  wrote:
> >> >> > On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
> >> >> >> Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
> >> >> >> masks for fixed-width types and also the corresponding BIT_U32(),
> >> >> >> BIT_U16() and BIT_U8().
> >> >> >
> >> >> > Why?
> >> >> 
> >> >> The main reason is that GENMASK() and BIT() size varies for 32/64 bit
> >> >> builds.
> >> >
> >> > When needed GENMASK_ULL() can be used (with respective castings perhaps)
> >> > and BIT_ULL(), no?
> >> 
> >> How does that help with making them the same 32-bit size on both 32 and
> >> 64 bit builds?
> >
> > u32 x = GENMASK();
> > u64 y = GENMASK_ULL();
> >
> > No? Then use in your code either x or y. Note that I assume that the 
> > parameters
> > to GENMASK*() are built-time constants. Is it the case for you?
> 
> What's wrong with wanting to define macros with specific size, depending
> on e.g. hardware registers instead of build size?

Nothing, but I think the problem is smaller than it's presented.
And there are already header for bitfields with a lot of helpers
for (similar) cases if not yours.

> What would you use for printk format if you wanted to to print
> GENMASK()?

%lu, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros

[Andy Shevchenko]

On Fri, May 12, 2023 at 09:29:23AM -0700, Lucas De Marchi wrote:
> On Fri, May 12, 2023 at 02:14:19PM +0300, Andy Shevchenko wrote:
> > On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
> > > Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
> > > masks for fixed-width types and also the corresponding BIT_U32(),
> > > BIT_U16() and BIT_U8().
> > 
> > Why?
> 
> to create the masks/values for device registers that are
> of a certain width, preventing mistakes like:
> 
>   #define REG10x10
>   #define REG1_ENABLE BIT(17)
>   #define REG1_FOOGENMASK(16, 15);
> 
>   register_write(REG1_ENABLE, REG1);
> 
> 
> ... if REG1 is a 16bit register for example. There were mistakes in the
> past in the i915 source leading to the creation of the REG_* variants on
> top of normal GENMASK/BIT (see last patch and commit 09b434d4f6d2
> ("drm/i915: introduce REG_BIT() and REG_GENMASK() to define register
> contents")

Doesn't it look like something for bitfield.h candidate?
If your definition doesn't fit the given mask, bail out.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros

[Andy Shevchenko]

On Fri, May 12, 2023 at 02:45:19PM +0300, Jani Nikula wrote:
> On Fri, 12 May 2023, Andy Shevchenko  
> wrote:
> > On Fri, May 12, 2023 at 02:25:18PM +0300, Jani Nikula wrote:
> >> On Fri, 12 May 2023, Andy Shevchenko  
> >> wrote:
> >> > On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
> >> >> Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
> >> >> masks for fixed-width types and also the corresponding BIT_U32(),
> >> >> BIT_U16() and BIT_U8().
> >> >
> >> > Why?
> >> 
> >> The main reason is that GENMASK() and BIT() size varies for 32/64 bit
> >> builds.
> >
> > When needed GENMASK_ULL() can be used (with respective castings perhaps)
> > and BIT_ULL(), no?
> 
> How does that help with making them the same 32-bit size on both 32 and
> 64 bit builds?

u32 x = GENMASK();
u64 y = GENMASK_ULL();

No? Then use in your code either x or y. Note that I assume that the parameters
to GENMASK*() are built-time constants. Is it the case for you?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros

[Andy Shevchenko]

On Fri, May 12, 2023 at 02:25:18PM +0300, Jani Nikula wrote:
> On Fri, 12 May 2023, Andy Shevchenko  
> wrote:
> > On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
> >> Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
> >> masks for fixed-width types and also the corresponding BIT_U32(),
> >> BIT_U16() and BIT_U8().
> >
> > Why?
> 
> The main reason is that GENMASK() and BIT() size varies for 32/64 bit
> builds.

When needed GENMASK_ULL() can be used (with respective castings perhaps)
and BIT_ULL(), no?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 2/3] linux/bits.h: Add fixed-width GENMASK and BIT macros

[Andy Shevchenko]

On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
> Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8()  macros to create
> masks for fixed-width types and also the corresponding BIT_U32(),
> BIT_U16() and BIT_U8().

Why?

> All of those depend on a new "U" suffix added to the integer constant.
> Due to naming clashes it's better to call the macro U32. Since C doesn't
> have a proper suffix for short and char types, the U16 and U18 variants
> just use U32 with one additional check in the BIT_* macros to make
> sure the compiler gives an error when the those types overflow.
> The BIT_U16() and BIT_U8() need the help of GENMASK_INPUT_CHECK(),
> as otherwise they would allow an invalid bit to be passed. Hence
> implement them in include/linux/bits.h rather than together with
> the other BIT* variants.

So, we have _Generic() in case you still wish to implement this.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] linux-next: manual merge of the usb tree with the drm-intel-fixes tree

[Andy Shevchenko]

On Tue, Jan 31, 2023 at 01:03:05PM +1100, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the usb tree got a conflict in:
> 
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c
> 
> between commit:
> 
>   5bc4b43d5c6c ("drm/i915: Fix up locking around dumping requests lists")
> 
> from the drm-intel-fixes tree and commit:
> 
>   4d70c74659d9 ("i915: Move list_count() to list.h as list_count_nodes() for 
> broader use")
> 
> from the usb tree.
> 
> I fixed it up (the former removed the code changed by the latter)

Hmm... Currently I see that 20230127002842.3169194-4-john.c.harri...@intel.com
moves the code to the drivers/gpu/drm/i915/gt/intel_execlists_submission.c.

Is there any new series beside the above mentioned that touches that file and
actually _removes_ that code?

>   and
> can carry the fix as necessary. This is now fixed as far as linux-next
> is concerned, but any non trivial conflicts should be mentioned to your
> upstream maintainer when your tree is submitted for merging.  You may
> also want to consider cooperating with the maintainer of the conflicting
> tree to minimise any particularly complex conflicts.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v3 1/6] drm/i915: Fix request locking during error capture & debugfs dump

[Andy Shevchenko]

On Fri, Jan 20, 2023 at 03:06:02PM -0800, John Harrison wrote:
> On 1/19/2023 07:16, Andy Shevchenko wrote:
> > On Wed, Jan 18, 2023 at 10:49:55PM -0800, john.c.harri...@intel.com wrote:

...

> > > + found = false;
> > > + spin_lock(>guc_state.lock);
> > >   list_for_each_entry(rq, >guc_state.requests, 
> > > sched.link) {
> > >   if (i915_test_request_state(rq) != 
> > > I915_REQUEST_ACTIVE)
> > >   continue;
> > > + found = true;
> > > + break;
> > > + }
> > This can be combined to (see also below)
> > 
> > list_for_each_entry(rq, >guc_state.requests, sched.link) {
> > if (i915_test_request_state(rq) == I915_REQUEST_ACTIVE)
> > break;
> > }
> > 
> > > + spin_unlock(>guc_state.lock);
> > Instead of 'found' you can check the current entry pointer
> > 
> > if (!list_entry_is_head(...))
> > 
> > And because requests can only be messed up with the guc_state itself, I 
> > think
> > you don't need to perform the above check under spinlock, so it's safe.
> I'm not following the argument as to why it is safe to test a guc_state
> owned list outside of holding the guc_state spinlock.

The very same reasons why found is not checked inside the lock.
If something bad to the list head pointer happens, it would mean
that we have much bigger issues. And list_entry_is_head() is specifically
to test the loop exit condition.

> I also think that having an explicit 'found' flag makes the code more
> readable and immediately obvious as to what is going on.

It depends on the perception. With boolean I have to go somewhere to be sure
that found has false when loop is fully revolved. (Sometimes it may be the
inverted loops like

found = true;
for (...loop...) {
if (...cond...) {
   found = false;
   break;
}
}

while with the helper it's obvious)

> For the sake of one
> bool (which the compiler would optimise out anyway),

Is it really optimized away?

> I don't think it is worth the obfuscation of behaviour and the risk of "I
> think this will work".

Whatever, not big deal :)

> > > + if (found) {
> > >   intel_engine_set_hung_context(engine, ce);

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v3 1/6] drm/i915: Fix request locking during error capture & debugfs dump

[Andy Shevchenko]

On Wed, Jan 18, 2023 at 10:49:55PM -0800, john.c.harri...@intel.com wrote:
> From: John Harrison 
> 
> When GuC support was added to error capture, the locking around the
> request object was broken. Fix it up.
> 
> The context based search manages the spinlocking around the search
> internally. So it needs to grab the reference count internally as
> well. The execlist only request based search relies on external
> locking, so it needs an external reference count but within the
> spinlock not outside it.
> 
> The only other caller of the context based search is the code for
> dumping engine state to debugfs. That code wasn't previously getting
> an explicit reference at all as it does everything while holding the
> execlist specific spinlock. So, that needs updaing as well as that
> spinlock doesn't help when using GuC submission. Rather than trying to
> conditionally get/put depending on submission model, just change it to
> always do the get/put.
> 
> In addition, intel_guc_find_hung_context() was not acquiring the
> correct spinlock before searching the request list. So fix that up
> too. While at it, add some extra whitespace padding for readability.

...

> + found = false;
> + spin_lock(>guc_state.lock);
>   list_for_each_entry(rq, >guc_state.requests, sched.link) {
>   if (i915_test_request_state(rq) != I915_REQUEST_ACTIVE)
>   continue;
>  
> + found = true;
> + break;
> + }

This can be combined to (see also below)

list_for_each_entry(rq, >guc_state.requests, sched.link) {
if (i915_test_request_state(rq) == I915_REQUEST_ACTIVE)
break;
}

> + spin_unlock(>guc_state.lock);

Instead of 'found' you can check the current entry pointer

if (!list_entry_is_head(...))

And because requests can only be messed up with the guc_state itself, I think
you don't need to perform the above check under spinlock, so it's safe.

> +     if (found) {
>   intel_engine_set_hung_context(engine, ce);

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v2 1/5] drm/i915: Fix request locking during error capture & debugfs dump

[Andy Shevchenko]

On Wed, Jan 18, 2023 at 09:34:47AM -0800, John Harrison wrote:
> On 1/18/2023 00:29, Andy Shevchenko wrote:
> > On Tue, Jan 17, 2023 at 01:36:26PM -0800, john.c.harri...@intel.com wrote:
> > > From: John Harrison 
> > > 
> > > When GuC support was added to error capture, the locking around the
> > > request object was broken. Fix it up.
> > > 
> > > The context based search manages the spinlocking around the search
> > > internally. So it needs to grab the reference count internally as
> > > well. The execlist only request based search relies on external
> > > locking, so it needs an external reference count. So no change to that
> > > code itself but the context version does change.
> > > 
> > > The only other caller is the code for dumping engine state to debugfs.
> > > That code wasn't previously getting an explicit reference at all as it
> > > does everything while holding the execlist specific spinlock. So that
> > > needs updaing as well as that spinlock doesn't help when using GuC
> > > submission. Rather than trying to conditionally get/put depending on
> > > submission model, just change it to always do the get/put.
> > > 
> > > In addition, intel_guc_find_hung_context() was not acquiring the
> > > correct spinlock before searching the request list. So fix that up too.
> > > Fixes: dc0dad365c5e ("drm/i915/guc: Fix for error capture after full GPU 
> > > reset
> > > with GuC")
> > Must be one line.
> In my tree it is one line. git itself does the line wrap when creating the
> email.

Can you elaborate? I never have had such issue with git send-email (starting
from v1.6.x of Git for sure).

> I missed that I need to manually unwrap it again before actually
> sending the email. Although the CI checkpatch also pointed this out in it's
> own obscure manner.

...

> > > Cc: Matthew Brost 
> > > Cc: John Harrison 
> > > Cc: Jani Nikula 
> > > Cc: Joonas Lahtinen 
> > > Cc: Rodrigo Vivi 
> > > Cc: Tvrtko Ursulin 
> > > Cc: Daniele Ceraolo Spurio 
> > > Cc: Andrzej Hajda 
> > > Cc: Chris Wilson 
> > > Cc: Matthew Auld 
> > > Cc: Matt Roper 
> > > Cc: Umesh Nerlige Ramappa 
> > > Cc: Michael Cheng 
> > > Cc: Lucas De Marchi 
> > > Cc: Tejas Upadhyay 
> > > Cc: Andy Shevchenko 
> > > Cc: Aravind Iddamsetty 
> > > Cc: Alan Previn 
> > > Cc: Bruce Chang 
> > > Cc: intel-gfx@lists.freedesktop.org
> > Is it possible to utilize --to --cc parameters to git send-email instead of
> > noisy Cc list?
> This is the list auto-generated by the 'dim fixes' tool. I am told this is
> the officially correct way to create a fixes patch - copy the output from
> 'dim fixes' as is into the patch headers.

Okay, so it may be question to the `dim` tool then...

...

> > Stray change.
> Intentional change to improve the readability of a function that is being
> modified by other changes in this patch.

But not described in the commit message. That's why "stray".

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v2 1/5] drm/i915: Fix request locking during error capture & debugfs dump

[Andy Shevchenko]

On Tue, Jan 17, 2023 at 01:36:26PM -0800, john.c.harri...@intel.com wrote:
> From: John Harrison 
> 
> When GuC support was added to error capture, the locking around the
> request object was broken. Fix it up.
> 
> The context based search manages the spinlocking around the search
> internally. So it needs to grab the reference count internally as
> well. The execlist only request based search relies on external
> locking, so it needs an external reference count. So no change to that
> code itself but the context version does change.
> 
> The only other caller is the code for dumping engine state to debugfs.
> That code wasn't previously getting an explicit reference at all as it
> does everything while holding the execlist specific spinlock. So that
> needs updaing as well as that spinlock doesn't help when using GuC
> submission. Rather than trying to conditionally get/put depending on
> submission model, just change it to always do the get/put.
> 
> In addition, intel_guc_find_hung_context() was not acquiring the
> correct spinlock before searching the request list. So fix that up too.

> Fixes: dc0dad365c5e ("drm/i915/guc: Fix for error capture after full GPU reset
> with GuC")

Must be one line.

> Fixes: 573ba126aef3 ("drm/i915/guc: Capture error state on context reset")

> Cc: Matthew Brost 
> Cc: John Harrison 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> Cc: Tvrtko Ursulin 
> Cc: Daniele Ceraolo Spurio 
> Cc: Andrzej Hajda 
> Cc: Chris Wilson 
> Cc: Matthew Auld 
> Cc: Matt Roper 
> Cc: Umesh Nerlige Ramappa 
> Cc: Michael Cheng 
> Cc: Lucas De Marchi 
> Cc: Tejas Upadhyay 
> Cc: Andy Shevchenko 
> Cc: Aravind Iddamsetty 
> Cc: Alan Previn 
> Cc: Bruce Chang 
> Cc: intel-gfx@lists.freedesktop.org

Is it possible to utilize --to --cc parameters to git send-email instead of
noisy Cc list?

...

> + if (hung_rq)
> + i915_request_put(hung_rq);

In Linux kernel the idiom is that freeing resources APIs should be NULL-aware
(or ERR_PTR aware or both). Does i915 follows that? If so, the test should be
inside i915_request_put() rather than in any of the callers.

...

> @@ -4847,6 +4857,7 @@ void intel_guc_find_hung_context(struct intel_engine_cs 
> *engine)
>   xa_lock(>context_lookup);
>       goto done;
>   }
> +
>  next:
>   intel_context_put(ce);
>   xa_lock(>context_lookup);

Stray change.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [RFC DO NOT MERGE] treewide: use __xchg in most obvious places

[Andy Shevchenko]

On Tue, Jan 10, 2023 at 01:46:37PM +0100, Andrzej Hajda wrote:
> On 10.01.2023 12:07, Andy Shevchenko wrote:
> > On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote:

...

> > > + return __xchg(_chain->p_prod_elem,
> > > +   (void *)(((u8 *)p_chain->p_prod_elem) + 
> > > p_chain->elem_size));
> > 
> > Wondering if you still need a (void *) casting after the change. Ditto for 
> > the
> > rest of similar cases.
> 
> IMHO it is not needed also before the change and IIRC gcc has an extension
> which allows to drop (u8 *) cast as well [1].

I guess you can drop at least the former one.

> [1]: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

...

> > Btw, is it done by coccinelle? If no, why not providing the script?
> 
> Yes I have used cocci. My cocci skills are far from perfect, so I did not
> want to share my dirty code, but this is nothing secret:

Thank you! It's not about secrecy, it's about automation / error proofness.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [RFC DO NOT MERGE] treewide: use __xchg in most obvious places

[Andy Shevchenko]

On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote:
> This patch tries to show usability of __xchg helper.
> It is not intended to be merged, but I can convert
> it to proper patchset if necessary.
> 
> There are many more places where __xchg can be used.
> This demo shows the most spectacular cases IMHO:
> - previous value is returned from function,
> - temporary variables are in use.
> 
> As a result readability is much better and diffstat is quite
> nice, less local vars to look at.
> In many cases whole body of functions is replaced
> with __xchg(ptr, val), so as further refactoring the whole
> function can be removed and __xchg can be called directly.

...

>  arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
> struct pt_regs *regs)
>  {
> - unsigned long orig_ret_vaddr;
> -
> - orig_ret_vaddr = regs->ARM_lr;
> - /* Replace the return addr with trampoline addr */
> - regs->ARM_lr = trampoline_vaddr;
> - return orig_ret_vaddr;
> + return __xchg(>ARM_lr, trampoline_vaddr);
>  }

If it's not a callback, the entire function can be killed.
And this is a good example of the function usage.
OTOH, these places might have a side effect (if it's in deep CPU
handlers), means we need to do this carefully.

...

>  static inline void *qed_chain_produce(struct qed_chain *p_chain)
>  {
> - void *p_ret = NULL, *p_prod_idx, *p_prod_page_idx;
> + void *p_prod_idx, *p_prod_page_idx;
>  
>   if (is_chain_u16(p_chain)) {
>   if ((p_chain->u.chain16.prod_idx &
> @@ -390,11 +391,8 @@ static inline void *qed_chain_produce(struct qed_chain 
> *p_chain)
>   p_chain->u.chain32.prod_idx++;
>   }
>  
> - p_ret = p_chain->p_prod_elem;
> - p_chain->p_prod_elem = (void *)(((u8 *)p_chain->p_prod_elem) +
> - p_chain->elem_size);
> -
> - return p_ret;
> + return __xchg(_chain->p_prod_elem,
> +   (void *)(((u8 *)p_chain->p_prod_elem) + 
> p_chain->elem_size));

Wondering if you still need a (void *) casting after the change. Ditto for the
rest of similar cases.

>  }

...

Btw, is it done by coccinelle? If no, why not providing the script?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 18/19] linux/include: add non-atomic version of xchg

[Andy Shevchenko]

On Thu, Dec 22, 2022 at 12:46:34PM +0100, Andrzej Hajda wrote:
> The pattern of setting variable with new value and returning old
> one is very common in kernel. Usually atomicity of the operation
> is not required, so xchg seems to be suboptimal and confusing in
> such cases.

FWIW,
Reviewed-by: Andy Shevchenko 

> Signed-off-by: Andrzej Hajda 
> ---
>  include/linux/non-atomic/xchg.h | 19 +++
>  1 file changed, 19 insertions(+)
>  create mode 100644 include/linux/non-atomic/xchg.h
> 
> diff --git a/include/linux/non-atomic/xchg.h b/include/linux/non-atomic/xchg.h
> new file mode 100644
> index 00..f7fa5dd746f37d
> --- /dev/null
> +++ b/include/linux/non-atomic/xchg.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_NON_ATOMIC_XCHG_H
> +#define _LINUX_NON_ATOMIC_XCHG_H
> +
> +/**
> + * __xchg - set variable pointed by @ptr to @val, return old value
> + * @ptr: pointer to affected variable
> + * @val: value to be written
> + *
> + * This is non-atomic variant of xchg.
> + */
> +#define __xchg(ptr, val) ({  \
> + __auto_type __ptr = ptr;\
> + __auto_type __t = *__ptr;   \
> + *__ptr = (val); \
> + __t;    \
> +})
> +
> +#endif
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andy Shevchenko]

On Tue, Dec 13, 2022 at 11:09:12AM +0100, Andrzej Hajda wrote:
> On 09.12.2022 19:56, Andy Shevchenko wrote:
> > On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:

...

> > > I hope there will be place for such tiny helper in kernel.
> > > Quick cocci analyze shows there is probably few thousands places
> > > where it could be used, of course I do not intend to do it :).
> > > 
> > > I was not sure where to put this macro, I hope near swap definition
> > > is the most suitable place.
> > 
> > Ah, swap() in this context is not the same. minmax.h hosts it because
> > it's often related to the swap function in the sort-type algorithms.

> >> Moreover sorry if to/cc is not correct - get_maintainers.pl was
> > > more confused than me, to who address this patch.

...

> > >   include/linux/minmax.h | 14 ++
> > 
> > Does it really suit this header? I would expect something else.
> > Maybe include/linux/non-atomic/xchg.h, dunno.
> 
> non-atomic seems quite strange for me, I would assume everything not in
> atomic is non-atomic, unless explicitly specified.
> 
> > 
> > Btw, have you looked if Ingo's gigantic series have done anything to 
> > cmpxchg.h
> > and related headers? Maybe some ideas can be taken from there?
> 
> Grepping it didn't give any clue.
> 
> Looking at 'near' languages just to get an idea (they name the function
> differently):
> 
> C++ [1]: exchange and swap are in utility header
> Rust[2]: replace and swap are in std::mem module
> 
> This is some argument to put them together.

Again, I left the above part on top of this message, the swap() in Linux kernel
is not related to __xchg() or similar. That said, minmax.h is not a good place
for the latter.

> [1]: https://en.cppreference.com/w/cpp/header/utility
> [2]: https://doc.rust-lang.org/std/mem/index.html

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state

[Andy Shevchenko]

On Mon, Dec 12, 2022 at 12:16:04AM +0200, david.keisars...@mail.huji.ac.il 
wrote:
> From: David 
> 
> Since the two functions
>  prandom_byte_state and prandom_u32_state
>  use the weak prng prandom_u32,
>  we added the prefix predictable_rng,
>  to their signatures so it is clear they are weak.

It's fancy indentation.

...

>   /* Fisher-Yates shuffle */
>   for (i = count - 1; i > 0; i--) {
> - rand = prandom_u32_state(_state);
> + rand = 
> predictable_rng_prandom_u32_state(_state);

Isn't it too many "random":s encoded in the name?

I would leave either "rng" or "[p]random".

>   rand %= (i + 1);
>   swap_free_obj(slab, i, rand);
>   }

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andy Shevchenko]

On Fri, Dec 09, 2022 at 08:56:28PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:

...

> > I hope there will be place for such tiny helper in kernel.
> > Quick cocci analyze shows there is probably few thousands places
> > where it could be used, of course I do not intend to do it :).
> > 
> > I was not sure where to put this macro, I hope near swap definition
> > is the most suitable place.
> 
> Ah, swap() in this context is not the same. minmax.h hosts it because
> it's often related to the swap function in the sort-type algorithms.
> 
> > Moreover sorry if to/cc is not correct - get_maintainers.pl was
> > more confused than me, to who address this patch.
> 
> ...
> 
> >  include/linux/minmax.h | 14 ++
> 
> Does it really suit this header? I would expect something else.

> Maybe include/linux/non-atomic/xchg.h, dunno.

It may become a candidate to host io-64 non-atomic versions and other
non-atomic generic headers...

> Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
> and related headers? Maybe some ideas can be taken from there?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andy Shevchenko]

On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:
> The pattern of setting variable with new value and returning old
> one is very common in kernel. Usually atomicity of the operation
> is not required, so xchg seems to be suboptimal and confusing in
> such cases. Since name xchg is already in use and __xchg is used
> in architecture code, proposition is to name the macro exchange.
> 
> Signed-off-by: Andrzej Hajda 
> ---
> Hi,
> 
> I hope there will be place for such tiny helper in kernel.
> Quick cocci analyze shows there is probably few thousands places
> where it could be used, of course I do not intend to do it :).
> 
> I was not sure where to put this macro, I hope near swap definition
> is the most suitable place.

Ah, swap() in this context is not the same. minmax.h hosts it because
it's often related to the swap function in the sort-type algorithms.

> Moreover sorry if to/cc is not correct - get_maintainers.pl was
> more confused than me, to who address this patch.

...

>  include/linux/minmax.h | 14 ++

Does it really suit this header? I would expect something else.
Maybe include/linux/non-atomic/xchg.h, dunno.

Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
and related headers? Maybe some ideas can be taken from there?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v5 1/4] i915: Move list_count() to list.h as list_count_nodes() for broader use

[Andy Shevchenko]

On Thu, Dec 08, 2022 at 02:14:54PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 08, 2022 at 02:54:45PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 30, 2022 at 03:48:35PM +0200, Andy Shevchenko wrote:
> > > Some of the existing users, and definitely will be new ones, want to
> > > count existing nodes in the list. Provide a generic API for that by
> > > moving code from i915 to list.h.
> > 
> > Greg, I believe this one is ready to be taken. Or please tell me what I need
> > to do.
> 
> Wait for me to get through the current backlog of patches that I have in
> my review queue.  Odds are, it will have to wait until after 6.2-rc1 is
> out based on when 6.1 is going to be released.

It's fine, no hurry and take your time!

> Don't worry, it's not lost.

Thank you, got it!

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH v5 1/4] i915: Move list_count() to list.h as list_count_nodes() for broader use

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 03:48:35PM +0200, Andy Shevchenko wrote:
> Some of the existing users, and definitely will be new ones, want to
> count existing nodes in the list. Provide a generic API for that by
> moving code from i915 to list.h.

Greg, I believe this one is ready to be taken. Or please tell me what I need
to do.

-- 
With Best Regards,
Andy Shevchenko

[Intel-gfx] [PATCH v5 1/4] i915: Move list_count() to list.h as list_count_nodes() for broader use

[Andy Shevchenko]

Some of the existing users, and definitely will be new ones, want to
count existing nodes in the list. Provide a generic API for that by
moving code from i915 to list.h.

Reviewed-by: Lucas De Marchi 
Acked-by: Jani Nikula 
Signed-off-by: Andy Shevchenko 
---
v5: added tag (Lucas), renamed API to list_count_nodes() (LKP)
v4: fixed prototype when converting to static inline
v3: added tag (Jani), changed to be static inline (Mike)
v2: dropped the duplicate code in i915 (LKP)
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 15 ++-
 include/linux/list.h  | 15 +++
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 1f7188129cd1..370164363b0d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -2004,17 +2004,6 @@ static void print_request_ring(struct drm_printer *m, 
struct i915_request *rq)
}
 }
 
-static unsigned long list_count(struct list_head *list)
-{
-   struct list_head *pos;
-   unsigned long count = 0;
-
-   list_for_each(pos, list)
-   count++;
-
-   return count;
-}
-
 static unsigned long read_ul(void *p, size_t x)
 {
return *(unsigned long *)(p + x);
@@ -2189,8 +2178,8 @@ void intel_engine_dump(struct intel_engine_cs *engine,
spin_lock_irqsave(>sched_engine->lock, flags);
engine_dump_active_requests(engine, m);
 
-   drm_printf(m, "\tOn hold?: %lu\n",
-  list_count(>sched_engine->hold));
+   drm_printf(m, "\tOn hold?: %zu\n",
+  list_count_nodes(>sched_engine->hold));
spin_unlock_irqrestore(>sched_engine->lock, flags);
 
drm_printf(m, "\tMMIO base:  0x%08x\n", engine->mmio_base);
diff --git a/include/linux/list.h b/include/linux/list.h
index 61762054b4be..f10344dbad4d 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -655,6 +655,21 @@ static inline void list_splice_tail_init(struct list_head 
*list,
 !list_is_head(pos, (head)); \
 pos = n, n = pos->prev)
 
+/**
+ * list_count_nodes - count nodes in the list
+ * @head:  the head for your list.
+ */
+static inline size_t list_count_nodes(struct list_head *head)
+{
+   struct list_head *pos;
+   size_t count = 0;
+
+   list_for_each(pos, head)
+   count++;
+
+   return count;
+}
+
 /**
  * list_entry_is_head - test if the entry points to the head of the list
  * @pos:   the type * to cursor
-- 
2.35.1