Re: [PATCH] drm/i915: limit eDP MSO pipe only for display version 20 and below

[Coelho, Luciano]

On Wed, 2024-01-24 at 10:22 -0300, Gustavo Sousa wrote:
> Hi, Luca!

Hi Gustavo!


> Quoting Luca Coelho (2024-01-24 05:52:29-03:00)
> > The pipes that can be used for eDP MSO are limited to pipe A (and
> > sometimes also pipe B) only for display version 20 and below.
> > 
> > Modify the function that returns the pipe mask for eDP MSO so that
> > these limitations only apply to version 20 and below, enabling all
> > pipes otherwise.
> > 
> > Bspec: 68923
> > Cc: Jani Nikula 
> > Cc: James Ausmus 
> > Signed-off-by: Luca Coelho 
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 922194b957be..5c99ae148213 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -2336,13 +2336,18 @@ static void intel_ddi_power_up_lanes(struct 
> > intel_encoder *encoder,
> > }
> > }
> > 
> > -/* Splitter enable for eDP MSO is limited to certain pipes. */
> > +/*
> > + * Splitter enable for eDP MSO is limited to certain pipes, on certain
> > + * platforms.
> > + */
> > static u8 intel_ddi_splitter_pipe_mask(struct drm_i915_private *i915)
> > {
> > if (IS_ALDERLAKE_P(i915))
> 
> Looks like Xe_LPD+ (MTL's display) and Xe2_LPD (LNL's display) both support 
> both
> pipes A and B. For Xe_LPD+ we have that info in BSpec 55473 and for Xe2_LPD, 
> in
> BSpec 68923. So, I think we could:
> 
>   a. OR the condition above with IS_DISPLAY_IP_RANGE(i915, IP_VER(14, 0),
>  IP_VER(20, 0)), and
>   b. And make the "else if" below be about display versions below 14.

Okay, but I guess we have never tested this with MTL and LNL, so can we
be sure we won't break anything?

In any case, we have bspecs for these, so I'll make the change as you
suggested.

> With those additions,
> 
> Reviewed-by: Gustavo Sousa 

Thanks!

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH v7] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Thu, 2023-12-07 at 08:24 +, Hogander, Jouni wrote:
> On Fri, 2023-12-01 at 12:00 +0200, Luca Coelho wrote:
> > The uncore code may not always be available (e.g. when we build the
> > display code with Xe), so we can't always rely on having the uncore's
> > spinlock.
> > 
> > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > create wrapper functions for locking and unlocking the uncore's
> > spinlock.  In these functions, we have a condition check and only
> > actually try to lock/unlock the spinlock when I915 is defined, and
> > thus uncore is available.
> > 
> > This keeps the ifdefs contained in these new functions and all such
> > logic inside the display code.
> > 
> > Cc: Tvrtko Ursulin 
> > Cc: Jani Nikula 
> > Cc: Ville Syrjala 
> > Cc: Rodrigo Vivi 
> > Signed-off-by: Luca Coelho 
> > ---
> > 
> > 
> > In v2:
> > 
> >    * Renamed uncore_spin_*() to intel_spin_*()
> >    * Corrected the order: save, lock, unlock, restore
> > 
> > In v3:
> > 
> >    * Undid the change to pass drm_i915_private instead of the lock
> >  itself, since we would have to include i915_drv.h and that pulls
> >  in a truckload of other includes.
> > 
> > In v4:
> > 
> >    * After a brief attempt to replace this with a different patch,
> >  we're back to this one;
> >    * Pass drm_i195_private again, and move the functions to
> >  intel_vblank.c, so we don't need to include i915_drv.h in a
> >  header file and it's already included in intel_vblank.c;
> > 
> > In v5:
> > 
> >    * Remove stray include in intel_display.h;
> >    * Remove unnecessary inline modifiers in the new functions.
> > 
> > In v6:
> > 
> >    * Just removed the umlauts from Ville's name, because patchwork
> >  didn't catch my patch and I suspect it was some UTF-8 confusion.
> > 
> > In v7:
> > 
> >    * Add __acquires()/__releases() annotation to resolve sparse
> >  warnings.
> > 
> >  drivers/gpu/drm/i915/display/intel_vblank.c | 51 +--
> > --
> >  1 file changed, 41 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
> > b/drivers/gpu/drm/i915/display/intel_vblank.c
> > index 2cec2abf9746..fe256bf7b485 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > @@ -265,6 +265,32 @@ int intel_crtc_scanline_to_hw(struct intel_crtc
> > *crtc, int scanline)
> > return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> >  }
> >  
> > +/*
> > + * The uncore version of the spin lock functions is used to decide
> > + * whether we need to lock the uncore lock or not.  This is only
> > + * needed in i915, not in Xe.
> > + *
> > + * This lock in i915 is needed because some old platforms (at least
> > + * IVB and possibly HSW as well), which are not supported in Xe,
> > need
> > + * all register accesses to the same cacheline to be serialized,
> > + * otherwise they may hang.
> > + */
> > +static void intel_vblank_section_enter(struct drm_i915_private
> > *i915)
> > +   __acquires(i915->uncore.lock)
> > +{
> > +#ifdef I915
> > +   spin_lock(>uncore.lock);
> > +#endif
> > +}
> > +
> > +static void intel_vblank_section_exit(struct drm_i915_private *i915)
> > +   __releases(i915->uncore.lock)
> > +{
> > +#ifdef I915
> > +   spin_unlock(>uncore.lock);
> > +#endif
> > +}
> > +
> 
> Why don't you move these into gpu/drm/i915/intel_uncore.c/h? Then you
> could have empty defines/functions for these in gpu/drm/xe/compat-i915-
> headers/intel_uncore.h. That way you don't need these ifdefs. If you
> move them as I proposed you should rename them as well.

We already went forth and back with this for some time.  In the end we
agreed that this is not related to uncore directly, so we decided to
keep it here.

We also agreed that I'll make a follow-up patch where it won't be only
the lock that will be handled by this, but also enabling/disabling
interrupts, which doesn't have anything to do with uncore, thus the
name of the function.


--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH v6] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Thu, 2023-11-30 at 09:31 -0500, Rodrigo Vivi wrote:
> On Thu, Nov 30, 2023 at 01:54:13PM +0000, Coelho, Luciano wrote:
> > On Thu, 2023-11-30 at 13:24 +, Tvrtko Ursulin wrote:
> > > On 30/11/2023 12:26, Coelho, Luciano wrote:
> > > > On Thu, 2023-11-30 at 12:21 +, Tvrtko Ursulin wrote:
> > > > > On 30/11/2023 11:35, Luca Coelho wrote:
> > > > > > The uncore code may not always be available (e.g. when we build the
> > > > > > display code with Xe), so we can't always rely on having the 
> > > > > > uncore's
> > > > > > spinlock.
> > > > > > 
> > > > > > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > > > > > spin_lock/unlock() followed by a call to local_irq_save/restore() 
> > > > > > and
> > > > > > create wrapper functions for locking and unlocking the uncore's
> > > > > > spinlock.  In these functions, we have a condition check and only
> > > > > > actually try to lock/unlock the spinlock when I915 is defined, and
> > > > > > thus uncore is available.
> > > > > > 
> > > > > > This keeps the ifdefs contained in these new functions and all such
> > > > > > logic inside the display code.
> > > > > > 
> > > > > > Cc: Tvrtko Ursulin 
> > > > > > Cc: Jani Nikula 
> > > > > > Cc: Ville Syrjala 
> > > > > > Reviewed-by: Rodrigo Vivi 
> > > > > > Signed-off-by: Luca Coelho 
> > > > > > ---
> > > > > > 
> > > > > > 
> > > > > > In v2:
> > > > > > 
> > > > > >  * Renamed uncore_spin_*() to intel_spin_*()
> > > > > >  * Corrected the order: save, lock, unlock, restore
> > > > > > 
> > > > > > In v3:
> > > > > > 
> > > > > >  * Undid the change to pass drm_i915_private instead of the lock
> > > > > >itself, since we would have to include i915_drv.h and that 
> > > > > > pulls
> > > > > >in a truckload of other includes.
> > > > > > 
> > > > > > In v4:
> > > > > > 
> > > > > >  * After a brief attempt to replace this with a different patch,
> > > > > >we're back to this one;
> > > > > >  * Pass drm_i195_private again, and move the functions to
> > > > > >intel_vblank.c, so we don't need to include i915_drv.h in a
> > > > > >header file and it's already included in intel_vblank.c;
> > > > > > 
> > > > > > In v5:
> > > > > > 
> > > > > >  * Remove stray include in intel_display.h;
> > > > > >  * Remove unnecessary inline modifiers in the new functions.
> > > > > > 
> > > > > > In v6:
> > > > > > 
> > > > > >  * Just removed the umlauts from Ville's name, because patchwork
> > > > > >didn't catch my patch and I suspect it was some UTF-8 
> > > > > > confusion.
> > > > > > 
> > > > > >drivers/gpu/drm/i915/display/intel_vblank.c | 49 
> > > > > > -
> > > > > >1 file changed, 39 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c 
> > > > > > b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > > > > index 2cec2abf9746..221fcd6bf77b 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > > > > @@ -265,6 +265,30 @@ int intel_crtc_scanline_to_hw(struct 
> > > > > > intel_crtc *crtc, int scanline)
> > > > > > return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> > > > > >}
> > > > > >
> > > > > > +/*
> > > > > > + * The uncore version of the spin lock functions is used to decide
> > > > > > + * whether we need to lock the uncore lock or not.  This is only
> > > > > > + * needed in i915, not in Xe.
> > > > > > + *
> > > > > > + * This lock in i915 

Re: [Intel-gfx] [PATCH v6] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Thu, 2023-11-30 at 13:24 +, Tvrtko Ursulin wrote:
> On 30/11/2023 12:26, Coelho, Luciano wrote:
> > On Thu, 2023-11-30 at 12:21 +, Tvrtko Ursulin wrote:
> > > On 30/11/2023 11:35, Luca Coelho wrote:
> > > > The uncore code may not always be available (e.g. when we build the
> > > > display code with Xe), so we can't always rely on having the uncore's
> > > > spinlock.
> > > > 
> > > > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > > > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > > > create wrapper functions for locking and unlocking the uncore's
> > > > spinlock.  In these functions, we have a condition check and only
> > > > actually try to lock/unlock the spinlock when I915 is defined, and
> > > > thus uncore is available.
> > > > 
> > > > This keeps the ifdefs contained in these new functions and all such
> > > > logic inside the display code.
> > > > 
> > > > Cc: Tvrtko Ursulin 
> > > > Cc: Jani Nikula 
> > > > Cc: Ville Syrjala 
> > > > Reviewed-by: Rodrigo Vivi 
> > > > Signed-off-by: Luca Coelho 
> > > > ---
> > > > 
> > > > 
> > > > In v2:
> > > > 
> > > >  * Renamed uncore_spin_*() to intel_spin_*()
> > > >  * Corrected the order: save, lock, unlock, restore
> > > > 
> > > > In v3:
> > > > 
> > > >  * Undid the change to pass drm_i915_private instead of the lock
> > > >itself, since we would have to include i915_drv.h and that pulls
> > > >in a truckload of other includes.
> > > > 
> > > > In v4:
> > > > 
> > > >  * After a brief attempt to replace this with a different patch,
> > > >we're back to this one;
> > > >  * Pass drm_i195_private again, and move the functions to
> > > >intel_vblank.c, so we don't need to include i915_drv.h in a
> > > >header file and it's already included in intel_vblank.c;
> > > > 
> > > > In v5:
> > > > 
> > > >  * Remove stray include in intel_display.h;
> > > >  * Remove unnecessary inline modifiers in the new functions.
> > > > 
> > > > In v6:
> > > > 
> > > >  * Just removed the umlauts from Ville's name, because patchwork
> > > >didn't catch my patch and I suspect it was some UTF-8 confusion.
> > > > 
> > > >drivers/gpu/drm/i915/display/intel_vblank.c | 49 
> > > > -
> > > >1 file changed, 39 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c 
> > > > b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > > index 2cec2abf9746..221fcd6bf77b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > > @@ -265,6 +265,30 @@ int intel_crtc_scanline_to_hw(struct intel_crtc 
> > > > *crtc, int scanline)
> > > > return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> > > >}
> > > >
> > > > +/*
> > > > + * The uncore version of the spin lock functions is used to decide
> > > > + * whether we need to lock the uncore lock or not.  This is only
> > > > + * needed in i915, not in Xe.
> > > > + *
> > > > + * This lock in i915 is needed because some old platforms (at least
> > > > + * IVB and possibly HSW as well), which are not supported in Xe, need
> > > > + * all register accesses to the same cacheline to be serialized,
> > > > + * otherwise they may hang.
> > > > + */
> > > > +static void intel_vblank_section_enter(struct drm_i915_private *i915)
> > > > +{
> > > > +#ifdef I915
> > > > +   spin_lock(>uncore.lock);
> > > > +#endif
> > > > +}
> > > > +
> > > > +static void intel_vblank_section_exit(struct drm_i915_private *i915)
> > > > +{
> > > > +#ifdef I915
> > > > +   spin_unlock(>uncore.lock);
> > > > +#endif
> > > > +}
> > > > +
> > > >static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
> > > >  bool in_vblan

Re: [Intel-gfx] [PATCH v6] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Thu, 2023-11-30 at 12:21 +, Tvrtko Ursulin wrote:
> On 30/11/2023 11:35, Luca Coelho wrote:
> > The uncore code may not always be available (e.g. when we build the
> > display code with Xe), so we can't always rely on having the uncore's
> > spinlock.
> > 
> > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > create wrapper functions for locking and unlocking the uncore's
> > spinlock.  In these functions, we have a condition check and only
> > actually try to lock/unlock the spinlock when I915 is defined, and
> > thus uncore is available.
> > 
> > This keeps the ifdefs contained in these new functions and all such
> > logic inside the display code.
> > 
> > Cc: Tvrtko Ursulin 
> > Cc: Jani Nikula 
> > Cc: Ville Syrjala 
> > Reviewed-by: Rodrigo Vivi 
> > Signed-off-by: Luca Coelho 
> > ---
> > 
> > 
> > In v2:
> > 
> > * Renamed uncore_spin_*() to intel_spin_*()
> > * Corrected the order: save, lock, unlock, restore
> > 
> > In v3:
> > 
> > * Undid the change to pass drm_i915_private instead of the lock
> >   itself, since we would have to include i915_drv.h and that pulls
> >   in a truckload of other includes.
> > 
> > In v4:
> > 
> > * After a brief attempt to replace this with a different patch,
> >   we're back to this one;
> > * Pass drm_i195_private again, and move the functions to
> >   intel_vblank.c, so we don't need to include i915_drv.h in a
> >   header file and it's already included in intel_vblank.c;
> > 
> > In v5:
> > 
> > * Remove stray include in intel_display.h;
> > * Remove unnecessary inline modifiers in the new functions.
> > 
> > In v6:
> > 
> > * Just removed the umlauts from Ville's name, because patchwork
> >   didn't catch my patch and I suspect it was some UTF-8 confusion.
> > 
> >   drivers/gpu/drm/i915/display/intel_vblank.c | 49 -
> >   1 file changed, 39 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c 
> > b/drivers/gpu/drm/i915/display/intel_vblank.c
> > index 2cec2abf9746..221fcd6bf77b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > @@ -265,6 +265,30 @@ int intel_crtc_scanline_to_hw(struct intel_crtc *crtc, 
> > int scanline)
> > return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> >   }
> >   
> > +/*
> > + * The uncore version of the spin lock functions is used to decide
> > + * whether we need to lock the uncore lock or not.  This is only
> > + * needed in i915, not in Xe.
> > + *
> > + * This lock in i915 is needed because some old platforms (at least
> > + * IVB and possibly HSW as well), which are not supported in Xe, need
> > + * all register accesses to the same cacheline to be serialized,
> > + * otherwise they may hang.
> > + */
> > +static void intel_vblank_section_enter(struct drm_i915_private *i915)
> > +{
> > +#ifdef I915
> > +   spin_lock(>uncore.lock);
> > +#endif
> > +}
> > +
> > +static void intel_vblank_section_exit(struct drm_i915_private *i915)
> > +{
> > +#ifdef I915
> > +   spin_unlock(>uncore.lock);
> > +#endif
> > +}
> > +
> >   static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
> >  bool in_vblank_irq,
> >  int *vpos, int *hpos,
> > @@ -302,11 +326,12 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc 
> > *_crtc,
> > }
> >   
> > /*
> > -* Lock uncore.lock, as we will do multiple timing critical raw
> > -* register reads, potentially with preemption disabled, so the
> > -* following code must not block on uncore.lock.
> > +* Enter vblank critical section, as we will do multiple
> > +* timing critical raw register reads, potentially with
> > +* preemption disabled, so the following code must not block.
> >  */
> > -   spin_lock_irqsave(_priv->uncore.lock, irqflags);
> > +   local_irq_save(irqflags);
> > +   intel_vblank_section_enter(dev_priv);
> 
> Shouldn't local_irq_save go into intel_vblank_section_enter()? It seems 
> all callers from both i915 and xe end up doing that anyway and naming 
> "vblank_start" was presumed there would be more to the section than 
> cacheline mmio bug. I mean that there is some benefit from keeping the 
> readout timings tight.
> 

The reason is that there is one caller that has already disabled
interrupts when this function is called (see below), so we shouldn't do
it again. 


> >   
> > /* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
> >   
> > @@ -374,7 +399,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc 
> > *_crtc,
> >   
> > /* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */
> >   
> > -   spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
> > +   intel_vblank_section_exit(dev_priv);
> > +   local_irq_restore(irqflags);
> > 

Re: [Intel-gfx] [Intel-xe] [PATCH v4] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Wed, 2023-11-29 at 15:34 -0500, Rodrigo Vivi wrote:
> On Wed, Nov 29, 2023 at 03:24:33PM -0500, Coelho, Luciano wrote:
> > On Wed, 2023-11-29 at 13:01 -0500, Rodrigo Vivi wrote:
> > > On Wed, Nov 29, 2023 at 11:17:28AM +0200, Luca Coelho wrote:
> > > > The uncore code may not always be available (e.g. when we build the
> > > > display code with Xe), so we can't always rely on having the uncore's
> > > > spinlock.
> > > > 
> > > > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > > > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > > > create wrapper functions for locking and unlocking the uncore's
> > > > spinlock.  In these functions, we have a condition check and only
> > > > actually try to lock/unlock the spinlock when I915 is defined, and
> > > > thus uncore is available.
> > > > 
> > > > This keeps the ifdefs contained in these new functions and all such
> > > > logic inside the display code.
> > > > 
> > > > Cc: Tvrtko Ursulin 
> > > > Cc: Jani Nikula 
> > > > Cc: Rodrigo Vivi 
> > > > Cc: Ville Syrj?l? 
> > > > Signed-off-by: Luca Coelho 
> > > > ---
> > > > 
> > > > In v2:
> > > > 
> > > >* Renamed uncore_spin_*() to intel_spin_*()
> > > >* Corrected the order: save, lock, unlock, restore
> > > > 
> > > > In v3:
> > > > 
> > > >* Undid the change to pass drm_i915_private instead of the lock
> > > >  itself, since we would have to include i915_drv.h and that pulls
> > > >  in a truckload of other includes.
> > > > 
> > > > In v4:
> > > > 
> > > >* After a brief attempt to replace this with a different patch,
> > > >  we're back to this one;
> > > >* Pass drm_i195_private again, and move the functions to
> > > >  intel_vblank.c, so we don't need to include i915_drv.h in a
> > > >  header file and it's already included in intel_vblank.c;
> > > > 
> > > >  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> > > >  drivers/gpu/drm/i915/display/intel_vblank.c  | 45 +++-
> > > >  2 files changed, 36 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
> > > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > > index 8548f49e3972..5ff299bc4b87 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > > @@ -29,6 +29,7 @@
> > > >  
> > > >  #include "i915_reg_defs.h"
> > > >  #include "intel_display_limits.h"
> > > > +#include "i915_drv.h"
> > > 
> > > please move this include to intel_vblank.c
> > 
> > Oops, this is a leftover of some tests I was making to see just how
> > much worse things would get by adding this here.
> > 
> > Actually, why don't we move the drm_i915_private structure (and maybe
> > others?) to a lighter header file than i915_drv.h? IMHO it's really
> > annoying to have the forward declarations for it in many places just
> > because we don't want to include the actual header.  When I want to
> > find its global definition, cscope always returns tens of results
> > because of the forward declarations... This is obviously orthogonal to
> > the current patch.
> 
> yeah, I know. It is really inconvenient sometimes. I got used to run
> cscope and then search for "struct something {" to find the right place.

That's what I do as well.  Actually, just searching for " {" in the
results usually suffices.  But anyway inconvenient.


> But this inconvenience is actually smaller when compared to the compilation
> time whenever a header gets modified and included by other headers. If I
> remember correctly Jani did the initial assessment of compilation times
> and started to move headers including out of other headers. He might
> have more details/data on his findings.

Okay, I'll talk to him.  But it's always nice to rant a bit in public,
so my blurt stands. :D

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH v4] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Wed, 2023-11-29 at 13:01 -0500, Rodrigo Vivi wrote:
> On Wed, Nov 29, 2023 at 11:17:28AM +0200, Luca Coelho wrote:
> > The uncore code may not always be available (e.g. when we build the
> > display code with Xe), so we can't always rely on having the uncore's
> > spinlock.
> > 
> > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > create wrapper functions for locking and unlocking the uncore's
> > spinlock.  In these functions, we have a condition check and only
> > actually try to lock/unlock the spinlock when I915 is defined, and
> > thus uncore is available.
> > 
> > This keeps the ifdefs contained in these new functions and all such
> > logic inside the display code.
> > 
> > Cc: Tvrtko Ursulin 
> > Cc: Jani Nikula 
> > Cc: Rodrigo Vivi 
> > Cc: Ville Syrj?l? 
> > Signed-off-by: Luca Coelho 
> > ---
> > 
> > In v2:
> > 
> >* Renamed uncore_spin_*() to intel_spin_*()
> >* Corrected the order: save, lock, unlock, restore
> > 
> > In v3:
> > 
> >* Undid the change to pass drm_i915_private instead of the lock
> >  itself, since we would have to include i915_drv.h and that pulls
> >  in a truckload of other includes.
> > 
> > In v4:
> > 
> >* After a brief attempt to replace this with a different patch,
> >  we're back to this one;
> >* Pass drm_i195_private again, and move the functions to
> >  intel_vblank.c, so we don't need to include i915_drv.h in a
> >  header file and it's already included in intel_vblank.c;
> > 
> >  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> >  drivers/gpu/drm/i915/display/intel_vblank.c  | 45 +++-
> >  2 files changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index 8548f49e3972..5ff299bc4b87 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -29,6 +29,7 @@
> >  
> >  #include "i915_reg_defs.h"
> >  #include "intel_display_limits.h"
> > +#include "i915_drv.h"
> 
> please move this include to intel_vblank.c

Oops, this is a leftover of some tests I was making to see just how
much worse things would get by adding this here.

Actually, why don't we move the drm_i915_private structure (and maybe
others?) to a lighter header file than i915_drv.h? IMHO it's really
annoying to have the forward declarations for it in many places just
because we don't want to include the actual header.  When I want to
find its global definition, cscope always returns tens of results
because of the forward declarations... This is obviously orthogonal to
the current patch.


> >  enum drm_scaling_filter;
> >  struct dpll;
> > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c 
> > b/drivers/gpu/drm/i915/display/intel_vblank.c
> > index 2cec2abf9746..d9625db82681 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > @@ -265,6 +265,26 @@ int intel_crtc_scanline_to_hw(struct intel_crtc *crtc, 
> > int scanline)
> > return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> >  }
> >  
> > +/*
> > + * The uncore version of the spin lock functions is used to decide
> > + * whether we need to lock the uncore lock or not.  This is only
> > + * needed in i915, not in Xe.  Keep the decision-making centralized
> > + * here.
> 
> maybe we could add brief mention that it is only needed because old hardware
> that is not supported by Xe.

Good idea, I'll add it.

> 
> > + */
> > +static inline void intel_vblank_section_enter(struct drm_i915_private 
> > *i915)
> 
> let's avoid inline here.

Okay, I'll remove it.


> > +{
> > +#ifdef I915
> > +   spin_lock(>uncore.lock);
> > +#endif
> > +}
> > +
> > +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
> 
> and here

Okay.


> With these changes:
> 
> Reviewed-by: Rodrigo Vivi 

Thanks for the review, Rodrigo!

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/i915: don't use uncore spinlock to protect critical section in vblank

[Coelho, Luciano]

On Fri, 2023-11-17 at 17:15 +, Zanoni, Paulo R wrote:
> On Fri, 2023-11-17 at 11:50 -0500, Rodrigo Vivi wrote:
> > On Fri, Nov 17, 2023 at 11:26:44AM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 17, 2023 at 10:41:43AM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 17, 2023 at 08:05:21AM +, Coelho, Luciano wrote:
> > > > > Thanks for your comments, Ville!
> > > > > 
> > > > > On Fri, 2023-11-17 at 09:19 +0200, Ville Syrjälä wrote:
> > > > > > On Thu, Nov 16, 2023 at 01:27:00PM +0200, Luca Coelho wrote:
> > > > > > > Since we're abstracting the display code from the underlying 
> > > > > > > driver
> > > > > > > (i.e. i915 vs xe), we can't use the uncore's spinlock to protect
> > > > > > > critical sections of our code.
> > > > > > > 
> > > > > > > After further inspection, it seems that the spinlock is not 
> > > > > > > needed at
> > > > > > > all and this can be handled by disabling preemption and interrupts
> > > > > > > instead.
> > > > > > 
> > > > > > uncore.lock has multiple purposes:
> > > > > > 1. serialize all register accesses to the same cacheline as on
> > > > > >certain platforms that can hang the machine
> > > > > 
> > > > > Okay, do you remember which platforms?
> > > > 
> > > > HSW is the one I remember for sure being affected.
> > > > Althoguh I don't recall if I ever managed to hang it
> > > > using display registers specifically. intel_gpu_top
> > > > certainly was very good at reproducing the problem.
> > > > 
> > > > > I couldn't find any reference to
> > > > > this reason. 
> > > > 
> > > > If all else fails git log is your friend.
> > > 
> > > It seems to be documented in intel_uncore.h. Though that one
> > > mentions IVB instead of HSW for some reason. I don't recall
> > > seeing it on IVB myself, but I suppose it might have been an
> > > issue there as well. How long the problem remained after HSW
> > > I have no idea.
> > 
> > Paulo very recently told me that he could easily reproduce the issue
> > on IVB, simply by running 2 glxgears at the same time.
> 
> Just a minor correction: I didn't give the degree of confidence in my
> answer that the sentence above suggests :). It's all "as far as I
> remember". This is all from like 10 years ago and I can't remember what
> I had for lunch yesterday. Maybe it was some other similar bug that I
> could reproduce with glxgears. Also, the way we used registers was
> different back then, maybe today glxgears is not enough to do it
> anymore. And I think it required vblank_mode=0.

Great, thanks for this information! It's good to know the actual facts
for this implementation.  So, we'll keep things mostly as they are,
without removing any locking and go back to my original version of this
implementation, which keeps the locking with i915.

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/i915: don't use uncore spinlock to protect critical section in vblank

[Coelho, Luciano]

On Fri, 2023-11-17 at 12:46 +, Tvrtko Ursulin wrote:
> On 17/11/2023 12:21, Coelho, Luciano wrote:
> > Adding Tvrtko, for some reason he didn't get CCed before.
> > 
> > 
> > On Fri, 2023-11-17 at 11:26 +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 17, 2023 at 10:41:43AM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 17, 2023 at 08:05:21AM +, Coelho, Luciano wrote:
> > > > > Thanks for your comments, Ville!
> > > > > 
> > > > > On Fri, 2023-11-17 at 09:19 +0200, Ville Syrjälä wrote:
> > > > > > On Thu, Nov 16, 2023 at 01:27:00PM +0200, Luca Coelho wrote:
> > > > > > > Since we're abstracting the display code from the underlying 
> > > > > > > driver
> > > > > > > (i.e. i915 vs xe), we can't use the uncore's spinlock to protect
> > > > > > > critical sections of our code.
> > > > > > > 
> > > > > > > After further inspection, it seems that the spinlock is not 
> > > > > > > needed at
> > > > > > > all and this can be handled by disabling preemption and interrupts
> > > > > > > instead.
> > > > > > 
> > > > > > uncore.lock has multiple purposes:
> > > > > > 1. serialize all register accesses to the same cacheline as on
> > > > > > certain platforms that can hang the machine
> > > > > 
> > > > > Okay, do you remember which platforms?
> > > > 
> > > > HSW is the one I remember for sure being affected.
> > > > Althoguh I don't recall if I ever managed to hang it
> > > > using display registers specifically. intel_gpu_top
> > > > certainly was very good at reproducing the problem.
> > > > 
> > > > > I couldn't find any reference to
> > > > > this reason.
> > > > 
> > > > If all else fails git log is your friend.
> > > 
> > > It seems to be documented in intel_uncore.h. Though that one
> > > mentions IVB instead of HSW for some reason. I don't recall
> > > seeing it on IVB myself, but I suppose it might have been an
> > > issue there as well. How long the problem remained after HSW
> > > I have no idea.
> > 
> > Oh, somehow I missed that.  Thanks.
> > 
> > So, it seems that the locking is indeed needed.  I think there are two
> > options, then:
> > 
> > 1. Go back to my previous version of the patch, where I had the wrapper
> > that didn't lock anything on Xe and implement the display part when we
> > get a similar implementation of the uncore.lock on Xe (if ever needed).
> > 
> > 2. Implement a display-local lock for this, as suggested at some point,
> > including the other intel_de*() accesses.  But can we be sure that it's
> > enough to protect only the registers accessed by the display? I.e.
> > won't accessing display registers non-serially in relation to non-
> > display registers?
> > 
> > 
> > Preferences?
> 
> AFAIR my initial complaint was the naming which was along the lines of 
> intel_spin_lock(uncore). How to come up with a clean and logical name is 
> the question...

You're right, that was your first comment, and now we're back to it. :)


> On Xe you don't need a lock since HSW/IVB/cacheline angle does not exist 
> but you need a name which does not clash with either.
> 
> Assuming you still need the preempt off (or irq off) on Xe for better 
> timings, maybe along the lines of intel_vblank_section_enter/exit(i915)?

I like this name, because it's indeed this vblank section we're trying
to protect here, and this is not used anywhere else.

 
> Although I am not up to speed with what object instead of i915 would you 
> be passing in from Xe ie. how exactly polymorphism is implemented in 
> shared code.

AFAICT we are still using the i915 structure for most things inside the
display code, so I guess we should use that for now.

I'll send a new version of my original patch in a bit.

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/i915: don't use uncore spinlock to protect critical section in vblank

[Coelho, Luciano]

Adding Tvrtko, for some reason he didn't get CCed before.


On Fri, 2023-11-17 at 11:26 +0200, Ville Syrjälä wrote:
> On Fri, Nov 17, 2023 at 10:41:43AM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 17, 2023 at 08:05:21AM +0000, Coelho, Luciano wrote:
> > > Thanks for your comments, Ville!
> > > 
> > > On Fri, 2023-11-17 at 09:19 +0200, Ville Syrjälä wrote:
> > > > On Thu, Nov 16, 2023 at 01:27:00PM +0200, Luca Coelho wrote:
> > > > > Since we're abstracting the display code from the underlying driver
> > > > > (i.e. i915 vs xe), we can't use the uncore's spinlock to protect
> > > > > critical sections of our code.
> > > > > 
> > > > > After further inspection, it seems that the spinlock is not needed at
> > > > > all and this can be handled by disabling preemption and interrupts
> > > > > instead.
> > > > 
> > > > uncore.lock has multiple purposes:
> > > > 1. serialize all register accesses to the same cacheline as on
> > > >certain platforms that can hang the machine
> > > 
> > > Okay, do you remember which platforms?
> > 
> > HSW is the one I remember for sure being affected.
> > Althoguh I don't recall if I ever managed to hang it
> > using display registers specifically. intel_gpu_top
> > certainly was very good at reproducing the problem.
> > 
> > > I couldn't find any reference to
> > > this reason. 
> > 
> > If all else fails git log is your friend.
> 
> It seems to be documented in intel_uncore.h. Though that one
> mentions IVB instead of HSW for some reason. I don't recall
> seeing it on IVB myself, but I suppose it might have been an
> issue there as well. How long the problem remained after HSW
> I have no idea.

Oh, somehow I missed that.  Thanks.

So, it seems that the locking is indeed needed.  I think there are two
options, then:

1. Go back to my previous version of the patch, where I had the wrapper
that didn't lock anything on Xe and implement the display part when we
get a similar implementation of the uncore.lock on Xe (if ever needed).

2. Implement a display-local lock for this, as suggested at some point,
including the other intel_de*() accesses.  But can we be sure that it's
enough to protect only the registers accessed by the display? I.e.
won't accessing display registers non-serially in relation to non-
display registers?


Preferences?

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/i915: don't use uncore spinlock to protect critical section in vblank

[Coelho, Luciano]

On Fri, 2023-11-17 at 10:41 +0200, Ville Syrjälä wrote:
> On Fri, Nov 17, 2023 at 08:05:21AM +0000, Coelho, Luciano wrote:
> > Thanks for your comments, Ville!
> > 
> > On Fri, 2023-11-17 at 09:19 +0200, Ville Syrjälä wrote:
> > > On Thu, Nov 16, 2023 at 01:27:00PM +0200, Luca Coelho wrote:
> > > > Since we're abstracting the display code from the underlying driver
> > > > (i.e. i915 vs xe), we can't use the uncore's spinlock to protect
> > > > critical sections of our code.
> > > > 
> > > > After further inspection, it seems that the spinlock is not needed at
> > > > all and this can be handled by disabling preemption and interrupts
> > > > instead.
> > > 
> > > uncore.lock has multiple purposes:
> > > 1. serialize all register accesses to the same cacheline as on
> > >certain platforms that can hang the machine
> > 
> > Okay, do you remember which platforms?
> 
> HSW is the one I remember for sure being affected.
> Althoguh I don't recall if I ever managed to hang it
> using display registers specifically. intel_gpu_top
> certainly was very good at reproducing the problem.

So do we know if the display registers are affected at all?


> > I couldn't find any reference to
> > this reason. 
> 
> If all else fails git log is your friend.

Of course! That's where I found the info about the RT stuff.  But I
didn't find anything else regarding this.  Maybe I should just look
harder, if you don't have a reference at hand. ;)

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/i915: don't use uncore spinlock to protect critical section in vblank

[Coelho, Luciano]

Thanks for your comments, Ville!

On Fri, 2023-11-17 at 09:19 +0200, Ville Syrjälä wrote:
> On Thu, Nov 16, 2023 at 01:27:00PM +0200, Luca Coelho wrote:
> > Since we're abstracting the display code from the underlying driver
> > (i.e. i915 vs xe), we can't use the uncore's spinlock to protect
> > critical sections of our code.
> > 
> > After further inspection, it seems that the spinlock is not needed at
> > all and this can be handled by disabling preemption and interrupts
> > instead.
> 
> uncore.lock has multiple purposes:
> 1. serialize all register accesses to the same cacheline as on
>certain platforms that can hang the machine

Okay, do you remember which platforms? I couldn't find any reference to
this reason.  Also, the only place where where we take the uncore.lock
is in this vblank code I changed, where the only explanation I found
was about timing, specifically when using RT-kernels and in very old
and slow platforms... (this was added 10 years ago).


> 2. protect the forcewake/etc. state
> 
> 1 is relevant here, 2 is not.

Okay, good that we have only one known problem. :)

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v6] drm/i915/dsb: DSB code refactoring

[Coelho, Luciano]

On Tue, 2023-11-14 at 11:35 +, Manna, Animesh wrote:
> 
> > -Original Message-
> > From: Coelho, Luciano 
> > Sent: Tuesday, November 14, 2023 4:47 PM
> > To: Manna, Animesh ; intel-
> > g...@lists.freedesktop.org
> > Cc: Nikula, Jani 
> > Subject: Re: [Intel-gfx] [PATCH v6] drm/i915/dsb: DSB code refactoring
> > 
> > On Fri, 2023-11-10 at 08:55 +0530, Animesh Manna wrote:
> > > Refactor DSB implementation to be compatible with Xe driver.
> > > 
> > > v1: RFC version.
> > > v2: Make intel_dsb structure opaque from external usage. [Jani]
> > > v3: Rebased on latest.
> > > v4:
> > > - Add boundary check in dsb_buffer_memset(). [Luca]
> > > - Use size_t instead of u32. [Luca]
> > > v5: WARN_ON() added for out of boudary case with some optimization.
> > [Luca]
> > 
> > ...and what is different in v6?
> 
> Rebased on latest and fix a rebase-miss which CI did not caught well. Before 
> merging want the green signal from CI.

Okay, it's good to mention that.

Anyway, my r-b is still valid. ;)

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v6] drm/i915/dsb: DSB code refactoring

[Coelho, Luciano]

On Fri, 2023-11-10 at 08:55 +0530, Animesh Manna wrote:
> Refactor DSB implementation to be compatible with Xe driver.
> 
> v1: RFC version.
> v2: Make intel_dsb structure opaque from external usage. [Jani]
> v3: Rebased on latest.
> v4:
> - Add boundary check in dsb_buffer_memset(). [Luca]
> - Use size_t instead of u32. [Luca]
> v5: WARN_ON() added for out of boudary case with some optimization. [Luca]

...and what is different in v6?

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Fri, 2023-11-03 at 03:31 +, Vivi, Rodrigo wrote:
> > > 
> > > Any other suggestions?
> > 
> > I think it will boil down to the reason uncore lock is held over
> > the 
> > respective sections ie. the comment in i915_get_crtc_scanoutpos.
> > 
> > If it is timing sensitive to the extent irq off was needed it may
> > apply 
> > to Xe as well. Likewise the need to use mmio helpers which rely on
> > the 
> > uncore lock already held. Question of whether there is conceptual 
> > commonality, will probably drive the best name, or the best
> > approach
> > in 
> > general.
> 
> yeap, this is how I'm seeing this. If i915-display needs this global
> lock around mmio operations, then we wound need to add it to the
> xe_mmio as well and then solve the name, etc.
> 
> However, I don't believe that other users of the mmio would need
> this lock. So I believe the right thing to do is to create a i915-
> display only spin_lock, around the intel_de_mmio calls and here.
> 
> With this we entirely kill the dependency on someone-else's lock
> and have something that is entirely inside display code so it
> doesn't need to be ported to one or another driver core components.

Right, I agree too.

My patch was just trying to address the quick hack made for Xe, not the
actual implementation in Xe's side.  But it makes sense to implement
this new lock internally in the display so there are no dependencies or
wrappers needed.

I'll respin.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Wed, 2023-10-25 at 11:25 +0100, Tvrtko Ursulin wrote:
> On 25/10/2023 11:18, Tvrtko Ursulin wrote:
> > 
> > On 23/10/2023 11:33, Luca Coelho wrote:
> > > The uncore code may not always be available (e.g. when we build the
> > > display code with Xe), so we can't always rely on having the uncore's
> > > spinlock.
> > > 
> > > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > > create wrapper functions for locking and unlocking the uncore's
> > > spinlock.  In these functions, we have a condition check and only
> > > actually try to lock/unlock the spinlock when I915 is defined, and
> > > thus uncore is available.
> > > 
> > > This keeps the ifdefs contained in these new functions and all such
> > > logic inside the display code.
> > > 
> > > Signed-off-by: Luca Coelho 
> > > ---
> > > 
> > > In v2:
> > > 
> > >     * Renamed uncore_spin_*() to intel_spin_*()
> > >     * Corrected the order: save, lock, unlock, restore
> > > 
> > > In v3:
> > > 
> > >     * Undid the change to pass drm_i915_private instead of the lock
> > >   itself, since we would have to include i915_drv.h and that pulls
> > >   in a truckload of other includes.
> > > 
> > >   drivers/gpu/drm/i915/display/intel_display.h | 20 
> > >   drivers/gpu/drm/i915/display/intel_vblank.c  | 19 ---
> > >   2 files changed, 32 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
> > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > index 0e5dffe8f018..2a33fcc8ce68 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -559,4 +559,24 @@ bool assert_port_valid(struct drm_i915_private 
> > > *i915, enum port port);
> > >   bool intel_scanout_needs_vtd_wa(struct drm_i915_private *i915);
> > > +/*
> > > + * The uncore version of the spin lock functions is used to decide
> > > + * whether we need to lock the uncore lock or not.  This is only
> > > + * needed in i915, not in Xe.  Keep the decision-making centralized
> > > + * here.
> > > + */
> > > +static inline void intel_spin_lock(spinlock_t *lock)
> > > +{
> > > +#ifdef I915
> > > +    spin_lock(lock);
> > > +#endif
> > > +}
> > > +
> > > +static inline void intel_spin_unlock(spinlock_t *lock)
> > > +{
> > > +#ifdef I915
> > > +    spin_unlock(lock);
> > > +#endif
> > > +}
> > > +
> > >   #endif
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c 
> > > b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > index 2cec2abf9746..9b482d648762 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > @@ -306,7 +306,8 @@ static bool i915_get_crtc_scanoutpos(struct 
> > > drm_crtc *_crtc,
> > >    * register reads, potentially with preemption disabled, so the
> > >    * following code must not block on uncore.lock.
> > >    */
> > > -    spin_lock_irqsave(_priv->uncore.lock, irqflags);
> > > +    local_irq_save(irqflags);
> > 
> > Does Xe needs interrupts off?

I'm actually not sure, but this is how it was in the Xe driver code, so
I kept it.


> > > +    intel_spin_lock(_priv->uncore.lock);
> > 
> > My 2p/c is that intel_spin_lock as a name does not work when it is 
> > specifically about the single and specific (uncore) lock. One cannot 
> > call intel_spin_lock(some->other->lock) etc.

Right, this was changed when I was passing only dev_priv, but I
couldn't do that wihtout adding i915_drv.h, which was not good
either... But yeah, this is too generic, while the actual case is not.


> > Perhaps call it i915_uncore_lock_irqsave(i915, flags) so it is clear it 
> > is only for i915.

I wanted to avoid "i915", since we also call it when the display is
used with xe...


> Or, if the implementation will later gain the #else block for Xe, 
> perhaps intel_uncore_lock_...?

But still, uncore doesn't exist in Xe, so this is still not good...

Any other suggestions?

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v4 21/23] drm/i915/display: Move verbose_state_checks under display

[Coelho, Luciano]

On Tue, 2023-10-24 at 15:41 +0300, Jouni Högander wrote:
> v2: Change device parameter permissions to 0400

I think it should be v4 here? Anyway, I don't care, I think I already
expressed my opinion against having the patch revision history in the
commit message at all...

> 
> Cc: Luca Coelho 
> Cc: Jani Nikula 
> 
> Signed-off-by: Jouni Högander 
> ---

Reviewed-by: Luca Coelho 

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH v2] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Mon, 2023-10-23 at 13:21 +0300, Jani Nikula wrote:
> On Mon, 23 Oct 2023, "Coelho, Luciano"  wrote:
> > On Mon, 2023-10-23 at 12:11 +0300, Jani Nikula wrote:
> > > On Mon, 23 Oct 2023, Luca Coelho  wrote:
> > > > The uncore code may not always be available (e.g. when we build the
> > > > display code with Xe), so we can't always rely on having the uncore's
> > > > spinlock.
> > > > 
> > > > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > > > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > > > create wrapper functions for locking and unlocking the uncore's
> > > > spinlock.  In these functions, we have a condition check and only
> > > > actually try to lock/unlock the spinlock when I915 is defined, and
> > > > thus uncore is available.
> > > > 
> > > > This keeps the ifdefs contained in these new functions and all such
> > > > logic inside the display code.
> > > > 
> > > > Signed-off-by: Luca Coelho 
> > > > ---
> > > > 
> > > > Note: this patch was accidentally sent only to intel-xe[1], but should
> > > > have been sent to intel-gfx.  Thus, this is v2.
> > > > 
> > > > In v2:
> > > > 
> > > >* Renamed uncore_spin_*() to intel_spin_*()
> > > >* Corrected the order: save, lock, unlock, restore
> > > > 
> > > > [1] https://patchwork.freedesktop.org/patch/563288/
> > > > 
> > > > 
> > > >  drivers/gpu/drm/i915/display/intel_display.h | 22 +++-
> > > >  drivers/gpu/drm/i915/display/intel_vblank.c  | 19 ++---
> > > >  2 files changed, 33 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
> > > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > > index 0e5dffe8f018..099476906f4c 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > > @@ -29,6 +29,7 @@
> > > >  
> > > >  #include "i915_reg_defs.h"
> > > >  #include "intel_display_limits.h"
> > > > +#include "i915_drv.h"
> > > 
> > > In general, please avoid including headers from headers. In particular,
> > > please don't include i915_drv.h from headers. The header
> > > interdependencies are pretty bad already, and we need to clean it up.
> > 
> > Right, I thought twice about this, but this seems far from clean, as
> > you say, so I thought it would be fine.
> 
> Adding that single include bumps the total recursive includes of this
> file from 2 to 97... i915_drv.h is pretty bad.

Argh.  I'm sending a v3 asap! :)

--
Luca.

Re: [Intel-gfx] [PATCH v2] drm/i915: handle uncore spinlock when not available

[Coelho, Luciano]

On Mon, 2023-10-23 at 12:11 +0300, Jani Nikula wrote:
> On Mon, 23 Oct 2023, Luca Coelho  wrote:
> > The uncore code may not always be available (e.g. when we build the
> > display code with Xe), so we can't always rely on having the uncore's
> > spinlock.
> > 
> > To handle this, split the spin_lock/unlock_irqsave/restore() into
> > spin_lock/unlock() followed by a call to local_irq_save/restore() and
> > create wrapper functions for locking and unlocking the uncore's
> > spinlock.  In these functions, we have a condition check and only
> > actually try to lock/unlock the spinlock when I915 is defined, and
> > thus uncore is available.
> > 
> > This keeps the ifdefs contained in these new functions and all such
> > logic inside the display code.
> > 
> > Signed-off-by: Luca Coelho 
> > ---
> > 
> > Note: this patch was accidentally sent only to intel-xe[1], but should
> > have been sent to intel-gfx.  Thus, this is v2.
> > 
> > In v2:
> > 
> >* Renamed uncore_spin_*() to intel_spin_*()
> >* Corrected the order: save, lock, unlock, restore
> > 
> > [1] https://patchwork.freedesktop.org/patch/563288/
> > 
> > 
> >  drivers/gpu/drm/i915/display/intel_display.h | 22 +++-
> >  drivers/gpu/drm/i915/display/intel_vblank.c  | 19 ++---
> >  2 files changed, 33 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index 0e5dffe8f018..099476906f4c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -29,6 +29,7 @@
> >  
> >  #include "i915_reg_defs.h"
> >  #include "intel_display_limits.h"
> > +#include "i915_drv.h"
> 
> In general, please avoid including headers from headers. In particular,
> please don't include i915_drv.h from headers. The header
> interdependencies are pretty bad already, and we need to clean it up.

Right, I thought twice about this, but this seems far from clean, as
you say, so I thought it would be fine.

There's not much point, though, since we have a declaration for
drm_i915_private in this file anyway, so the dependency is still
there...

In any case, I'll unspin this change and go back to passing the actual
lock instead of passing drm_i915_private.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v2] drm/i915: Only check eDP HPD when AUX CH is shared

[Coelho, Luciano]

On Fri, 2023-09-08 at 08:25 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Apparently Acer Chromebook C740 (BDW-ULT) doesn't have the
> eDP HPD line properly connected, and thus fails the new
> HPD check during eDP probe. The result is that we lose the
> eDP output.
> 
> I suspect all such machines would all be Chromebooks or other

Small duplication here "...all such machines would all...".


> Linux exclusive systems as the Windows driver likely wouldn't
> work either. I did check a few other BDW machines here and
> those do have eDP HPD connected, one of them even is a
> different Chromebook (Samus).
> 
> To account for these funky machines let's skip the HPD check when
> it looks like the eDP port is the only one using that specific AUX
> channel. In case of multiple ports sharing the same AUX CH (eg. on
> Asrock B250M-HDV) we still do the check and thus should correctly
> ignore the eDP port in favor of the other DP port (usually a DP->VGA
> converter).
> 
> v2: Don't oops during list iteration
> 
> Cc: sta...@vger.kernel.org
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9264
> Fixes: cfe5bdfb27fa ("drm/i915: Check HPD live state during eDP probe")
> Signed-off-by: Ville Syrjälä 
> ---

Regardless of the small grammatical issue, LGTM:

Reviewed-by: Luca Coelho 

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v4 0/4] drm/i915/tc: some clean-ups in max lane count handling code

[Coelho, Luciano]

On Tue, 2023-09-05 at 19:49 -0700, Lucas De Marchi wrote:
> On Fri, Aug 25, 2023 at 11:16:34AM +0300, Luca Coelho wrote:
> > Hi,
> > 
> > Here are four patches with some clean-ups in the code that handles the
> > max lane count of Type-C connections.
> > 
> > This is done mostly in preparation for a new way to read the pin
> > assignments and lane count in future devices.
> > 
> > In v2:
> >   * Fix some rebasing damage.
> > 
> > In v3:
> >   * Fixed "assigment" typo, as reported by checkpatch.
> > 
> > In v4:
> >   * Rebased;
> >   * Renamed port_max to lane_max (Lucas' comment).
> > 
> > Please review.
> 
> All patches are reviewed. I looked to the CI results and none of the
> regressions seem related.
> 
> Pushed. Thanks

Great, thanks for checking and pushing, Lucas!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v4 1/4] drm/i915/fbc: Clear frontbuffer busy bits on flip

[Coelho, Luciano]

On Mon, 2023-09-04 at 08:40 +, Hogander, Jouni wrote:
> On Mon, 2023-09-04 at 07:25 +0000, Coelho, Luciano wrote:
> > Hi Jouni,
> > 
> > On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote:
> > > We are planning to move flush performed from work queue. This
> > > means it is possible to have invalidate -> flip -> flush sequence.
> > > Handle this by clearing possible busy bits on flip.
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > Signed-off-by: Jouni Högander 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 1c6d467cec26..817e5784660b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct
> > > intel_fbc *fbc)
> > > lockdep_assert_held(>lock);
> > >  
> > > fbc->flip_pending = false;
> > > +   fbc->busy_bits = 0;
> > >  
> > > -   if (!fbc->busy_bits)
> > > -   intel_fbc_activate(fbc);
> > > -   else
> > > -   intel_fbc_deactivate(fbc, "frontbuffer write");
> > > +   intel_fbc_activate(fbc);
> > 
> > Can you explain why the call to intel_fbc_deactivate() is not needed
> > here anymore? I think it would be a good idea to explain that in the
> > commit message.  Or, at least, an explanation about it here, so it's
> > documented. ;)
> 
> We are clearing fbc->busy_bits -> I.e. if(!fbc->busy_bits) is always
> taken :
> 
> Post plane update is called at the end of the flip. If you consider
> case where busy_bits != 0 at this point: it means someone have
> initiated frontbuffer write (invalidate) which is not yet completed
> (flush from workqueue). That flush pending in workqueue is not valid
> anymore as there was a flip and the buffer which was frontbuffer is not
> a frontbuffer anymore. Even if the same buffer would be used when doing
> a flip the atomic commit would take care of flushing the buffer towards
> fbc. Also waiting for dma fences is take caren by the atomic commit
> code.

Thanks for the explanation! It makes sense.

So you have my:

Reviewed-by: Luca Coelho 

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v4 1/4] drm/i915/fbc: Clear frontbuffer busy bits on flip

[Coelho, Luciano]

Hi Jouni,

On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote:
> We are planning to move flush performed from work queue. This
> means it is possible to have invalidate -> flip -> flush sequence.
> Handle this by clearing possible busy bits on flip.
> 
> Signed-off-by: Ville Syrjälä 
> Signed-off-by: Jouni Högander 
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c 
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 1c6d467cec26..817e5784660b 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct intel_fbc 
> *fbc)
>   lockdep_assert_held(>lock);
>  
>   fbc->flip_pending = false;
> + fbc->busy_bits = 0;
>  
> - if (!fbc->busy_bits)
> - intel_fbc_activate(fbc);
> - else
> - intel_fbc_deactivate(fbc, "frontbuffer write");
> + intel_fbc_activate(fbc);

Can you explain why the call to intel_fbc_deactivate() is not needed
here anymore? I think it would be a good idea to explain that in the
commit message.  Or, at least, an explanation about it here, so it's
documented. ;)

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 27/42] drm/i915/xe2lpd: Read pin assignment from IOM

[Coelho, Luciano]

On Wed, 2023-08-23 at 10:07 -0700, Lucas De Marchi wrote:
> From: Luca Coelho 
> 
> Starting from display version 20, we need to read the pin assignment
> from the IOM TCSS_DDI_STATUS register instead of reading it from the
> FIA.
> 
> We use the pin assignment to decide the maximum lane count.  So, to
> support this change, add a new lnl_tc_port_get_max_lane_count() function
> that reads from the TCSS_DDI_STATUS register and decides the maximum
> lane count based on that.
> 
> BSpec: 69594
> Cc: Mika Kahola 
> Signed-off-by: Luca Coelho 
> Signed-off-by: Lucas De Marchi 
> ---

Lucas, do you want me to send this together with my patchset with the
preparation for this?

In a way, the 4 patches I sent out can be applied independently of LNL-
related changes, so maybe I could resend just those 4 patches and you
base your entire series on top of my patches after they get applied?
Then this patch, which is really related to LNL could be part of your
series...

Let me know what you prefer.

--
Cheers,
Luca.

Re: [Intel-gfx] [Intel-xe] [PATCH 27/42] drm/i915/xe2lpd: Read pin assignment from IOM

[Coelho, Luciano]

On Wed, 2023-08-23 at 13:28 -0700, Matt Roper wrote:
> On Wed, Aug 23, 2023 at 10:07:25AM -0700, Lucas De Marchi wrote:
> > From: Luca Coelho 
> > 
> > Starting from display version 20, we need to read the pin assignment
> > from the IOM TCSS_DDI_STATUS register instead of reading it from the
> > FIA.
> > 
> > We use the pin assignment to decide the maximum lane count.  So, to
> > support this change, add a new lnl_tc_port_get_max_lane_count() function
> > that reads from the TCSS_DDI_STATUS register and decides the maximum
> > lane count based on that.
> > 
> > BSpec: 69594
> > Cc: Mika Kahola 
> > Signed-off-by: Luca Coelho 
> > Signed-off-by: Lucas De Marchi 
> > ---
> >  drivers/gpu/drm/i915/display/intel_tc.c | 28 +
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c 
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 3c94bbcb5497..37b0f8529b4f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -290,6 +290,31 @@ u32 intel_tc_port_get_pin_assignment_mask(struct 
> > intel_digital_port *dig_port)
> >DP_PIN_ASSIGNMENT_SHIFT(tc->phy_fia_idx);
> >  }
> >  
> > +static int lnl_tc_port_get_max_lane_count(struct intel_digital_port 
> > *dig_port)
> > +{
> > +   struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > +   enum tc_port tc_port = intel_port_to_tc(i915, dig_port->base.port);
> > +   intel_wakeref_t wakeref;
> > +   u32 val, pin_assignment;
> > +
> > +   with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> 
> Do we need this?  I don't think POWER_DOMAIN_DISPLAY_CORE has been tied
> to any power wells since VLV/CHV.
> 
> Hmm, it looks like we actually grab it (and even assert it) in a bunch of
> places on modern platforms that don't make sense to me since it isn't
> tied to anything.
> 
> I guess leaving this here doesn't hurt anything, although we might want
> to go back and take another look at this in the future.
> 
> Reviewed-by: Matt Roper 

Thanks, Matt! You have a good point, but as you said, maybe this should
be revisited in all occurrences and changed in one go.  I just kept it
consistent with other usage.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Coelho, Luciano]

On Thu, 2023-08-24 at 11:12 +, Kandpal, Suraj wrote:
> > On Wed, 2023-08-16 at 08:54 +, Kandpal, Suraj wrote:
> > > > This makes the code a bit more symmetric and readable,
> > > > especially
> > > > when we start adding more display version-specific
> > > > alternatives.
> > > > 
> > > > Signed-off-by: Luca Coelho 
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++--
> > > > 
> > > > 
> > > >  1 file changed, 19 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > index de848b329f4b..43b8eeba26f8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > @@ -311,23 +311,12 @@ static int
> > > > mtl_tc_port_get_max_lane_count(struct
> > > > intel_digital_port *dig_port)
> > > >     }
> > > >  }
> > > > 
> > > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > > *dig_port)
> > > > +static int intel_tc_port_get_max_lane_count(struct
> > > > intel_digital_port
> > > > +*dig_port)
> > > >  {
> > > >     struct drm_i915_private *i915 = to_i915(dig_port-
> > > > > base.base.dev);
> > > > -   struct intel_tc_port *tc = to_tc_port(dig_port);
> > > > -   enum phy phy = intel_port_to_phy(i915, dig_port-
> > > > > base.port);
> > > >     intel_wakeref_t wakeref;
> > > > -   u32 lane_mask;
> > > > -
> > > > -   if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > > TC_PORT_DP_ALT)
> > > > -   return 4;
> > > > +   u32 lane_mask = 0;
> > > > 
> > > > -   assert_tc_cold_blocked(tc);
> > > > -
> > > > -   if (DISPLAY_VER(i915) >= 14)
> > > > -   return
> > > > mtl_tc_port_get_max_lane_count(dig_port);
> > > > -
> > > > -   lane_mask = 0;
> > > >     with_intel_display_power(i915,
> > > > POWER_DOMAIN_DISPLAY_CORE,
> > > > wakeref)
> > > >     lane_mask =
> > > > intel_tc_port_get_lane_mask(dig_port);
> > > > 
> > > > @@ -348,6 +337,23 @@ int
> > > > intel_tc_port_fia_max_lane_count(struct
> > > > intel_digital_port *dig_port)
> > > >     }
> > > >  }
> > > > 
> > > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > > +*dig_port) {
> > > > +   struct drm_i915_private *i915 = to_i915(dig_port-
> > > > > base.base.dev);
> > > > +   struct intel_tc_port *tc = to_tc_port(dig_port);
> > > > +   enum phy phy = intel_port_to_phy(i915, dig_port-
> > > > > base.port);
> > > > +
> > > > +   if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > > TC_PORT_DP_ALT)
> > > > +   return 4;
> > > > +
> > > > +   assert_tc_cold_blocked(tc);
> > > > +
> > > > +   if (DISPLAY_VER(i915) >= 14)
> > > > +   return
> > > > mtl_tc_port_get_max_lane_count(dig_port);
> > > > +
> > > > +   return intel_tc_port_get_max_lane_count(dig_port);
> > > > +}
> > > 
> > > Looking at this I think we have more scope of optimization here I
> > > think we can go about it in the following way
> > > 
> > > intel_tc_port_fia_max_lane_count
> > > already calls
> > > with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> > > wakeref)
> > > lane_mask =
> > > intel_tc_port_get_lane_mask(dig_port);
> > > 
> > > which we also duplicate in  mtl_tc_port_get_pin_assignment_mask
> > > (now mtl_tc_port_get_max_lane_count) and the only difference
> > > between
> > > them Is the switch case what if we have a function or repurpose
> > > mtl_tc_port_get_max_lane_count to only have the switch case block
> > > with
> > > assignment varied by display version and call it after
> > > intel_tc_port_get_lane_mask
> > > 
> > > maybe also move the legacy switch case in its own function?
> > 
> > This would be another option, but I think it's nicer to keep things
> > very isolated
> > from each other and this tiny code duplication is not too bad.
> > 
> > Especially because later, as you can see in my LNL patch that Lucas
> > sent out[1]
> > we have another combination in a new function.  So we have:
> > 
> > intel_tc_port_get_max_lane_count();
> > intel_tc_port_get_lane_mask();
> > switch with lane_mask;
> > 
> > mlt_tc_port_get_lane_mask(dig_port);
> > intel_tc_port_get_pin_assignment_mask();
> > switch with pin_mask;
> > 
> > lnl_tc_port_get_lane_mask():
> > intel_uncore_read(uncore, TCSS_DDI_STATUS(tc_port));
> > switch with pin_assignment;
> > 
> > 
> > So now we have 3 different ways to read and two different ways to
> > parse (with
> > the switch-case).  I think it's a lot cleaner to keep this all
> > separate than to try
> > to reuse these small pieces of code, which would make it a bit
> > harder to read.
> > 
> > Makes sense?
> 
> Good with it
> 
> Reviewed-by: Suraj Kandpal 

Thanks, Suraj!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 09/42] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Coelho, Luciano]

On Thu, 2023-08-24 at 05:43 +, Kandpal, Suraj wrote:
> 
> > -Original Message-
> > From: Intel-gfx  On Behalf Of Lucas
> > De Marchi
> > Sent: Wednesday, August 23, 2023 10:37 PM
> > To: intel...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > Cc: Coelho, Luciano 
> > Subject: [Intel-gfx] [PATCH 09/42] drm/i915/tc: move legacy code out of the
> > main _max_lane_count() func
> > 
> > From: Luca Coelho 
> > 
> > This makes the code a bit more symmetric and readable, especially when we
> > start adding more display version-specific alternatives.
> > 
> > Signed-off-by: Luca Coelho 
> > Link: https://lore.kernel.org/r/2023072121.369227-4-
> > luciano.coe...@intel.com
> > ---
> >  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++--
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index de848b329f4b..43b8eeba26f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -311,23 +311,12 @@ static int mtl_tc_port_get_max_lane_count(struct
> > intel_digital_port *dig_port)
> > }
> >  }
> > 
> > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
> > +static int intel_tc_port_get_max_lane_count(struct intel_digital_port
> > +*dig_port)
> >  {
> > struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > -   struct intel_tc_port *tc = to_tc_port(dig_port);
> > -   enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
> > intel_wakeref_t wakeref;
> > -   u32 lane_mask;
> > -
> > -   if (!intel_phy_is_tc(i915, phy) || tc->mode != TC_PORT_DP_ALT)
> > -   return 4;
> > +   u32 lane_mask = 0;
> > 
> > -   assert_tc_cold_blocked(tc);
> > -
> > -   if (DISPLAY_VER(i915) >= 14)
> > -   return mtl_tc_port_get_max_lane_count(dig_port);
> > -
> > -   lane_mask = 0;
> > with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> > wakeref)
> > lane_mask = intel_tc_port_get_lane_mask(dig_port);
> > 
> > @@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct
> > intel_digital_port *dig_port)
> > }
> >  }
> 
> Rather than having two functions:
> mtl_tc_port_get_max_lane_count()
> & intel_tc_port_get_max_lane_count() that both call:
> with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> lane_mask = intel_tc_port_get_lane_mask(dig_port);
> to get the lane mask the us directly pass the lane_mask to the above two 
> functions
> and make the call for getting the lane_mask common i.e lets call it in 
> intel_tc_port_fia_max_lane_count().

As I wrote in reply to your previous comment, this changes later, when
we add the special case for LNL.  So I don't think it will help much to
combine this call into a single function.  IMHO it's cleaner to have
them all cleanly separated in different functions, for readability. 
The compiler will certainly optimize all this in its own ways anyway.

Do you agree?

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Coelho, Luciano]

On Wed, 2023-08-16 at 08:54 +, Kandpal, Suraj wrote:
> > This makes the code a bit more symmetric and readable, especially
> > when we
> > start adding more display version-specific alternatives.
> > 
> > Signed-off-by: Luca Coelho 
> > ---
> >  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++--
> > 
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index de848b329f4b..43b8eeba26f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -311,23 +311,12 @@ static int
> > mtl_tc_port_get_max_lane_count(struct
> > intel_digital_port *dig_port)
> >     }
> >  }
> > 
> > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > *dig_port)
> > +static int intel_tc_port_get_max_lane_count(struct
> > intel_digital_port
> > +*dig_port)
> >  {
> >     struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > -   struct intel_tc_port *tc = to_tc_port(dig_port);
> > -   enum phy phy = intel_port_to_phy(i915, dig_port-
> > >base.port);
> >     intel_wakeref_t wakeref;
> > -   u32 lane_mask;
> > -
> > -   if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > TC_PORT_DP_ALT)
> > -   return 4;
> > +   u32 lane_mask = 0;
> > 
> > -   assert_tc_cold_blocked(tc);
> > -
> > -   if (DISPLAY_VER(i915) >= 14)
> > -   return mtl_tc_port_get_max_lane_count(dig_port);
> > -
> > -   lane_mask = 0;
> >     with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> > wakeref)
> >     lane_mask = intel_tc_port_get_lane_mask(dig_port);
> > 
> > @@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct
> > intel_digital_port *dig_port)
> >     }
> >  }
> > 
> > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > +*dig_port) {
> > +   struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > +   struct intel_tc_port *tc = to_tc_port(dig_port);
> > +   enum phy phy = intel_port_to_phy(i915, dig_port-
> > >base.port);
> > +
> > +   if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > TC_PORT_DP_ALT)
> > +   return 4;
> > +
> > +   assert_tc_cold_blocked(tc);
> > +
> > +   if (DISPLAY_VER(i915) >= 14)
> > +   return mtl_tc_port_get_max_lane_count(dig_port);
> > +
> > +   return intel_tc_port_get_max_lane_count(dig_port);
> > +}
> 
> Looking at this I think we have more scope of optimization here I
> think we can go about it in the following way
> 
> intel_tc_port_fia_max_lane_count
> already calls 
> with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> lane_mask = intel_tc_port_get_lane_mask(dig_port);
> 
> which we also duplicate in  mtl_tc_port_get_pin_assignment_mask
> (now mtl_tc_port_get_max_lane_count) and the only difference between
> them
> Is the switch case what if we have a function or repurpose 
> mtl_tc_port_get_max_lane_count to only have the switch case block
> with 
> assignment varied by display version and call it after
> intel_tc_port_get_lane_mask
> 
> maybe also move the legacy switch case in its own function?

This would be another option, but I think it's nicer to keep things
very isolated from each other and this tiny code duplication is not too
bad.

Especially because later, as you can see in my LNL patch that Lucas
sent out[1] we have another combination in a new function.  So we have:

intel_tc_port_get_max_lane_count();
intel_tc_port_get_lane_mask();
switch with lane_mask;

mlt_tc_port_get_lane_mask(dig_port);
intel_tc_port_get_pin_assignment_mask();
switch with pin_mask;

lnl_tc_port_get_lane_mask():
intel_uncore_read(uncore, TCSS_DDI_STATUS(tc_port));
switch with pin_assignment;


So now we have 3 different ways to read and two different ways to parse
(with the switch-case).  I think it's a lot cleaner to keep this all
separate than to try to reuse these small pieces of code, which would
make it a bit harder to read.

Makes sense?

[1] 
https://patchwork.kernel.org/project/intel-gfx/patch/20230823170740.1180212-28-lucas.demar...@intel.com/

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 0/4] drm/i915/tc: some clean-ups in max lane count handling code

[Coelho, Luciano]

On Mon, 2023-08-21 at 17:27 +, Kandpal, Suraj wrote:
> 0/4] drm/i915/tc: some clean-ups in max
> > lane count handling code
> > 
> > On Fri, Jul 21, 2023 at 02:11:17PM +0300, Luca Coelho wrote:
> > > Hi,
> > > 
> > > Here are four patches with some clean-ups in the code that handles the
> > > max lane count of Type-C connections.
> > > 
> > > This is done mostly in preparation for a new way to read the pin
> > > assignments and lane count in future devices.
> > > 
> > > In v2:
> > >   * Fix some rebasing damage.
> > > 
> > > In v3:
> > >   * Fixed "assigment" typo, as reported by checkpatch.
> > > 
> > > Please review.
> > 
> > what happened to this series? It seems only the last patch is not reviewed.
> > Are you going to submit a rebased version?
> > 
> 
> So I had some review comments on patch 3 was waiting for Luciano to upstream
> the latest changes then review the 4th patch

Sorry for the delay, I've been focusing on a high-priority bug.

I'll send a new version out soon.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

[Coelho, Luciano]

On Fri, 2023-08-18 at 20:50 -0700, Lucas De Marchi wrote:
> On Wed, Aug 16, 2023 at 09:08:44AM +0000, Coelho, Luciano wrote:
> > On Wed, 2023-08-16 at 08:13 +, Kandpal, Suraj wrote:
> > > > This function doesn't really return the pin assignment mask, but
> > > > the max lane
> > > > count derived from that.  So rename the function to
> > > > mtl_tc_port_get_max_lane_count() to better reflect what it really
> > > > does.
> > > > 
> > > Maybe also add the version changes on commit messages here as cover
> > > letter ends up getting discarded
> > 
> > Ah, right.  I discussed this with someone else before and we agreed to
> > disagree.  I don't really see the point in having the change history
> > in the commit itself for the mainline.  The discussions should be
> > openly available in the mailing list archives, so duplicating it in the
> > commit logs, IMHO, is moot.
> > 
> > A link in the commit log to lore, for instance, would add much more
> > value IMHO.
> > 
> > But anyway, since this guideline was already in place when I came, I
> > will (almost grudgingly) comply. 
> 
> some people want them, some people want them removed. A lot of people in
> drm like it while people outside will shout loudly if you add that.
> Don't let this hold off getting the patch into a mergeable state. 
> 
> 
> Reviewed-by: Lucas De Marchi 
> 
> It may need a rebase though.

Thanks, Lucas.  I'll keep the version history out then, if that's a
possibility, I prefer it. 

[...]

> > > 
> > > With that fixed
> > > 
> > > Reviewed-by: Suraj Kandpal 

Suraj, let me know if you want me to add your r-b tag even though I'm
not going to add the history.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

[Coelho, Luciano]

On Wed, 2023-08-16 at 08:13 +, Kandpal, Suraj wrote:
> > This function doesn't really return the pin assignment mask, but
> > the max lane
> > count derived from that.  So rename the function to
> > mtl_tc_port_get_max_lane_count() to better reflect what it really
> > does.
> > 
> Maybe also add the version changes on commit messages here as cover
> letter ends up getting discarded

Ah, right.  I discussed this with someone else before and we agreed to
disagree.  I don't really see the point in having the change history
in the commit itself for the mainline.  The discussions should be
openly available in the mailing list archives, so duplicating it in the
commit logs, IMHO, is moot.

A link in the commit log to lore, for instance, would add much more
value IMHO.

But anyway, since this guideline was already in place when I came, I
will (almost grudgingly) comply. 

> 
> With that fixed
> 
> Reviewed-by: Suraj Kandpal 

Thanks!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/display: pre-initialize some values in probe_gmdid_display()

[Coelho, Luciano]

On Thu, 2023-06-22 at 10:08 +, Kandpal, Suraj wrote:
> > On Tue, 2023-06-20 at 10:30 +, Kandpal, Suraj wrote:
> > > > When intel_display_device_probe() (and, subsequently,
> > > > probe_gmdid_display()) returns, the caller expects ver, rel and
> > > > step
> > > > to be initialized.  Since there's no way to check that there
> > > > was a
> > > > failure and no_display was returned without some further
> > > > refactoring, pre- initiliaze all these values to zero to keep
> > > > it
> > > > simple and safe.
> > > > 
> > > > Signed-off-by: Luca Coelho 
> > > 
> > > Looks okay to me just a small suggestion/question below.
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display_device.c | 9
> > > > +
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git
> > > > a/drivers/gpu/drm/i915/display/intel_display_device.c
> > > > b/drivers/gpu/drm/i915/display/intel_display_device.c
> > > > index 464df1764a86..fb6354e9e704 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > > > @@ -731,6 +731,15 @@ probe_gmdid_display(struct
> > > > drm_i915_private
> > > > *i915, u16 *ver, u16 *rel, u16 *step
> > > >     u32 val;
> > > >     int i;
> > > > 
> > > > +   /* The caller expects to ver, rel and step to be
> > > > initialized
> > > > +* here, and there's no good way to check when there
> > > > was a
> > > > +* failure and no_display was returned.  So initialize
> > > > all
> > > > these
> > > > +* values here zero, to be sure.
> > > > +*/
> > > > +   *ver = 0;
> > > > +   *rel = 0;
> > > > +   *step = 0;
> > > > +
> > > 
> > > From what I can see this is only called from
> > > intel_display_device_probe() which is in turn called from
> > > intel_device_info_driver_create() where the above variables are
> > > defined maybe we initialize these values there itself.
> > 
> > Thanks for the review!
> > 
> > I thought about initializing the variables on the caller side, but
> > reckoned that
> > it would be more intuitive to initialize them in the
> > probe_gmdid_display() function instead, because the caller expects
> > those
> > values to be set in successful cases and there's no way for it to
> > know whether
> > there was a failure or not (because we return a pointer to local
> > no_display
> > structure that the caller doesn't know about).
> > 
> > Obviously with the current code in the caller, that doesn't make
> > much
> > difference, but I thought it was cleaner as I did.
> > 
> > But I'm okay to change it and initialize them at the caller, so
> > just let me know
> > if you want that.
> 
> I don’t think it needs to be changed then and the explanation looks
> reasonable.
> So this LGTM
> 
> Reviewed-by: Suraj Kandpal 
> 

Thanks, Suraj! Can someone merge this for me, please?

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/display: pre-initialize some values in probe_gmdid_display()

[Coelho, Luciano]

On Tue, 2023-06-20 at 10:30 +, Kandpal, Suraj wrote:
> > When intel_display_device_probe() (and, subsequently,
> > probe_gmdid_display()) returns, the caller expects ver, rel and
> > step to be
> > initialized.  Since there's no way to check that there was a
> > failure and
> > no_display was returned without some further refactoring, pre-
> > initiliaze all
> > these values to zero to keep it simple and safe.
> > 
> > Signed-off-by: Luca Coelho 
> 
> Looks okay to me just a small suggestion/question below.
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display_device.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c
> > b/drivers/gpu/drm/i915/display/intel_display_device.c
> > index 464df1764a86..fb6354e9e704 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > @@ -731,6 +731,15 @@ probe_gmdid_display(struct drm_i915_private
> > *i915, u16 *ver, u16 *rel, u16 *step
> >     u32 val;
> >     int i;
> > 
> > +   /* The caller expects to ver, rel and step to be
> > initialized
> > +* here, and there's no good way to check when there was a
> > +* failure and no_display was returned.  So initialize all
> > these
> > +* values here zero, to be sure.
> > +*/
> > +   *ver = 0;
> > +   *rel = 0;
> > +   *step = 0;
> > +
> 
> From what I can see this is only called from
> intel_display_device_probe() which is in turn
> called from intel_device_info_driver_create() where the above
> variables are defined maybe 
> we initialize these values there itself.

Thanks for the review!

I thought about initializing the variables on the caller side, but
reckoned that it would be more intuitive to initialize them in the
probe_gmdid_display() function instead, because the caller expects
those values to be set in successful cases and there's no way for it to
know whether there was a failure or not (because we return a pointer to
local no_display structure that the caller doesn't know about).

Obviously with the current code in the caller, that doesn't make much
difference, but I thought it was cleaner as I did.

But I'm okay to change it and initialize them at the caller, so just
let me know if you want that.

--
Cheers,
Luca.

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: implement internal workqueues (rev3)

[Coelho, Luciano]

On Tue, 2023-06-06 at 14:30 +, Coelho, Luciano wrote:
> On Tue, 2023-06-06 at 14:33 +0100, Tvrtko Ursulin wrote:
> > On 06/06/2023 12:06, Coelho, Luciano wrote:
> > > On Tue, 2023-06-06 at 11:06 +0100, Tvrtko Ursulin wrote:
> > > > On 05/06/2023 16:06, Jani Nikula wrote:
> > > > > On Wed, 31 May 2023, Patchwork  
> > > > > wrote:
> > > > > >  Possible regressions 
> > > > > > 
> > > > > > * igt@gem_close_race@basic-process:
> > > > > >   - fi-blb-e6850:   [PASS][1] -> [ABORT][2]
> > > > > >  [1]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-blb-e6850/igt@gem_close_r...@basic-process.html
> > > > > >  [2]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-blb-e6850/igt@gem_close_r...@basic-process.html
> > > > > >   - fi-hsw-4770:[PASS][3] -> [ABORT][4]
> > > > > >  [3]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-hsw-4770/igt@gem_close_r...@basic-process.html
> > > > > >  [4]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-hsw-4770/igt@gem_close_r...@basic-process.html
> > > > > >   - fi-elk-e7500:   [PASS][5] -> [ABORT][6]
> > > > > >  [5]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-elk-e7500/igt@gem_close_r...@basic-process.html
> > > > > >  [6]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-elk-e7500/igt@gem_close_r...@basic-process.html
> > > > > > 
> > > > > > * igt@i915_selftest@live@evict:
> > > > > >   - bat-adlp-9: [PASS][7] -> [ABORT][8]
> > > > > >  [7]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-adlp-9/igt@i915_selftest@l...@evict.html
> > > > > >  [8]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-adlp-9/igt@i915_selftest@l...@evict.html
> > > > > >   - bat-rpls-2: [PASS][9] -> [ABORT][10]
> > > > > >  [9]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-rpls-2/igt@i915_selftest@l...@evict.html
> > > > > >  [10]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-rpls-2/igt@i915_selftest@l...@evict.html
> > > > > >   - bat-adlm-1: [PASS][11] -> [ABORT][12]
> > > > > >  [11]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-adlm-1/igt@i915_selftest@l...@evict.html
> > > > > >  [12]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-adlm-1/igt@i915_selftest@l...@evict.html
> > > > > >   - bat-rpls-1: [PASS][13] -> [ABORT][14]
> > > > > >  [13]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-rpls-1/igt@i915_selftest@l...@evict.html
> > > > > >  [14]: 
> > > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-rpls-1/igt@i915_selftest@l...@evict.html
> > > > > 
> > > > > This still fails consistently, I have no clue why, and the above 
> > > > > aren't
> > > > > even remotely related to display.
> > > > > 
> > > > > What now? Tvrtko?
> > > > 
> > > > Hmm..
> > > > 
> > > > <4> [46.782321] Chain exists of:
> > > > (wq_completion)i915 --> (work_completion)(>mm.free_work) --> 
> > > > >mutex
> > > > <4> [46.782329]  Possible unsafe locking scenario:
> > > > <4> [46.782332]CPU0CPU1
> > > > <4> [46.782334]
> > > > <4> [46.782337]   lock(>mutex);
> > > > <4> [46.782340]
> > > > lock((work_completion)(>mm.free_work));
> > > > <4> [46.782344]lock(>mutex);
> > > > <4> [46.782348]   lock((wq_completion)i915);
> > > > 
> > > > 
> > > > "(wq_completion)i915"
> > > > 
> > > > So it's not about the n

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: implement internal workqueues (rev3)

[Coelho, Luciano]

On Tue, 2023-06-06 at 14:33 +0100, Tvrtko Ursulin wrote:
> On 06/06/2023 12:06, Coelho, Luciano wrote:
> > On Tue, 2023-06-06 at 11:06 +0100, Tvrtko Ursulin wrote:
> > > On 05/06/2023 16:06, Jani Nikula wrote:
> > > > On Wed, 31 May 2023, Patchwork  wrote:
> > > > >  Possible regressions 
> > > > > 
> > > > > * igt@gem_close_race@basic-process:
> > > > >   - fi-blb-e6850:   [PASS][1] -> [ABORT][2]
> > > > >  [1]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-blb-e6850/igt@gem_close_r...@basic-process.html
> > > > >  [2]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-blb-e6850/igt@gem_close_r...@basic-process.html
> > > > >   - fi-hsw-4770:[PASS][3] -> [ABORT][4]
> > > > >  [3]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-hsw-4770/igt@gem_close_r...@basic-process.html
> > > > >  [4]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-hsw-4770/igt@gem_close_r...@basic-process.html
> > > > >   - fi-elk-e7500:   [PASS][5] -> [ABORT][6]
> > > > >  [5]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-elk-e7500/igt@gem_close_r...@basic-process.html
> > > > >  [6]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-elk-e7500/igt@gem_close_r...@basic-process.html
> > > > > 
> > > > > * igt@i915_selftest@live@evict:
> > > > >   - bat-adlp-9: [PASS][7] -> [ABORT][8]
> > > > >  [7]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-adlp-9/igt@i915_selftest@l...@evict.html
> > > > >  [8]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-adlp-9/igt@i915_selftest@l...@evict.html
> > > > >   - bat-rpls-2: [PASS][9] -> [ABORT][10]
> > > > >  [9]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-rpls-2/igt@i915_selftest@l...@evict.html
> > > > >  [10]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-rpls-2/igt@i915_selftest@l...@evict.html
> > > > >   - bat-adlm-1: [PASS][11] -> [ABORT][12]
> > > > >  [11]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-adlm-1/igt@i915_selftest@l...@evict.html
> > > > >  [12]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-adlm-1/igt@i915_selftest@l...@evict.html
> > > > >   - bat-rpls-1: [PASS][13] -> [ABORT][14]
> > > > >  [13]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-rpls-1/igt@i915_selftest@l...@evict.html
> > > > >  [14]: 
> > > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-rpls-1/igt@i915_selftest@l...@evict.html
> > > > 
> > > > This still fails consistently, I have no clue why, and the above aren't
> > > > even remotely related to display.
> > > > 
> > > > What now? Tvrtko?
> > > 
> > > Hmm..
> > > 
> > > <4> [46.782321] Chain exists of:
> > > (wq_completion)i915 --> (work_completion)(>mm.free_work) --> 
> > > >mutex
> > > <4> [46.782329]  Possible unsafe locking scenario:
> > > <4> [46.782332]CPU0CPU1
> > > <4> [46.782334]
> > > <4> [46.782337]   lock(>mutex);
> > > <4> [46.782340]
> > > lock((work_completion)(>mm.free_work));
> > > <4> [46.782344]lock(>mutex);
> > > <4> [46.782348]   lock((wq_completion)i915);
> > > 
> > > 
> > > "(wq_completion)i915"
> > > 
> > > So it's not about the new wq even. Perhaps it is this hunk:
> > > 
> > > --- a/drivers/gpu/drm/i915/intel_wakeref.c
> > > +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> > > @@ -75,7 +75,7 @@ void __intel_wakeref_put_last(struct intel_wakeref *wf, 
> > > unsigned long flags)
> > >
> > >   /* Assume we are not in process context and so cannot sleep. */
> > >   if (flags & INTEL_WAKER

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: implement internal workqueues (rev3)

[Coelho, Luciano]

On Tue, 2023-06-06 at 11:06 +0100, Tvrtko Ursulin wrote:
> On 05/06/2023 16:06, Jani Nikula wrote:
> > On Wed, 31 May 2023, Patchwork  wrote:
> > >  Possible regressions 
> > > 
> > >* igt@gem_close_race@basic-process:
> > >  - fi-blb-e6850:   [PASS][1] -> [ABORT][2]
> > > [1]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-blb-e6850/igt@gem_close_r...@basic-process.html
> > > [2]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-blb-e6850/igt@gem_close_r...@basic-process.html
> > >  - fi-hsw-4770:[PASS][3] -> [ABORT][4]
> > > [3]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-hsw-4770/igt@gem_close_r...@basic-process.html
> > > [4]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-hsw-4770/igt@gem_close_r...@basic-process.html
> > >  - fi-elk-e7500:   [PASS][5] -> [ABORT][6]
> > > [5]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/fi-elk-e7500/igt@gem_close_r...@basic-process.html
> > > [6]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/fi-elk-e7500/igt@gem_close_r...@basic-process.html
> > > 
> > >* igt@i915_selftest@live@evict:
> > >  - bat-adlp-9: [PASS][7] -> [ABORT][8]
> > > [7]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-adlp-9/igt@i915_selftest@l...@evict.html
> > > [8]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-adlp-9/igt@i915_selftest@l...@evict.html
> > >  - bat-rpls-2: [PASS][9] -> [ABORT][10]
> > > [9]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-rpls-2/igt@i915_selftest@l...@evict.html
> > > [10]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-rpls-2/igt@i915_selftest@l...@evict.html
> > >  - bat-adlm-1: [PASS][11] -> [ABORT][12]
> > > [11]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-adlm-1/igt@i915_selftest@l...@evict.html
> > > [12]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-adlm-1/igt@i915_selftest@l...@evict.html
> > >  - bat-rpls-1: [PASS][13] -> [ABORT][14]
> > > [13]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13203/bat-rpls-1/igt@i915_selftest@l...@evict.html
> > > [14]: 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117618v3/bat-rpls-1/igt@i915_selftest@l...@evict.html
> > 
> > This still fails consistently, I have no clue why, and the above aren't
> > even remotely related to display.
> > 
> > What now? Tvrtko?
> 
> Hmm..
> 
> <4> [46.782321] Chain exists of:
>(wq_completion)i915 --> (work_completion)(>mm.free_work) --> 
> >mutex
> <4> [46.782329]  Possible unsafe locking scenario:
> <4> [46.782332]CPU0CPU1
> <4> [46.782334]
> <4> [46.782337]   lock(>mutex);
> <4> [46.782340]
> lock((work_completion)(>mm.free_work));
> <4> [46.782344]lock(>mutex);
> <4> [46.782348]   lock((wq_completion)i915);
> 
> 
> "(wq_completion)i915"
> 
> So it's not about the new wq even. Perhaps it is this hunk:
> 
> --- a/drivers/gpu/drm/i915/intel_wakeref.c
> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> @@ -75,7 +75,7 @@ void __intel_wakeref_put_last(struct intel_wakeref *wf, 
> unsigned long flags)
>   
>   /* Assume we are not in process context and so cannot sleep. */
>   if (flags & INTEL_WAKEREF_PUT_ASYNC || !mutex_trylock(>mutex)) {
> - mod_delayed_work(system_wq, >work,
> + mod_delayed_work(wf->i915->wq, >work,
> 
> Transformation from this patch otherwise is system_wq with the new unordered 
> wq, so I'd try that first.

Indeed this seems to be exactly the block that is causing the issue.  I
was sort of bisecting through all these changes and reverting this one
prevents the lockdep splat from happening...

So there's something that needs to be synced with the system_wq here,
but what? I need to dig into it.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v2 2/3] drm/i915: add a dedicated workqueue inside drm_i915_private

[Coelho, Luciano]

On Fri, 2023-05-26 at 14:30 +0300, Jani Nikula wrote:
> On Wed, 24 May 2023, Tvrtko Ursulin  wrote:
> > On 24/05/2023 10:05, Luca Coelho wrote:
> > > In order to avoid flush_scheduled_work() usage, add a dedicated
> > > workqueue in the drm_i915_private structure.  In this way, we don't
> > > need to use the system queue anymore.
> > > 
> > > This change is mostly mechanical and based on Tetsuo's original
> > > patch[1].
> > > 
> > > Link: https://patchwork.freedesktop.org/series/114608/ [1]
> > > Cc: Tetsuo Handa 
> > > Cc: Tvrtko Ursulin 
> > > Cc: Jani Nikula 
> > > Cc: Ville Syrjälä 
> > > Signed-off-by: Luca Coelho 
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_display.c  |  5 ++--
> > >   .../drm/i915/display/intel_display_driver.c   |  2 +-
> > >   drivers/gpu/drm/i915/display/intel_dmc.c  |  2 +-
> > >   drivers/gpu/drm/i915/display/intel_dp.c   |  2 +-
> > >   .../drm/i915/display/intel_dp_link_training.c |  3 ++-
> > >   drivers/gpu/drm/i915/display/intel_drrs.c |  4 +++-
> > >   drivers/gpu/drm/i915/display/intel_fbc.c  |  2 +-
> > >   drivers/gpu/drm/i915/display/intel_fbdev.c|  3 ++-
> > >   drivers/gpu/drm/i915/display/intel_hdcp.c | 23 +++
> > >   drivers/gpu/drm/i915/display/intel_hotplug.c  | 18 ++-
> > >   drivers/gpu/drm/i915/display/intel_opregion.c |  3 ++-
> > >   drivers/gpu/drm/i915/display/intel_pps.c  |  4 +++-
> > >   drivers/gpu/drm/i915/display/intel_psr.c  |  8 ---
> > >   .../drm/i915/gt/intel_execlists_submission.c  |  5 ++--
> > >   .../gpu/drm/i915/gt/intel_gt_buffer_pool.c| 10 
> > >   drivers/gpu/drm/i915/gt/intel_gt_irq.c|  2 +-
> > >   drivers/gpu/drm/i915/gt/intel_gt_requests.c   | 10 
> > >   drivers/gpu/drm/i915/gt/intel_reset.c |  2 +-
> > >   drivers/gpu/drm/i915/gt/intel_rps.c   | 20 
> > >   drivers/gpu/drm/i915/gt/selftest_engine_cs.c  |  2 +-
> > >   drivers/gpu/drm/i915/i915_driver.c| 11 +
> > >   drivers/gpu/drm/i915/i915_drv.h   |  9 +++-
> > >   drivers/gpu/drm/i915/i915_request.c   |  2 +-
> > >   drivers/gpu/drm/i915/intel_wakeref.c  |  2 +-
> > >   24 files changed, 99 insertions(+), 55 deletions(-)
> > 
> > I'll take a look at the gt/ parts only since display experts need to 
> > okay the point Daniel raise anyway.
> 
> There's a bunch of lockdep failures in BAT. Are they possible related to
> switching to unordered wq?

I have not checked those results yet, because I'll send a new version
soon anyway.  I tested this before, but with the dedicated wakeref
workqueues, and it all passed cleanly.  So it _may_ be related to that
change.  I'll check it.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v2 2/3] drm/i915: add a dedicated workqueue inside drm_i915_private

[Coelho, Luciano]

On Wed, 2023-05-24 at 12:00 +0100, Tvrtko Ursulin wrote:
> On 24/05/2023 10:05, Luca Coelho wrote:
> > In order to avoid flush_scheduled_work() usage, add a dedicated
> > workqueue in the drm_i915_private structure.  In this way, we don't
> > need to use the system queue anymore.
> > 
> > This change is mostly mechanical and based on Tetsuo's original
> > patch[1].
> > 
> > Link: https://patchwork.freedesktop.org/series/114608/ [1]
> > Cc: Tetsuo Handa 
> > Cc: Tvrtko Ursulin 
> > Cc: Jani Nikula 
> > Cc: Ville Syrjälä 
> > Signed-off-by: Luca Coelho 
> > ---
> >   drivers/gpu/drm/i915/display/intel_display.c  |  5 ++--
> >   .../drm/i915/display/intel_display_driver.c   |  2 +-
> >   drivers/gpu/drm/i915/display/intel_dmc.c  |  2 +-
> >   drivers/gpu/drm/i915/display/intel_dp.c   |  2 +-
> >   .../drm/i915/display/intel_dp_link_training.c |  3 ++-
> >   drivers/gpu/drm/i915/display/intel_drrs.c |  4 +++-
> >   drivers/gpu/drm/i915/display/intel_fbc.c  |  2 +-
> >   drivers/gpu/drm/i915/display/intel_fbdev.c|  3 ++-
> >   drivers/gpu/drm/i915/display/intel_hdcp.c | 23 +++
> >   drivers/gpu/drm/i915/display/intel_hotplug.c  | 18 ++-
> >   drivers/gpu/drm/i915/display/intel_opregion.c |  3 ++-
> >   drivers/gpu/drm/i915/display/intel_pps.c  |  4 +++-
> >   drivers/gpu/drm/i915/display/intel_psr.c  |  8 ---
> >   .../drm/i915/gt/intel_execlists_submission.c  |  5 ++--
> >   .../gpu/drm/i915/gt/intel_gt_buffer_pool.c| 10 
> >   drivers/gpu/drm/i915/gt/intel_gt_irq.c|  2 +-
> >   drivers/gpu/drm/i915/gt/intel_gt_requests.c   | 10 
> >   drivers/gpu/drm/i915/gt/intel_reset.c |  2 +-
> >   drivers/gpu/drm/i915/gt/intel_rps.c   | 20 
> >   drivers/gpu/drm/i915/gt/selftest_engine_cs.c  |  2 +-
> >   drivers/gpu/drm/i915/i915_driver.c| 11 +
> >   drivers/gpu/drm/i915/i915_drv.h   |  9 +++-
> >   drivers/gpu/drm/i915/i915_request.c   |  2 +-
> >   drivers/gpu/drm/i915/intel_wakeref.c  |  2 +-
> >   24 files changed, 99 insertions(+), 55 deletions(-)
> 
> I'll take a look at the gt/ parts only since display experts need to 
> okay the point Daniel raise anyway.

Thanks!



> 8<
> > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
> > b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > index 750326434677..2ebd937f3b4c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > @@ -2327,6 +2327,7 @@ static u32 active_ccid(struct intel_engine_cs *engine)
> >   
> >   static void execlists_capture(struct intel_engine_cs *engine)
> >   {
> > +   struct drm_i915_private *i915 = engine->i915;
> > struct execlists_capture *cap;
> >   
> > if (!IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR))
> > @@ -2375,7 +2376,7 @@ static void execlists_capture(struct intel_engine_cs 
> > *engine)
> > goto err_rq;
> >   
> > INIT_WORK(>work, execlists_capture_work);
> > -   schedule_work(>work);
> > +   queue_work(i915->unordered_wq, >work);
> > return;
> >   
> >   err_rq:
> > @@ -3680,7 +3681,7 @@ static void virtual_context_destroy(struct kref *kref)
> >  * lock, we can delegate the free of the engine to an RCU worker.
> >  */
> > INIT_RCU_WORK(>rcu, rcu_virtual_context_destroy);
> > -   queue_rcu_work(system_wq, >rcu);
> > +   queue_rcu_work(ve->context.engine->i915->unordered_wq, >rcu);
> >   }
> >   
> >   static void virtual_engine_initial_hint(struct virtual_engine *ve)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c 
> > b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
> > index cadfd85785b1..86b5a9ba323d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
> > @@ -88,10 +88,11 @@ static void pool_free_work(struct work_struct *wrk)
> >   {
> > struct intel_gt_buffer_pool *pool =
> > container_of(wrk, typeof(*pool), work.work);
> > +   struct intel_gt *gt = container_of(pool, struct intel_gt, buffer_pool);
> 
> Okay. Or alternatively, pool = >buffer_pool, might read simpler...

Sorry, I don't follow.  wrk is inside intel_gt_buffer_pool, so we can
derive pool from work.  And then we know that pool is inside intel_gt,
so we derive gt from that.  How would I get gt first and derive pool
from that?


> >   
> > if (pool_free_older_than(pool, HZ))
> > -   schedule_delayed_work(>work,
> > - round_jiffies_up_relative(HZ));
> > +   queue_delayed_work(gt->i915->unordered_wq, >work,
> > +  round_jiffies_up_relative(HZ));
> >   }
> >   
> >   static void pool_retire(struct i915_active *ref)
> > @@ -99,6 +100,7 @@ static void pool_retire(struct i915_active *ref)
> > struct intel_gt_buffer_pool_node *node =
> > container_of(ref, typeof(*node), 

Re: [Intel-gfx] [PATCH 2/3] drm/i915/gt: create workqueue dedicated to wake references

[Coelho, Luciano]

On Fri, 2023-05-12 at 13:16 +0100, Tvrtko Ursulin wrote:
> On 12/05/2023 10:54, Coelho, Luciano wrote:
> > On Fri, 2023-05-12 at 10:32 +0100, Tvrtko Ursulin wrote:
> > > On 12/05/2023 10:10, Coelho, Luciano wrote:
> > > > On Fri, 2023-05-12 at 10:04 +0100, Tvrtko Ursulin wrote:
> > > > > On 11/05/2023 09:20, Luca Coelho wrote:
> > > > > > Add a work queue in the intel_wakeref structure to be used 
> > > > > > exclusively
> > > > > > by the wake reference mechanism.  This is needed in order to avoid
> > > > > > using the system workqueue and relying on flush_scheduled_work().
> > > > > > 
> > > > > > Cc: Tetsuo Handa 
> > > > > > Cc: Tvrtko Ursulin 
> > > > > > Cc: Jani Nikula 
> > > > > > Cc: Ville Syrjälä 
> > > > > > Signed-off-by: Luca Coelho 
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/gt/intel_engine_cs.c |  7 ++-
> > > > > > drivers/gpu/drm/i915/gt/intel_engine_pm.c | 15 --
> > > > > > drivers/gpu/drm/i915/gt/intel_engine_pm.h |  3 ++-
> > > > > > drivers/gpu/drm/i915/gt/mock_engine.c |  8 +++-
> > > > > > drivers/gpu/drm/i915/intel_wakeref.c  | 21 
> > > > > > ++-
> > > > > > drivers/gpu/drm/i915/intel_wakeref.h  | 25 
> > > > > > +++
> > > > > > 6 files changed, 60 insertions(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
> > > > > > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > > > > index 0aff5bb13c53..6505bfa70cd0 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > > > > @@ -1290,7 +1290,11 @@ static int engine_setup_common(struct 
> > > > > > intel_engine_cs *engine)
> > > > > > goto err_cmd_parser;
> > > > > > 
> > > > > > intel_engine_init_execlists(engine);
> > > > > > -   intel_engine_init__pm(engine);
> > > > > > +
> > > > > > +   err = intel_engine_init__pm(engine);
> > > > > > +   if (err)
> > > > > > +   goto err_cmd_parser;
> > > > > > +
> > > > > > intel_engine_init_retire(engine);
> > > > > > 
> > > > > > /* Use the whole device by default */
> > > > > > @@ -1525,6 +1529,7 @@ void intel_engine_cleanup_common(struct 
> > > > > > intel_engine_cs *engine)
> > > > > > {
> > > > > > 
> > > > > > GEM_BUG_ON(!list_empty(>sched_engine->requests));
> > > > > > 
> > > > > > +   intel_engine_destroy__pm(engine);
> > > > > > i915_sched_engine_put(engine->sched_engine);
> > > > > > intel_breadcrumbs_put(engine->breadcrumbs);
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c 
> > > > > > b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > > > index ee531a5c142c..859b62cf660f 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > > > @@ -294,14 +294,25 @@ static const struct intel_wakeref_ops wf_ops 
> > > > > > = {
> > > > > > .put = __engine_park,
> > > > > > };
> > > > > > 
> > > > > > -void intel_engine_init__pm(struct intel_engine_cs *engine)
> > > > > > +int intel_engine_init__pm(struct intel_engine_cs *engine)
> > > > > > {
> > > > > > struct intel_runtime_pm *rpm = engine->uncore->rpm;
> > > > > > +   int err;
> > > > > > +
> > > > > > +   err = intel_wakeref_init(>wakeref, rpm, _ops);
> > > > > > +   if (err)
> > > > > > +   return err;
> > > > > > 
> > > > > > -   intel_wakeref_init(>wakeref, rpm, _ops);
> > > > > > intel_engine_

Re: [Intel-gfx] [PATCH 2/3] drm/i915/gt: create workqueue dedicated to wake references

[Coelho, Luciano]

On Fri, 2023-05-12 at 10:32 +0100, Tvrtko Ursulin wrote:
> On 12/05/2023 10:10, Coelho, Luciano wrote:
> > On Fri, 2023-05-12 at 10:04 +0100, Tvrtko Ursulin wrote:
> > > On 11/05/2023 09:20, Luca Coelho wrote:
> > > > Add a work queue in the intel_wakeref structure to be used exclusively
> > > > by the wake reference mechanism.  This is needed in order to avoid
> > > > using the system workqueue and relying on flush_scheduled_work().
> > > > 
> > > > Cc: Tetsuo Handa 
> > > > Cc: Tvrtko Ursulin 
> > > > Cc: Jani Nikula 
> > > > Cc: Ville Syrjälä 
> > > > Signed-off-by: Luca Coelho 
> > > > ---
> > > >drivers/gpu/drm/i915/gt/intel_engine_cs.c |  7 ++-
> > > >drivers/gpu/drm/i915/gt/intel_engine_pm.c | 15 --
> > > >drivers/gpu/drm/i915/gt/intel_engine_pm.h |  3 ++-
> > > >drivers/gpu/drm/i915/gt/mock_engine.c |  8 +++-
> > > >drivers/gpu/drm/i915/intel_wakeref.c  | 21 ++-
> > > >drivers/gpu/drm/i915/intel_wakeref.h  | 25 
> > > > +++
> > > >6 files changed, 60 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
> > > > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > > index 0aff5bb13c53..6505bfa70cd0 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > > @@ -1290,7 +1290,11 @@ static int engine_setup_common(struct 
> > > > intel_engine_cs *engine)
> > > > goto err_cmd_parser;
> > > >
> > > > intel_engine_init_execlists(engine);
> > > > -   intel_engine_init__pm(engine);
> > > > +
> > > > +   err = intel_engine_init__pm(engine);
> > > > +   if (err)
> > > > +   goto err_cmd_parser;
> > > > +
> > > > intel_engine_init_retire(engine);
> > > >
> > > > /* Use the whole device by default */
> > > > @@ -1525,6 +1529,7 @@ void intel_engine_cleanup_common(struct 
> > > > intel_engine_cs *engine)
> > > >{
> > > > GEM_BUG_ON(!list_empty(>sched_engine->requests));
> > > >
> > > > +   intel_engine_destroy__pm(engine);
> > > > i915_sched_engine_put(engine->sched_engine);
> > > > intel_breadcrumbs_put(engine->breadcrumbs);
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c 
> > > > b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > index ee531a5c142c..859b62cf660f 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > @@ -294,14 +294,25 @@ static const struct intel_wakeref_ops wf_ops = {
> > > > .put = __engine_park,
> > > >};
> > > >
> > > > -void intel_engine_init__pm(struct intel_engine_cs *engine)
> > > > +int intel_engine_init__pm(struct intel_engine_cs *engine)
> > > >{
> > > > struct intel_runtime_pm *rpm = engine->uncore->rpm;
> > > > +   int err;
> > > > +
> > > > +   err = intel_wakeref_init(>wakeref, rpm, _ops);
> > > > +   if (err)
> > > > +   return err;
> > > >
> > > > -   intel_wakeref_init(>wakeref, rpm, _ops);
> > > > intel_engine_init_heartbeat(engine);
> > > >
> > > > intel_gsc_idle_msg_enable(engine);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +void intel_engine_destroy__pm(struct intel_engine_cs *engine)
> > > > +{
> > > > +   intel_wakeref_destroy(>wakeref);
> > > >}
> > > >
> > > >/**
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h 
> > > > b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> > > > index d68675925b79..e8568f7d10c6 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> > > > @@ -104,7 +104,8 @@ intel_engine_create_kernel_request(struct 
> > > > intel_engine_cs *engine)
> > > >   

Re: [Intel-gfx] [PATCH 2/3] drm/i915/gt: create workqueue dedicated to wake references

[Coelho, Luciano]

On Fri, 2023-05-12 at 10:04 +0100, Tvrtko Ursulin wrote:
> On 11/05/2023 09:20, Luca Coelho wrote:
> > Add a work queue in the intel_wakeref structure to be used exclusively
> > by the wake reference mechanism.  This is needed in order to avoid
> > using the system workqueue and relying on flush_scheduled_work().
> > 
> > Cc: Tetsuo Handa 
> > Cc: Tvrtko Ursulin 
> > Cc: Jani Nikula 
> > Cc: Ville Syrjälä 
> > Signed-off-by: Luca Coelho 
> > ---
> >   drivers/gpu/drm/i915/gt/intel_engine_cs.c |  7 ++-
> >   drivers/gpu/drm/i915/gt/intel_engine_pm.c | 15 --
> >   drivers/gpu/drm/i915/gt/intel_engine_pm.h |  3 ++-
> >   drivers/gpu/drm/i915/gt/mock_engine.c |  8 +++-
> >   drivers/gpu/drm/i915/intel_wakeref.c  | 21 ++-
> >   drivers/gpu/drm/i915/intel_wakeref.h  | 25 +++
> >   6 files changed, 60 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
> > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index 0aff5bb13c53..6505bfa70cd0 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -1290,7 +1290,11 @@ static int engine_setup_common(struct 
> > intel_engine_cs *engine)
> > goto err_cmd_parser;
> >   
> > intel_engine_init_execlists(engine);
> > -   intel_engine_init__pm(engine);
> > +
> > +   err = intel_engine_init__pm(engine);
> > +   if (err)
> > +   goto err_cmd_parser;
> > +
> > intel_engine_init_retire(engine);
> >   
> > /* Use the whole device by default */
> > @@ -1525,6 +1529,7 @@ void intel_engine_cleanup_common(struct 
> > intel_engine_cs *engine)
> >   {
> > GEM_BUG_ON(!list_empty(>sched_engine->requests));
> >   
> > +   intel_engine_destroy__pm(engine);
> > i915_sched_engine_put(engine->sched_engine);
> > intel_breadcrumbs_put(engine->breadcrumbs);
> >   
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c 
> > b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > index ee531a5c142c..859b62cf660f 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > @@ -294,14 +294,25 @@ static const struct intel_wakeref_ops wf_ops = {
> > .put = __engine_park,
> >   };
> >   
> > -void intel_engine_init__pm(struct intel_engine_cs *engine)
> > +int intel_engine_init__pm(struct intel_engine_cs *engine)
> >   {
> > struct intel_runtime_pm *rpm = engine->uncore->rpm;
> > +   int err;
> > +
> > +   err = intel_wakeref_init(>wakeref, rpm, _ops);
> > +   if (err)
> > +   return err;
> >   
> > -   intel_wakeref_init(>wakeref, rpm, _ops);
> > intel_engine_init_heartbeat(engine);
> >   
> > intel_gsc_idle_msg_enable(engine);
> > +
> > +   return 0;
> > +}
> > +
> > +void intel_engine_destroy__pm(struct intel_engine_cs *engine)
> > +{
> > +   intel_wakeref_destroy(>wakeref);
> >   }
> >   
> >   /**
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h 
> > b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> > index d68675925b79..e8568f7d10c6 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> > @@ -104,7 +104,8 @@ intel_engine_create_kernel_request(struct 
> > intel_engine_cs *engine)
> > return rq;
> >   }
> >   
> > -void intel_engine_init__pm(struct intel_engine_cs *engine);
> > +int intel_engine_init__pm(struct intel_engine_cs *engine);
> > +void intel_engine_destroy__pm(struct intel_engine_cs *engine);
> >   
> >   void intel_engine_reset_pinned_contexts(struct intel_engine_cs *engine);
> >   
> > diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c 
> > b/drivers/gpu/drm/i915/gt/mock_engine.c
> > index c0637bf799a3..0a3c702c21e2 100644
> > --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> > +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> > @@ -336,6 +336,7 @@ static void mock_engine_release(struct intel_engine_cs 
> > *engine)
> > intel_context_put(engine->kernel_context);
> >   
> > intel_engine_fini_retire(engine);
> > +   intel_engine_destroy__pm(engine);
> >   }
> >   
> >   struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
> > @@ -393,6 +394,7 @@ struct intel_engine_cs *mock_engine(struct 
> > drm_i915_private *i915,
> >   int mock_engine_init(struct intel_engine_cs *engine)
> >   {
> > struct intel_context *ce;
> > +   int err;
> >   
> > INIT_LIST_HEAD(>pinned_contexts_list);
> >   
> > @@ -402,7 +404,11 @@ int mock_engine_init(struct intel_engine_cs *engine)
> > engine->sched_engine->private_data = engine;
> >   
> > intel_engine_init_execlists(engine);
> > -   intel_engine_init__pm(engine);
> > +
> > +   err = intel_engine_init__pm(engine);
> > +   if (err)
> > +   return err;
> > +
> > intel_engine_init_retire(engine);
> >   
> > engine->breadcrumbs = intel_breadcrumbs_create(NULL);
> > diff --git a/drivers/gpu/drm/i915/intel_wakeref.c 
> > b/drivers/gpu/drm/i915/intel_wakeref.c
> > index 

Re: [Intel-gfx] [RFC] drm/i915: make dev_priv usage explitic in some macros

[Coelho, Luciano]

On Wed, 2023-02-01 at 10:07 -0800, Lucas De Marchi wrote:
> On Wed, Feb 01, 2023 at 09:20:42AM -0800, Coelho, Luciano wrote:
> > On Wed, 2023-02-01 at 19:09 +0200, Jani Nikula wrote:
> > > On Wed, 01 Feb 2023, Luca Coelho  wrote:
> > > > There are a few macros (e.g. DPLL()) that implicitly use dev_priv, by
> > > > using other macros that implcitily use dev_priv.
> > > > 
> > > > In an effort to align all definitions of struct drm_i915_private to be
> > > > declared as i915 instead of arbitrarily using either i915 or dev_priv,
> > > > we need to make these macros explicitly use dev_priv, so that we can
> > > > change them later to be defined as i915.
> > > 
> > > Lucas posted a slightly related patch [1], and I think based on the
> > > discussion we should probably add AUX and DPLL registers that are
> > > VLV/CHV specific, and include the MMIO offset directly without dev_priv,
> > > and non-VLV/CHV macros that will have MMIO offset 0. This would reduce
> > > the implicit dev_priv considerably, and avoid the need to pass i915
> > > pointer to those register macros altogether.
> > 
> > Yes, I saw that.
> > 
> > 
> > > > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi_regs.h 
> > > > b/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
> > > > index abbe427e462e..d00e9321064a 100644
> > > > --- a/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
> > > > +++ b/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
> > > > @@ -100,7 +100,7 @@
> > > > 
> > > >  #define _MIPIA_DEVICE_READY(_MIPI_MMIO_BASE(dev_priv) + 
> > > > 0xb000)
> > > >  #define _MIPIC_DEVICE_READY(_MIPI_MMIO_BASE(dev_priv) + 
> > > > 0xb800)
> > > > -#define MIPI_DEVICE_READY(port)_MMIO_MIPI(port, 
> > > > _MIPIA_DEVICE_READY, _MIPIC_DEVICE_READY)
> > > > +#define MIPI_DEVICE_READY(dev_priv, port) _MMIO_MIPI(port, 
> > > > _MIPIA_DEVICE_READY, _MIPIC_DEVICE_READY)
> > > 
> > > While this kind of passes dev_priv as parameter, the dev_priv in
> > > _MIPIA_DEVICE_READY and _MIPIC_DEVICE_READY is still implicit.
> > 
> > Yes, this was on purpose and my second change is to modify the the
> > calls to the inner macros to pass the parameter as well.
> > 
> > In any case, this already works as is, even if we pass i915 to
> > MIPI_DEVICE_READY() here, because the dev_priv that MIPI*_DEVICE_READY
> > use will be expanded  to whatever we passed as dev_priv to the outer
> > macro.
> > 
> > 
> > > I think these could use a similar treatment as in [1], moving the
> > > _MIPI_MMIO_BASE() part one level up.
> > 
> > Yes, and this can also be done with more rules after my other patches.
> > The problem is if we all start making changes in this area at the same
> > time, then there will be conflict after conflict.
> 
> As I shared previously I think these manual changes need to come
> *before* not after - why would you change the callers to pass dev_priv
> and then ultimately remove? Let the scripted changes only do the
> addition of dev_priv to the callers, after you removed the ones that
> shouldn't have it at all
> 
> This makes the script easier to write and run and can be postponed to
> when the branches are in sync if we are going to cross the display/
> boundary.

Yes, you're totally right.  Sorry if my comment came out negative, that
was not my intent.  We're obviously all working on the same code base
so we work in parallel and conflicts are a part of nature. :)


> Anyway since you are changing this, I'll hold off on more patches
> related to it.

It's maybe a bit easier like that, but in any case the beauty of
semantic patches is that they should "understand" other changes and act
accordingly.  With your changes, my rules wouldn't matchthose cases
anymore, but would still match other ones.

Regarding adding arguments just to remove them later, I think it's okay
if we don't want to do everything in a gigantic patch.  One patch is
lying the foundation for the next that will actually make things
cleaner.  It's a bit easier to review, IMHO.  We probably can, in the
end, before actually merging the changes, squash everything together to
avoid adding-just-to-remove.  Let's see how it goes.

Thanks for the comments!

--
Cheers,
Luca.

Re: [Intel-gfx] [RFC] drm/i915: make dev_priv usage explitic in some macros

[Coelho, Luciano]

On Wed, 2023-02-01 at 19:09 +0200, Jani Nikula wrote:
> On Wed, 01 Feb 2023, Luca Coelho  wrote:
> > There are a few macros (e.g. DPLL()) that implicitly use dev_priv, by
> > using other macros that implcitily use dev_priv.
> > 
> > In an effort to align all definitions of struct drm_i915_private to be
> > declared as i915 instead of arbitrarily using either i915 or dev_priv,
> > we need to make these macros explicitly use dev_priv, so that we can
> > change them later to be defined as i915.
> 
> Lucas posted a slightly related patch [1], and I think based on the
> discussion we should probably add AUX and DPLL registers that are
> VLV/CHV specific, and include the MMIO offset directly without dev_priv,
> and non-VLV/CHV macros that will have MMIO offset 0. This would reduce
> the implicit dev_priv considerably, and avoid the need to pass i915
> pointer to those register macros altogether.

Yes, I saw that.


> > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi_regs.h 
> > b/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
> > index abbe427e462e..d00e9321064a 100644
> > --- a/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
> > +++ b/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
> > @@ -100,7 +100,7 @@
> >  
> >  #define _MIPIA_DEVICE_READY(_MIPI_MMIO_BASE(dev_priv) + 
> > 0xb000)
> >  #define _MIPIC_DEVICE_READY(_MIPI_MMIO_BASE(dev_priv) + 
> > 0xb800)
> > -#define MIPI_DEVICE_READY(port)_MMIO_MIPI(port, 
> > _MIPIA_DEVICE_READY, _MIPIC_DEVICE_READY)
> > +#define MIPI_DEVICE_READY(dev_priv, port) _MMIO_MIPI(port, 
> > _MIPIA_DEVICE_READY, _MIPIC_DEVICE_READY)
> 
> While this kind of passes dev_priv as parameter, the dev_priv in
> _MIPIA_DEVICE_READY and _MIPIC_DEVICE_READY is still implicit.

Yes, this was on purpose and my second change is to modify the the
calls to the inner macros to pass the parameter as well.

In any case, this already works as is, even if we pass i915 to
MIPI_DEVICE_READY() here, because the dev_priv that MIPI*_DEVICE_READY
use will be expanded  to whatever we passed as dev_priv to the outer
macro.


> I think these could use a similar treatment as in [1], moving the
> _MIPI_MMIO_BASE() part one level up.

Yes, and this can also be done with more rules after my other patches.
The problem is if we all start making changes in this area at the same
time, then there will be conflict after conflict.

--
Cheers,
Luca.

Re: [Intel-gfx] [RFC] drm/i915: make dev_priv usage explitic in some macros

[Coelho, Luciano]

On Wed, 2023-02-01 at 17:10 +0100, Andrzej Hajda wrote:
> On 01.02.2023 14:53, Luca Coelho wrote:
> > There are a few macros (e.g. DPLL()) that implicitly use dev_priv, by
> > using other macros that implcitily use dev_priv.
> > 
> > In an effort to align all definitions of struct drm_i915_private to be
> > declared as i915 instead of arbitrarily using either i915 or dev_priv,
> > we need to make these macros explicitly use dev_priv, so that we can
> > change them later to be defined as i915.
> > 
> > In order to find and change all occurrences, the following semantic
> > patch rules were used:
> 
> Quite challenging task.

It was definitely a very nice exercise! :)


> > @macros_noargs@
> > identifier m;
> > expression e =~ "dev_priv";
> > @@
> > 
> > @nested_macros@
> > identifier macros_noargs.m;
> > identifier nm;
> > identifier list il;
> > @@
> 
> Out of curiosity, what is purpose of above stances? Matching sections 
> are empty.
> 

Oops, it seems that git removed the lines that start with # when I
entered the commit message, since it ignores all lines starting with #.

This is how these two should actually look like:

@macros_noargs@
identifier m;
expression e =~ "dev_priv";
@@
#define m <+...e...+>

@nested_macros@
identifier macros_noargs.m;
identifier nm;
identifier list il;
@@
#define nm(il) <+...m...+>

I basically use the first one to match all "inner" macros that use
dev_priv and the second one I use to match all "outer" macros that use
the ones I found before.


> > @@
> > identifier nested_macros.nm;
> > identifier dev_priv, f;
> > expression list el;
> > @@
> > f(...) {
> > ...
> > struct drm_i915_private *dev_priv = ...;
> > 
> > <+...
> > nm(
> > +   dev_priv,
> > el)
> > ...+>
> > }
> > 
> > @@
> > identifier nested_macros.nm;
> > identifier dev_priv, f;
> > expression list el;
> > @@
> > f(..., struct drm_i915_private *dev_priv, ...) {
> > <+...
> > nm(
> > +   dev_priv,
> > el)
> > ...+>
> > }
> > 
> > @@
> > identifier nested_macros.nm;
> > identifier list il;
> > expression e;
> > @@
> > -#define nm(il) e
> > +#define nm(dev_priv,il) e
> > 
> > Signed-off-by: Luca Coelho 
> > ---
> >   drivers/gpu/drm/i915/display/intel_display.c  |  34 ++-
> >   .../drm/i915/display/intel_display_power.c|   2 +-
> >   .../i915/display/intel_display_power_well.c   |  22 +-
> >   drivers/gpu/drm/i915/display/intel_dp_aux.c   |  40 +--
> >   drivers/gpu/drm/i915/display/intel_dpll.c |  57 ++--
> >   drivers/gpu/drm/i915/display/intel_dvo.c  |   6 +-
> >   drivers/gpu/drm/i915/display/intel_pps.c  |   2 +-
> >   drivers/gpu/drm/i915/display/vlv_dsi.c| 266 +++---
> >   drivers/gpu/drm/i915/display/vlv_dsi_pll.c|   6 +-
> >   drivers/gpu/drm/i915/display/vlv_dsi_regs.h   |  93 +++---
> >   drivers/gpu/drm/i915/gvt/handlers.c   |  22 +-
> >   drivers/gpu/drm/i915/i915_reg.h   |  26 +-
> >   drivers/gpu/drm/i915/intel_gvt_mmio_table.c   |   6 +-
> >   13 files changed, 316 insertions(+), 266 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index e37cca6b18c6..f563d6dd3b4f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -469,11 +469,11 @@ void vlv_wait_port_ready(struct drm_i915_private 
> > *dev_priv,
> > fallthrough;
> > case PORT_B:
> > port_mask = DPLL_PORTB_READY_MASK;
> > -   dpll_reg = DPLL(0);
> > +   dpll_reg = DPLL(dev_priv, 0);
> > break;
> > case PORT_C:
> > port_mask = DPLL_PORTC_READY_MASK;
> > -   dpll_reg = DPLL(0);
> > +   dpll_reg = DPLL(dev_priv, 0);
> > expected_mask <<= 4;
> > break;
> > case PORT_D:
> > @@ -3248,14 +3248,16 @@ static bool i9xx_get_pipe_config(struct intel_crtc 
> > *crtc,
> > if (IS_CHERRYVIEW(dev_priv) && crtc->pipe != PIPE_A)
> > tmp = dev_priv->display.state.chv_dpll_md[crtc->pipe];
> > else
> > -   tmp = intel_de_read(dev_priv, DPLL_MD(crtc->pipe));
> > +   tmp = intel_de_read(dev_priv,
> > +   DPLL_MD(dev_priv, crtc->pipe));
> > pipe_config->pixel_multiplier =
> > ((tmp & DPLL_MD_UDI_MULTIPLIER_MASK)
> >  >> DPLL_MD_UDI_MULTIPLIER_SHIFT) + 1;
> > pipe_config->dpll_hw_state.dpll_md = tmp;
> > } else if (IS_I945G(dev_priv) || IS_I945GM(dev_priv) ||
> >IS_G33(dev_priv) || IS_PINEVIEW(dev_priv)) {
> > -   tmp = intel_de_read(dev_priv, DPLL(crtc->pipe));
> > +   tmp = intel_de_read(dev_priv,
> > +   DPLL(dev_priv, crtc->pipe));
> > pipe_config->pixel_multiplier =
> > ((tmp & SDVO_MULTIPLIER_MASK)
> >  >> 

Re: [Intel-gfx] [PATCH v3] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Coelho, Luciano]

On Mon, 2023-01-30 at 10:06 +0200, Jouni Högander wrote:
> SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> SEL_FETCH_* instances of plane configuration are used when doing
> selective update and normal plane register instances for full updates.
> Currently all SEL_FETCH_* registers are written as a part of noarm
> plane configuration. If noarm and arm plane configuration are not
> happening within same vblank we may end up having plane as a part of
> selective update before it's PLANE_SURF register is written.
> 
> Fix this by splitting plane selective fetch configuration into arm and
> noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> version.
> 
> v3:
>  - add arm suffix into intel_psr2_disable_plane_sel_fetch
> v2:
>  - drop color_plane parameter from arm part
>  - dev_priv -> i915 in arm part
> 
> Cc: Ville Syrjälä 
> Cc: José Roberto de Souza 
> Cc: Mika Kahola 
> Cc: Vinod Govindapillai 
> Cc: Stanislav Lisovskiy 
> Cc: Luca Coelho 
> Signed-off-by: Jouni Högander 
> Reviewed-by: José Roberto de Souza 
> ---

Reviewed-by: Luca Coelho 

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Coelho, Luciano]

On Fri, 2023-01-27 at 10:27 +0200, Jouni Högander wrote:
> SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> SEL_FETCH_* instances of plane configuration are used when doing
> selective update and normal plane register instances for full updates.
> Currently all SEL_FETCH_* registers are written as a part of noarm
> plane configuration. If noarm and arm plane configuration are not
> happening within same vblank we may end up having plane as a part of
> selective update before it's PLANE_SURF register is written.
> 
> Fix this by splitting plane selective fetch configuration into arm and
> noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> version.
> 
> v2:
>  - drop color_plane parameter from arm part
>  - dev_priv -> i915 in arm part
> 
> Cc: Ville Syrjälä 
> Cc: José Roberto de Souza 
> Cc: Mika Kahola 
> Cc: Vinod Govindapillai 
> Cc: Stanislav Lisovskiy 
> Cc: Luca Coelho 
> Signed-off-by: Jouni Högander 
> ---

Looks good to me:

Reviewed-by: Luca Coelho 

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915: update src and dst scaler limits for display ver 12 and 13

[Coelho, Luciano]

On Fri, 2023-01-13 at 16:04 +0530, Nautiyal, Ankit K wrote:
> Hi Luca,

Hi Ankit,


> Patch looks good to me. Please add 'Bspec:50441' in commit message for 
> reference.

Good point, I'll add it and resend.


> Also, you might need to re-submit for test, as last time the other 
> scaler changes were not merged, and CI build had failed.

I'll have to resend it anyway because of the bspec tag, so it will run
again.


> Reviewed-by: Ankit Nautiyal 

Thanks a lot!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v7 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Mon, 2023-01-09 at 09:06 +0200, Lisovskiy, Stanislav wrote:
> On Fri, Dec 23, 2022 at 03:05:08PM +0200, Luca Coelho wrote:
> > In newer hardware versions (i.e. display version >= 14), the second
> > scaler doesn't support vertical scaling.
> > 
> > The current implementation of the scaling limits is simplified and
> > only occurs when the planes are created, so we don't know which scaler
> > is being used.
> > 
> > In order to handle separate scaling limits for horizontal and vertical
> > scaling, and different limits per scaler, split the checks in two
> > phases.  We first do a simple check during plane creation and use the
> > best-case scenario (because we don't know the scaler that may be used
> > at a later point) and then do a more specific check when the scalers
> > are actually being set up.
> > 
> > Signed-off-by: Luca Coelho 
> > ---
> > 
> > In v2:
> >* fix DRM_PLANE_NO_SCALING renamed macros;
> > 
> > In v3:
> >* No changes.
> > 
> > In v4:
> >* Got rid of the changes in the general planes max scale code;
> >* Added a couple of FIXMEs;
> >* Made intel_atomic_setup_scaler() return an int with errors;
> > 
> > In v5:
> >* Just resent with a cover letter.
> > 
> > In v6:
> >* Now the correct version again (same as v4).
> > 
> > In v7:
> >* Constify a couple of local variables;
> >* Return -EINVAL, instead of -EOPNOTSUPP and -EBUSY;
> >* Add another FIXME;
> >* Remove unnecessary undoing of change in error cases.
> > 
> > 
> >  drivers/gpu/drm/i915/display/intel_atomic.c | 85 ++---
> >  1 file changed, 75 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c 
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..a9a3f3715279 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -41,6 +41,7 @@
> >  #include "intel_global_state.h"
> >  #include "intel_hdcp.h"
> >  #include "intel_psr.h"
> > +#include "intel_fb.h"
> >  #include "skl_universal_plane.h"
> >  
> >  /**
> > @@ -310,11 +311,11 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> > kfree(crtc_state);
> >  }
> >  
> > -static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state 
> > *scaler_state,
> > - int num_scalers_need, struct intel_crtc 
> > *intel_crtc,
> > - const char *name, int idx,
> > - struct intel_plane_state *plane_state,
> > - int *scaler_id)
> > +static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state 
> > *scaler_state,
> > +int num_scalers_need, struct intel_crtc 
> > *intel_crtc,
> > +const char *name, int idx,
> > +struct intel_plane_state *plane_state,
> > +int *scaler_id)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> > int j;
> > @@ -334,7 +335,7 @@ static void intel_atomic_setup_scaler(struct 
> > intel_crtc_scaler_state *scaler_sta
> >  
> > if (drm_WARN(_priv->drm, *scaler_id < 0,
> >  "Cannot find scaler for %s:%d\n", name, idx))
> > -   return;
> > +   return -EINVAL;
> 
> As I understand that change is a bit irrelevant to the patch topic,
> ideally it should be reflected in the commit message, that we are doing
> this and most importantly why.

Right, maybe this should have been mentioned in the commit message.  I
initially didn't return an error for the new failure path, but Ville
asked me to do so, so I changed the function to return an error here as
well.


> However I'm not going to be picky here, as it is a small thing, just
> as a side note.

Thanks!


> Reviewed-by: Stanislav Lisovskiy 

Thanks for the review, Stan!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v7 0/2] drm/i915/mtl: handle some MTL scaler limitations

[Coelho, Luciano]

On Fri, 2022-12-23 at 15:05 +0200, Luca Coelho wrote:
> Hi,
> 
> Here's an updated version of the patches after Ville's last comments.
> The versioning history is in the patches themselves.
> 
> Please review.
> 
> Cheers,
> Luca.
> 
> 
> Animesh Manna (1):
>   drm/i915/mtl: update scaler source and destination limits for MTL
> 
> Luca Coelho (1):
>   drm/i915/mtl: limit second scaler vertical scaling in ver >= 14
> 
>  drivers/gpu/drm/i915/display/intel_atomic.c | 85 ++---
>  drivers/gpu/drm/i915/display/skl_scaler.c   | 40 --
>  2 files changed, 107 insertions(+), 18 deletions(-)
> 

Hi Ville,

Can you please review this new revision of my patchset? For some reason
I forgot to CC you on this one. 

Thanks!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v7 2/2] drm/i915/mtl: update scaler source and destination limits for MTL

[Coelho, Luciano]

On Fri, 2022-12-23 at 19:42 +0530, Nautiyal, Ankit K wrote:
> On 12/23/2022 7:37 PM, Nautiyal, Ankit K wrote:
> > 
> > On 12/23/2022 6:35 PM, Luca Coelho wrote:
> > > From: Animesh Manna 
> > > 
> > > The max source and destination limits for scalers in MTL have changed.
> > > Use the new values accordingly.
> > > 
> > > Signed-off-by: José Roberto de Souza 
> > > Signed-off-by: Animesh Manna 
> > > Signed-off-by: Luca Coelho 
> > > ---
> > > 
> > > In v2:
> > >     * No changes;
> > > 
> > > In v3:
> > >     * Removed stray reviewed-by tag;
> > >     * Added my s-o-b.
> > > 
> > > In v4:
> > >     * No changes.
> > > 
> > > In v5:
> > >     * Just resent with a cover letter.
> > > 
> > > In v6:
> > >     * Now the correct version again (same as v4).
> > > 
> > > In v7:
> > >     * Update to new MTL limits according to the bspec.
> > > 
> > > 
> > >   drivers/gpu/drm/i915/display/skl_scaler.c | 40 ++-
> > >   1 file changed, 32 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c 
> > > b/drivers/gpu/drm/i915/display/skl_scaler.c
> > > index d7390067b7d4..01e881293612 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> > > @@ -87,6 +87,10 @@ static u16 skl_scaler_calc_phase(int sub, int 
> > > scale, bool chroma_cosited)
> > >   #define ICL_MAX_SRC_H 4096
> > >   #define ICL_MAX_DST_W 5120
> > >   #define ICL_MAX_DST_H 4096
> > > +#define MTL_MAX_SRC_W 4096
> > > +#define MTL_MAX_SRC_H 8192
> > > +#define MTL_MAX_DST_W 8192
> > > +#define MTL_MAX_DST_H 8192
> > >   #define SKL_MIN_YUV_420_SRC_W 16
> > >   #define SKL_MIN_YUV_420_SRC_H 16
> > >   @@ -103,6 +107,8 @@ skl_update_scaler(struct intel_crtc_state 
> > > *crtc_state, bool force_detach,
> > >   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >   const struct drm_display_mode *adjusted_mode =
> > >   _state->hw.adjusted_mode;
> > > +    int min_src_w, min_src_h, min_dst_w, min_dst_h;
> > > +    int max_src_w, max_src_h, max_dst_w, max_dst_h;
> > >     /*
> > >    * Src coordinates are already rotated by 270 degrees for
> > > @@ -157,15 +163,33 @@ skl_update_scaler(struct intel_crtc_state 
> > > *crtc_state, bool force_detach,
> > >   return -EINVAL;
> > >   }
> > >   +    min_src_w = SKL_MIN_SRC_W;
> > > +    min_src_h = SKL_MIN_SRC_H;
> > > +    min_dst_w = SKL_MIN_DST_W;
> > > +    min_dst_h = SKL_MIN_DST_H;
> > > +
> > > +    if (DISPLAY_VER(dev_priv) < 11) {
> > > +    max_src_w = SKL_MAX_SRC_W;
> > > +    max_src_h = SKL_MAX_SRC_H;
> > > +    max_dst_w = SKL_MAX_DST_W;
> > > +    max_dst_h = SKL_MAX_DST_H;
> > > +    } else if (DISPLAY_VER(dev_priv) < 14) {
> > > +    max_src_w = ICL_MAX_SRC_W;
> > > +    max_src_h = ICL_MAX_SRC_H;
> > > +    max_dst_w = ICL_MAX_DST_W;
> > > +    max_dst_h = ICL_MAX_DST_H;
> > 
> > Hi Luca,
> > 
> > Recently there is a change in Bspec:50441 and now for Gen 12 scalers, 
> > the MAX_SRC_W is 5120 pixels and MAX_SRC_H is 8192.
> 
> 
> Slight correction : this is for both Gen12,and 13.
> 
> Regards,
> 
> Ankit
> 
> 
> > 
> > MAX_DST_W, and MAX_DST_H are 8192.
> > 
> > As we are refactoring this part, can we include a separate patch for 
> > Gen 12 in this series?

Thanks for pointing out, Ankit!

But since my series is specifically targeting MTL, I'm going to send it
as a stand-alone patch, if that's ok.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v6 2/2] drm/i915/mtl: Limit scaler input to 4k in plane scaling

[Coelho, Luciano]

On Tue, 2022-12-20 at 23:10 +0200, Ville Syrjälä wrote:
> On Tue, Dec 20, 2022 at 02:07:24PM +0200, Luca Coelho wrote:
> > From: Animesh Manna 
> > 
> > As part of die area reduction max input source modified to 4096
> > for MTL so modified range check logic of scaler.
> > 
> > Signed-off-by: Jos? Roberto de Souza 
> > Signed-off-by: Animesh Manna 
> > Signed-off-by: Luca Coelho 
> > ---
> >  drivers/gpu/drm/i915/display/skl_scaler.c | 31 +--
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c 
> > b/drivers/gpu/drm/i915/display/skl_scaler.c
> > index d7390067b7d4..6baa07142b03 100644
> > --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> > +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> > @@ -103,6 +103,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, 
> > bool force_detach,
> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > const struct drm_display_mode *adjusted_mode =
> > _state->hw.adjusted_mode;
> > +   int min_src_w, min_src_h, min_dst_w, min_dst_h;
> > +   int max_src_w, max_src_h, max_dst_w, max_dst_h;
> >  
> > /*
> >  * Src coordinates are already rotated by 270 degrees for
> > @@ -157,15 +159,28 @@ skl_update_scaler(struct intel_crtc_state 
> > *crtc_state, bool force_detach,
> > return -EINVAL;
> > }
> >  
> > +   min_src_w = SKL_MIN_SRC_W;
> > +   min_src_h = SKL_MIN_SRC_H;
> > +   min_dst_w = SKL_MIN_DST_W;
> > +   min_dst_h = SKL_MIN_DST_H;
> > +
> > +   if (DISPLAY_VER(dev_priv) >= 11 && DISPLAY_VER(dev_priv) < 14) {
> > +   max_src_w = ICL_MAX_SRC_W;
> > +   max_src_h = ICL_MAX_SRC_H;
> > +   max_dst_w = ICL_MAX_DST_W;
> > +   max_dst_h = ICL_MAX_DST_H;
> > +   } else {
> > +   max_src_w = SKL_MAX_SRC_W;
> > +   max_src_h = SKL_MAX_SRC_H;
> > +   max_dst_w = SKL_MAX_DST_W;
> > +   max_dst_h = SKL_MAX_DST_H;
> > +   }
> 
> Bspec says max_src_w=4096, max_src_h=8192, max_dst_w=8192,
> max_dst_h=8192.

Yes, thanks for pointing out! I heard that these values were changed in
the bspec after the original implementation was made internally.  I
have updated the patch accordingly now.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v6 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Tue, 2022-12-20 at 23:00 +0200, Ville Syrjälä wrote:
> On Tue, Dec 20, 2022 at 02:07:23PM +0200, Luca Coelho wrote:
> > In newer hardware versions (i.e. display version >= 14), the second
> > scaler doesn't support vertical scaling.
> > 
> > The current implementation of the scaling limits is simplified and
> > only occurs when the planes are created, so we don't know which scaler
> > is being used.
> > 
> > In order to handle separate scaling limits for horizontal and vertical
> > scaling, and different limits per scaler, split the checks in two
> > phases.  We first do a simple check during plane creation and use the
> > best-case scenario (because we don't know the scaler that may be used
> > at a later point) and then do a more specific check when the scalers
> > are actually being set up.
> > 
> > Signed-off-by: Luca Coelho 
> > ---
> > 
> > In v2:
> >* fix DRM_PLANE_NO_SCALING renamed macros;
> > 
> > In v3:
> >* No changes.
> > 
> > In v4:
> >* Got rid of the changes in the general planes max scale code;
> >* Added a couple of FIXMEs;
> >* Made intel_atomic_setup_scaler() return an int with errors;
> > 
> > In v5:
> >* Just resent with a cover letter.
> > 
> > In v6:
> >* Now the correct version again (same as v4).
> > 
> > 
> > drivers/gpu/drm/i915/display/intel_atomic.c | 83 ++---
> >  1 file changed, 73 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c 
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..8373be283d8b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -41,6 +41,7 @@
> >  #include "intel_global_state.h"
> >  #include "intel_hdcp.h"
> >  #include "intel_psr.h"
> > +#include "intel_fb.h"
> >  #include "skl_universal_plane.h"
> >  
> >  /**
> > @@ -310,11 +311,11 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> > kfree(crtc_state);
> >  }
> >  
> > -static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state 
> > *scaler_state,
> > - int num_scalers_need, struct intel_crtc 
> > *intel_crtc,
> > - const char *name, int idx,
> > - struct intel_plane_state *plane_state,
> > - int *scaler_id)
> > +static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state 
> > *scaler_state,
> > +int num_scalers_need, struct intel_crtc 
> > *intel_crtc,
> > +const char *name, int idx,
> > +struct intel_plane_state *plane_state,
> > +int *scaler_id)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> > int j;
> > @@ -334,7 +335,7 @@ static void intel_atomic_setup_scaler(struct 
> > intel_crtc_scaler_state *scaler_sta
> >  
> > if (drm_WARN(_priv->drm, *scaler_id < 0,
> >  "Cannot find scaler for %s:%d\n", name, idx))
> > -   return;
> > +   return -EBUSY;
> >  
> > /* set scaler mode */
> > if (plane_state && plane_state->hw.fb &&
> > @@ -375,9 +376,69 @@ static void intel_atomic_setup_scaler(struct 
> > intel_crtc_scaler_state *scaler_sta
> > mode = SKL_PS_SCALER_MODE_DYN;
> > }
> >  
> > +   /*
> > +* FIXME: we should also check the scaler factors for pfit, so
> > +* this shouldn't be tied directly to planes.
> > +*/
> > +   if (plane_state && plane_state->hw.fb) {
> > +   const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +   struct drm_rect *src = _state->uapi.src;
> > +   struct drm_rect *dst = _state->uapi.dst;
> 
> Those can be const.

Done.


> > +   int hscale, vscale, max_vscale, max_hscale;
> > +
> > +   /*
> > +* FIXME: When two scalers are needed, but only one of
> > +* them needs to downscale, we should make sure that
> > +* the one that needs downscaling support is assigned
> > +* as the first scaler, so we don't reject downscaling
> > +* unnecessarily.
> > +*/
> > +
> > +   if (DISPLAY_VER(dev_priv) >= 14) {
> > +   /*
> > +* On versions 14 and up, only the first
> > +* scaler supports a vertical scaling factor
> > +* of more than 1.0, while a horizontal
> > +* scaling factor of 3.0 is supported.
> > +*/
> > +   max_hscale = 0x3 - 1;
> > +   if (*scaler_id == 0)
> > +   max_vscale = 0x3 - 1;
> > +   else
> > +   max_vscale = 0x1;
> > +
> > +   } else if (DISPLAY_VER(dev_priv) >= 10 ||
> > +  !intel_format_info_is_yuv_semiplanar(fb->format, 
> > 

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Tue, 2022-12-20 at 13:09 +0200, Ville Syrjälä wrote:
> On Tue, Dec 20, 2022 at 10:11:16AM +0200, Luca Coelho wrote:
> > In newer hardware versions (i.e. display version >= 14), the second
> > scaler doesn't support vertical scaling.
> > 
> > The current implementation of the scaling limits is simplified and
> > only occurs when the planes are created, so we don't know which scaler
> > is being used.
> > 
> > In order to handle separate scaling limits for horizontal and vertical
> > scaling, and different limits per scaler, split the checks in two
> > phases.  We first do a simple check during plane creation and use the
> > best-case scenario (because we don't know the scaler that may be used
> > at a later point) and then do a more specific check when the scalers
> > are actually being set up.
> > 
> > Signed-off-by: Luca Coelho 
> > ---
> > 
> > In v2:
> >* fix DRM_PLANE_NO_SCALING renamed macros;
> > 
> > In v3:
> >* No changes.
> > 
> > In v4:
> >* Got rid of the changes in the general planes max scale code;
> >* Added a couple of FIXMEs;
> >* Made intel_atomic_setup_scaler() return an int with errors;
> > 
> > In v5:
> >* Just resent with a cover letter.
> > 
> > drivers/gpu/drm/i915/display/i9xx_plane.c |  4 +-
> >  drivers/gpu/drm/i915/display/intel_atomic.c   | 83 ---
> >  .../gpu/drm/i915/display/intel_atomic_plane.c | 30 ++-
> >  .../gpu/drm/i915/display/intel_atomic_plane.h |  2 +-
> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  4 +-
> >  drivers/gpu/drm/i915/display/intel_sprite.c   | 20 +
> >  .../drm/i915/display/skl_universal_plane.c| 45 +-
> >  7 files changed, 128 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c 
> > b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > index ecaeb7dc196b..390e96f0692b 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > @@ -326,9 +326,7 @@ i9xx_plane_check(struct intel_crtc_state *crtc_state,
> > if (ret)
> > return ret;
> >  
> > -   ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
> > -   DRM_PLANE_NO_SCALING,
> > -   DRM_PLANE_NO_SCALING,
> > +   ret = intel_atomic_plane_check_clipping(plane_state, crtc_state, false,
> > 
> > i9xx_plane_has_windowing(plane));
> > if (ret)
> > return ret;
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c 
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..bf4761a40675 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -38,6 +38,7 @@
> >  #include "intel_atomic.h"
> >  #include "intel_cdclk.h"
> >  #include "intel_display_types.h"
> > +#include "intel_fb.h"
> >  #include "intel_global_state.h"
> >  #include "intel_hdcp.h"
> >  #include "intel_psr.h"
> > @@ -310,11 +311,11 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> > kfree(crtc_state);
> >  }
> >  
> > -static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state 
> > *scaler_state,
> > - int num_scalers_need, struct intel_crtc 
> > *intel_crtc,
> > - const char *name, int idx,
> > - struct intel_plane_state *plane_state,
> > - int *scaler_id)
> > +static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state 
> > *scaler_state,
> > +int num_scalers_need, struct intel_crtc 
> > *intel_crtc,
> > +const char *name, int idx,
> > +struct intel_plane_state *plane_state,
> > +int *scaler_id)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> > int j;
> > @@ -334,7 +335,7 @@ static void intel_atomic_setup_scaler(struct 
> > intel_crtc_scaler_state *scaler_sta
> >  
> > if (drm_WARN(_priv->drm, *scaler_id < 0,
> >  "Cannot find scaler for %s:%d\n", name, idx))
> > -   return;
> > +   return -EBUSY;
> >  
> > /* set scaler mode */
> > if (plane_state && plane_state->hw.fb &&
> > @@ -375,9 +376,69 @@ static void intel_atomic_setup_scaler(struct 
> > intel_crtc_scaler_state *scaler_sta
> > mode = SKL_PS_SCALER_MODE_DYN;
> > }
> >  
> > +   /*
> > +* FIXME: we should also check the scaler factors for pfit, so
> > +* this shouldn't be tied directly to planes.
> > +*/
> > +   if (plane_state && plane_state->hw.fb) {
> > +   const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +   struct drm_rect *src = _state->uapi.src;
> > +   struct drm_rect *dst = _state->uapi.dst;
> > +   int hscale, vscale, max_vscale, max_hscale;

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Wed, 2022-11-23 at 12:21 +0200, Luciano Coelho wrote:
> On Wed, 2022-11-23 at 11:57 +0200, Ville Syrjälä wrote:
> > On Wed, Nov 23, 2022 at 09:16:42AM +0000, Coelho, Luciano wrote:
> > > On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> > > > On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > > > > In newer hardware versions (i.e. display version >= 14), the second
> > > > > scaler doesn't support vertical scaling.
> > > > > 
> > > > > The current implementation of the scaling limits is simplified and
> > > > > only occurs when the planes are created, so we don't know which scaler
> > > > > is being used.
> > > > > 
> > > > > In order to handle separate scaling limits for horizontal and vertical
> > > > > scaling, and different limits per scaler, split the checks in two
> > > > > phases.  We first do a simple check during plane creation and use the
> > > > > best-case scenario (because we don't know the scaler that may be used
> > > > > at a later point) and then do a more specific check when the scalers
> > > > > are actually being set up.
> > > > > 
> > > > > Signed-off-by: Luca Coelho 
> > > > > ---
> > > 
> > > 
> > > [...]
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c 
> > > > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > index 6621aa245caf..43b1c7a227f8 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > @@ -38,6 +38,7 @@
> > > > >  #include "intel_atomic.h"
> > > > >  #include "intel_cdclk.h"
> > > > >  #include "intel_display_types.h"
> > > > > +#include "intel_fb.h"
> > > > >  #include "intel_global_state.h"
> > > > >  #include "intel_hdcp.h"
> > > > >  #include "intel_psr.h"
> > > > > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct 
> > > > > intel_crtc_scaler_state *scaler_sta
> > > > >   mode = SKL_PS_SCALER_MODE_DYN;
> > > > >   }
> > > > >  
> > > > > + if (plane_state && plane_state->hw.fb) {
> > > > > + const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > > + struct drm_rect *src = _state->uapi.src;
> > > > > + struct drm_rect *dst = _state->uapi.dst;
> > > > 
> > > > We want the scale factor checks for the pfit use case too. So this
> > > > stuff shouldn't be so tied into planes. I guess we could go with
> > > > a FIXME initially.
> > > 
> > > Okay, I'll add a FIXME.  It was tied to the plane checks before,
> > > though, wasn't it? So nothing should have changed in that regard here.
> > > 
> > > 
> > > > > + int hscale, vscale, max_vscale, max_hscale;
> > > > > +
> > > > > + if (DISPLAY_VER(dev_priv) >= 14) {
> > > > > + /*
> > > > > +  * On versions 14 and up, only the first
> > > > > +  * scaler supports a vertical scaling factor
> > > > > +  * of more than 1.0, while a horizontal
> > > > > +  * scaling factor of 3.0 is supported.
> > > > > +  */
> > > > > + max_hscale = 0x3 - 1;
> > > > > + if (*scaler_id == 0)
> > > > > + max_vscale = 0x3 - 1;
> > > > > + else
> > > > > + max_vscale = 0x1;
> > > > 
> > > > We still have the chicken vs. egg problem here that we'd need to
> > > > consider the scale factors already when selecting the scaler.
> > > > But that could be another FIXME.
> > > 
> > > Do you mean in regards to the HQ vs. non-HQ needs?
> > 
> > I mean the "no downscaling on the second scaler" thing. The
> > problem is that this thing walks the scaler users in order
> > and assigns each one a scaler in turn. So if the first user
> > doesn't need downscaling but the second one does

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Wed, 2022-11-23 at 11:57 +0200, Ville Syrjälä wrote:
> On Wed, Nov 23, 2022 at 09:16:42AM +0000, Coelho, Luciano wrote:
> > On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > > > In newer hardware versions (i.e. display version >= 14), the second
> > > > scaler doesn't support vertical scaling.
> > > > 
> > > > The current implementation of the scaling limits is simplified and
> > > > only occurs when the planes are created, so we don't know which scaler
> > > > is being used.
> > > > 
> > > > In order to handle separate scaling limits for horizontal and vertical
> > > > scaling, and different limits per scaler, split the checks in two
> > > > phases.  We first do a simple check during plane creation and use the
> > > > best-case scenario (because we don't know the scaler that may be used
> > > > at a later point) and then do a more specific check when the scalers
> > > > are actually being set up.
> > > > 
> > > > Signed-off-by: Luca Coelho 
> > > > ---
> > 
> > 
> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c 
> > > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > index 6621aa245caf..43b1c7a227f8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > @@ -38,6 +38,7 @@
> > > >  #include "intel_atomic.h"
> > > >  #include "intel_cdclk.h"
> > > >  #include "intel_display_types.h"
> > > > +#include "intel_fb.h"
> > > >  #include "intel_global_state.h"
> > > >  #include "intel_hdcp.h"
> > > >  #include "intel_psr.h"
> > > > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct 
> > > > intel_crtc_scaler_state *scaler_sta
> > > > mode = SKL_PS_SCALER_MODE_DYN;
> > > > }
> > > >  
> > > > +   if (plane_state && plane_state->hw.fb) {
> > > > +   const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > +   struct drm_rect *src = _state->uapi.src;
> > > > +   struct drm_rect *dst = _state->uapi.dst;
> > > 
> > > We want the scale factor checks for the pfit use case too. So this
> > > stuff shouldn't be so tied into planes. I guess we could go with
> > > a FIXME initially.
> > 
> > Okay, I'll add a FIXME.  It was tied to the plane checks before,
> > though, wasn't it? So nothing should have changed in that regard here.
> > 
> > 
> > > > +   int hscale, vscale, max_vscale, max_hscale;
> > > > +
> > > > +   if (DISPLAY_VER(dev_priv) >= 14) {
> > > > +   /*
> > > > +* On versions 14 and up, only the first
> > > > +* scaler supports a vertical scaling factor
> > > > +* of more than 1.0, while a horizontal
> > > > +* scaling factor of 3.0 is supported.
> > > > +*/
> > > > +   max_hscale = 0x3 - 1;
> > > > +   if (*scaler_id == 0)
> > > > +   max_vscale = 0x3 - 1;
> > > > +   else
> > > > +   max_vscale = 0x1;
> > > 
> > > We still have the chicken vs. egg problem here that we'd need to
> > > consider the scale factors already when selecting the scaler.
> > > But that could be another FIXME.
> > 
> > Do you mean in regards to the HQ vs. non-HQ needs?
> 
> I mean the "no downscaling on the second scaler" thing. The
> problem is that this thing walks the scaler users in order
> and assigns each one a scaler in turn. So if the first user
> doesn't need downscaling but the second one does then we
> will fail. OTOH had we just assigned the scalers in the
> reverse order we would have succeeded.

Ah, now I get it.

So, I guess we can do a similar thing to what we already do earlier in
this function to select the first scaler if HQ is needed.  If
downscaling is needed in one of the scalers but not in the other, we
can swap them to make sure the one that needs downscaling in in the
first o

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > In newer hardware versions (i.e. display version >= 14), the second
> > scaler doesn't support vertical scaling.
> > 
> > The current implementation of the scaling limits is simplified and
> > only occurs when the planes are created, so we don't know which scaler
> > is being used.
> > 
> > In order to handle separate scaling limits for horizontal and vertical
> > scaling, and different limits per scaler, split the checks in two
> > phases.  We first do a simple check during plane creation and use the
> > best-case scenario (because we don't know the scaler that may be used
> > at a later point) and then do a more specific check when the scalers
> > are actually being set up.
> > 
> > Signed-off-by: Luca Coelho 
> > ---


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c 
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..43b1c7a227f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -38,6 +38,7 @@
> >  #include "intel_atomic.h"
> >  #include "intel_cdclk.h"
> >  #include "intel_display_types.h"
> > +#include "intel_fb.h"
> >  #include "intel_global_state.h"
> >  #include "intel_hdcp.h"
> >  #include "intel_psr.h"
> > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct 
> > intel_crtc_scaler_state *scaler_sta
> > mode = SKL_PS_SCALER_MODE_DYN;
> > }
> >  
> > +   if (plane_state && plane_state->hw.fb) {
> > +   const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +   struct drm_rect *src = _state->uapi.src;
> > +   struct drm_rect *dst = _state->uapi.dst;
> 
> We want the scale factor checks for the pfit use case too. So this
> stuff shouldn't be so tied into planes. I guess we could go with
> a FIXME initially.

Okay, I'll add a FIXME.  It was tied to the plane checks before,
though, wasn't it? So nothing should have changed in that regard here.


> > +   int hscale, vscale, max_vscale, max_hscale;
> > +
> > +   if (DISPLAY_VER(dev_priv) >= 14) {
> > +   /*
> > +* On versions 14 and up, only the first
> > +* scaler supports a vertical scaling factor
> > +* of more than 1.0, while a horizontal
> > +* scaling factor of 3.0 is supported.
> > +*/
> > +   max_hscale = 0x3 - 1;
> > +   if (*scaler_id == 0)
> > +   max_vscale = 0x3 - 1;
> > +   else
> > +   max_vscale = 0x1;
> 
> We still have the chicken vs. egg problem here that we'd need to
> consider the scale factors already when selecting the scaler.
> But that could be another FIXME.

Do you mean in regards to the HQ vs. non-HQ needs?


> > +
> > +   } else if (DISPLAY_VER(dev_priv) >= 10 ||
> > +  !intel_format_info_is_yuv_semiplanar(fb->format, 
> > fb->modifier)) {
> > +   max_hscale = 0x3 - 1;
> > +   max_vscale = 0x3 - 1;
> > +   } else {
> > +   max_hscale = 0x2 - 1;
> > +   max_vscale = 0x2 - 1;
> > +   }
> 
> Pre-glk hq scaler case not handled.

I don't recall seen this specifically checked before.  Is this the
stuff I missed from g4x_sprite_check() below? Or am I missing something
else?


> > +
> > +   /* Check if required scaling is within limits */
> > +   hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
> > +   vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
> > +
> > +   if (hscale < 0 || vscale < 0) {
> > +   drm_dbg_kms(_priv->drm,
> > +   "Scaler %d doesn't support required plane 
> > scaling\n",
> > +   *scaler_id);
> > +   drm_rect_debug_print("src: ", src, true);
> > +   drm_rect_debug_print("dst: ", dst, false);
> > +
> > +   scaler_state->scalers[*scaler_id].in_use = 0;
> > +   *scaler_id = -1;
> > +
> > +   return;
> 
> This would have to return an error rather than pretending that
> everything is fine.

We were already pretending everything is fine if a scaler if we
couldn't find a free scaler, for instance, so I just kept the same
logic, clearing up the scaler_id and marking the scaler as not in use
as well.

I can convert this to return an error, of course.  But then in the "not
free scaler" case we would still just ignore it or should we change the
behavior and make it fail as well?


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c 
> > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 10e1fc9d0698..9100f328df60 100644
> > --- 

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/mtl: Limit scaler input to 4k in plane scaling

[Coelho, Luciano]

On Mon, 2022-11-21 at 13:50 +0200, Luca Coelho wrote:
> From: Animesh Manna 
> 
> As part of die area reduction max input source modified to 4096
> for MTL so modified range check logic of scaler.
> 
> Signed-off-by: José Roberto de Souza 
> Signed-off-by: Animesh Manna 
> Reviewed-by: Manasi Navare 

Oops, this is an internal reviewed-by tag that should not be here.

I'll send a new version.

--
Cheers,
Luca.

Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: disable FBC if its pipe is fused off

[Coelho, Luciano]

Hi,

This failure is not a real issue.  For some reason the 
igt@kms_writeback@writeback-check-output test was not running before and now it 
is.  The test is always getting skipped (even in local tests), so this result 
is as expected.  The test is also skipped without this patch.

This failure can be ignored.

--
Cheers,
Luca.

On Mon, 2022-09-26 at 19:13 +, Patchwork wrote:
Patch Details
Series: drm/i915: disable FBC if its pipe is fused off
URL:https://patchwork.freedesktop.org/series/109044/
State:  failure
Details:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109044v1/index.html
CI Bug Log - changes from CI_DRM_12182_full -> Patchwork_109044v1_full
Summary

FAILURE

Serious unknown changes coming with Patchwork_109044v1_full absolutely need to 
be
verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_109044v1_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.

Participating hosts (9 -> 11)

Additional (2): shard-rkl shard-tglu

Possible new issues

Here are the unknown changes that may have been introduced in 
Patchwork_109044v1_full:

IGT changes
Possible regressions

  *   igt@kms_writeback@writeback-check-output:
 *   shard-tglb: NOTRUN -> 
SKIP

Known issues

Here are the changes found in Patchwork_109044v1_full that come from known 
issues:

CI changes
IGT changes
Issues hit

  *   igt@gem_eio@wait-immediate:

 *   shard-apl: 
PASS
 -> 
DMESG-WARN
 (i915#62) +13 similar 
issues
  *   igt@gem_exec_balancer@parallel:

 *   shard-iclb: 
PASS
 -> 
SKIP
 (i915#4525) +1 similar 
issue
  *   igt@gem_exec_fair@basic-none@bcs0:

 *   shard-tglb: NOTRUN -> 
FAIL
 (i915#2842) +4 similar 
issues
  *   igt@gem_exec_fair@basic-pace@vecs0:

 *   shard-iclb: 
PASS
 -> 
FAIL
 (i915#2842)
  *   igt@gem_huc_copy@huc-copy:

 *   shard-tglb: 
PASS
 -> 
SKIP
 (i915#2190)
  *   igt@gem_lmem_swapping@heavy-multi:

 *   shard-tglb: NOTRUN -> 
SKIP
 (i915#4613)
  *   igt@gem_render_copy@linear-to-vebox-yf-tiled:

 *   shard-iclb: NOTRUN -> 
SKIP
 (i915#768)
  *   igt@gem_workarounds@suspend-resume:

 *   shard-apl: 
PASS
 -> 
DMESG-WARN
 (i915#180) +5 similar 
issues
  *   igt@gen9_exec_parse@bb-start-param:

 *   shard-tglb: NOTRUN -> 
SKIP
 (i915#2527 / 
i915#2856)
  *   igt@gen9_exec_parse@shadow-peek:

 *   shard-iclb: NOTRUN -> 
SKIP
 (i915#2856)
  *   igt@i915_pm_rc6_residency@rc6-idle@rcs0:

 *   shard-tglb: NOTRUN ->