Re: [Nouveau] [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

[Sean Paul]

On Wed, Jul 12, 2023 at 10:52 AM Jani Nikula  wrote:
>
> On Wed, 12 Jul 2023, Uwe Kleine-König  wrote:
> > Hello,
> >
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> >
> > I think there is a big benefit when these are all renamed to "drm_dev".
> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> >
> > Some statistics:
> >
> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq 
> > -c | sort -n
> >   1 struct drm_device *adev_to_drm
> >   1 struct drm_device *drm_
> >   1 struct drm_device  *drm_dev
> >   1 struct drm_device*drm_dev
> >   1 struct drm_device *pdev
> >   1 struct drm_device *rdev
> >   1 struct drm_device *vdev
> >   2 struct drm_device *dcss_drv_dev_to_drm
> >   2 struct drm_device **ddev
> >   2 struct drm_device *drm_dev_alloc
> >   2 struct drm_device *mock
> >   2 struct drm_device *p_ddev
> >   5 struct drm_device *device
> >   9 struct drm_device * dev
> >  25 struct drm_device *d
> >  95 struct drm_device *
> > 216 struct drm_device *ddev
> > 234 struct drm_device *drm_dev
> > 611 struct drm_device *drm
> >4190 struct drm_device *dev
> >
> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > it's not only me and others like the result of this effort it should be
> > followed up by adapting the other structs and the individual usages in
> > the different drivers.
>
> I think this is an unnecessary change. In drm, a dev is usually a drm
> device, i.e. struct drm_device *. As shown by the numbers above.
>

I'd really prefer this patch (series or single) is not accepted. This
will cause problems for everyone cherry-picking patches to a
downstream kernel (LTS or distro tree). I usually wouldn't expect
sympathy here, but the questionable benefit does not outweigh the cost
IM[biased]O.

Sean

> If folks insist on following through with this anyway, I'm firmly in the
> camp the name should be "drm" and nothing else.
>
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

[Sean Paul]

On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König
 wrote:
>
> hello Sean,
>
> On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul wrote:
> > I'd really prefer this patch (series or single) is not accepted. This
> > will cause problems for everyone cherry-picking patches to a
> > downstream kernel (LTS or distro tree). I usually wouldn't expect
> > sympathy here, but the questionable benefit does not outweigh the cost
> > IM[biased]O.
>
> I agree that for backports this isn't so nice. However with the split
> approach (that was argumented against here) it's not soo bad. Patch #1
> (and similar changes for the other affected structures) could be
> trivially backported and with that it doesn't matter if you write dev or
> drm (or whatever name is chosen in the end); both work in the same way.

Patch #1 avoids the need to backport the entire set, however every
change occuring after the rename patches will cause conflicts on
future cherry-picks. Downstream kernels will have to backport the
whole set. Backporting the entire set will create an epoch in
downstream kernels where cherry-picking patches preceding this set
will need to undergo conflict resolution as well. As mentioned in my
previous email, I don't expect sympathy here, it's part of maintaining
a downstream kernel, but there is a real cost to kernel consumers.

>
> But even with the one-patch-per-rename approach I'd consider the
> renaming a net win, because ease of understanding code has a big value.
> It's value is not so easy measurable as "conflicts when backporting",
> but it also matters in say two years from now, while backporting
> shouldn't be an issue then any more.

You've rightly identified the conjecture in your statement. I've been
on both sides of the argument, having written/maintained drm code
upstream and cherry-picked changes to a downstream kernel. Perhaps
it's because drm's definition of dev is ingrained in my muscle memory,
or maybe it's because I don't do a lot of upstream development these
days, but I just have a hard time seeing the benefit here.

I appreciate your engagement on the topic, thank you!

Sean

>
> Thanks for your input, best regards
> Uwe
>
> --
> Pengutronix e.K.   | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |

Re: [Nouveau] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible

[Sean Paul]

On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > Hi all,
> > > 
> > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was 
> > > to
> > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what 
> > > this
> > > patch series is about.
> > > 
> > > You will find two types of changes here:
> > > 
> > >   - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) 
> > > with
> > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it 
> > > has
> > > already been done in previous commits such as b7ea04d2)
> > > 
> > >   - Replacing "drm_modeset_lock_all()" with 
> > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > in the remaining places (as it has already been done in previous 
> > > commits
> > > such as 57037094)
> > > 
> > > Most of the changes are straight forward, except for a few cases in the 
> > > "amd"
> > > and "i915" drivers where some extra dancing was needed to overcome the
> > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be 
> > > used
> > > once inside the same function (the reason being that the macro expansion
> > > includes *labels*, and you can not have two labels named the same inside 
> > > one
> > > function)
> > > 
> > > Notice that, even after this patch series, some places remain where
> > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still 
> > > present,
> > > all inside drm core (which makes sense), except for two (in "amd" and 
> > > "i915")
> > > which cannot be replaced due to the way they are being used.
> > > 
> > > Changes in v2:
> > > 
> > >   - Fix commit message typo
> > >   - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > >   - Split drm/i915 patch into two simpler ones
> > >   - Remove drm_modeset_(un)lock_all()
> > >   - Fix build problems in non-x86 platforms
> > > 
> > > Fernando Ramos (17):
> > >   drm: cleanup: drm_modeset_lock_all_ctx() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/i915: cleanup: drm_modeset_lock_all_ctx() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/msm: cleanup: drm_modeset_lock_all_ctx() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() 
> > > drm/vmwgfx: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/tegra: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/shmobile: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/radeon: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/omapdrm: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/nouveau: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/msm: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/i915: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/i915: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > >   drm/gma500: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm/amd: cleanup: drm_modeset_lock_all() --> 
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > >   drm: cleanup: remove drm_modeset_(un)lock_all()
> > >   doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> > > 
> > 
> > Thank you for revising, Fernando! I've pushed the set to drm-misc-next 
> > (along
> > with the necessary drm-tip conflict resolutions).
> 
> Ugh. Did anyone actually review the locking changes this does?
> I shot the previous i915 stuff down because the commit messages
> did not address any of it.

I reviewed the set on 9/17, I didn't see your feedback on that thread.

Sean

> 
> -- 
> Ville Syrjälä
> Intel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible

[Sean Paul]

 drivers/gpu/drm/shmobile/shmob_drm_drv.c  |  6 +-
>  drivers/gpu/drm/tegra/dsi.c   |  6 +-
>  drivers/gpu/drm/tegra/hdmi.c  |  6 +-
>  drivers/gpu/drm/tegra/sor.c   | 11 ++-
>  drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 ++-
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   | 12 ++-
>  include/drm/drm_modeset_lock.h|  2 -
>  30 files changed, 265 insertions(+), 292 deletions(-)
> 
> 
> base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 15/15] doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:52PM +0200, Fernando Ramos wrote:
> The previous commits do exactly what this entry in the TODO file asks
> for, thus we can remove it now as it is no longer applicable.

Thanks for doing this work!

Can we remove drm_modeset_lock_all[_ctx] now? If so, let's queue that up as part
of the set.


Reviewed-by: Sean Paul 


> 
> Signed-off-by: Fernando Ramos 
> ---
>  Documentation/gpu/todo.rst| 17 -
>  Documentation/locking/ww-mutex-design.rst |  2 +-
>  2 files changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 12e61869939e..6613543955e9 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -353,23 +353,6 @@ converted, except for struct drm_driver.gem_prime_mmap.
>  
>  Level: Intermediate
>  
> -Use DRM_MODESET_LOCK_ALL_* helpers instead of boilerplate
> --
> -
> -For cases where drivers are attempting to grab the modeset locks with a local
> -acquire context. Replace the boilerplate code surrounding
> -drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN() and
> -DRM_MODESET_LOCK_ALL_END() instead.
> -
> -This should also be done for all places where drm_modeset_lock_all() is still
> -used.
> -
> -As a reference, take a look at the conversions already completed in drm core.
> -
> -Contact: Sean Paul, respective driver maintainers
> -
> -Level: Starter
> -
>  Rename CMA helpers to DMA helpers
>  -
>  
> diff --git a/Documentation/locking/ww-mutex-design.rst 
> b/Documentation/locking/ww-mutex-design.rst
> index 6a4d7319f8f0..6a8f8beb9ec4 100644
> --- a/Documentation/locking/ww-mutex-design.rst
> +++ b/Documentation/locking/ww-mutex-design.rst
> @@ -60,7 +60,7 @@ Concepts
>  Compared to normal mutexes two additional concepts/objects show up in the 
> lock
>  interface for w/w mutexes:
>  
> -Acquire context: To ensure eventual forward progress it is important the a 
> task
> +Acquire context: To ensure eventual forward progress it is important that a 
> task
>  trying to acquire locks doesn't grab a new reservation id, but keeps the one 
> it
>  acquired when starting the lock acquisition. This ticket is stored in the
>  acquire context. Furthermore the acquire context keeps track of debugging 
> state
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 14/15] drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

 drm_modeset_lock_all(dev);
> - dm_restore_drm_connector_state(dev, connector);
> - drm_modeset_unlock_all(dev);
> -
> - drm_kms_helper_hotplug_event(dev);
>   } else if (param[0] == 0) {
>   if (!aconnector->dc_link)
>   goto unlock;
> @@ -1259,13 +1257,16 @@ static ssize_t trigger_hotplug(struct file *f, const 
> char __user *buf,
>  
>   amdgpu_dm_update_connector_after_detect(aconnector);
>  
> - drm_modeset_lock_all(dev);
> - dm_restore_drm_connector_state(dev, connector);
> - drm_modeset_unlock_all(dev);
> -
> - drm_kms_helper_hotplug_event(dev);
> + } else {
> + goto unlock;
>   }
>  
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> + dm_restore_drm_connector_state(dev, connector);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);

Check ret here?

> +
> + drm_kms_helper_hotplug_event(dev);
> +
>  unlock:
>   mutex_unlock(>hpd_lock);
>  
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 13/15] drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:50PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/gma500/psb_device.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/psb_device.c 
> b/drivers/gpu/drm/gma500/psb_device.c
> index 951725a0f7a3..4e27f65a1f16 100644
> --- a/drivers/gpu/drm/gma500/psb_device.c
> +++ b/drivers/gpu/drm/gma500/psb_device.c
> @@ -8,6 +8,7 @@
>  #include 
>  
>  #include 
> +#include 
>  
>  #include "gma_device.h"
>  #include "intel_bios.h"
> @@ -169,8 +170,10 @@ static int psb_save_display_registers(struct drm_device 
> *dev)
>  {
>   struct drm_psb_private *dev_priv = dev->dev_private;
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
>   struct gma_connector *connector;
>   struct psb_state *regs = _priv->regs.psb;
> + int ret;
>  
>   /* Display arbitration control + watermarks */
>   regs->saveDSPARB = PSB_RVDC32(DSPARB);
> @@ -183,7 +186,7 @@ static int psb_save_display_registers(struct drm_device 
> *dev)
>   regs->saveCHICKENBIT = PSB_RVDC32(DSPCHICKENBIT);
>  
>   /* Save crtc and output state */
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   list_for_each_entry(crtc, >mode_config.crtc_list, head) {
>   if (drm_helper_crtc_in_use(crtc))
>   dev_priv->ops->save_crtc(crtc);
> @@ -193,7 +196,8 @@ static int psb_save_display_registers(struct drm_device 
> *dev)
>   if (connector->save)
>   connector->save(>base);
>  
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> +
>   return 0;

Return ret here please

>  }
>  
> @@ -207,8 +211,10 @@ static int psb_restore_display_registers(struct 
> drm_device *dev)
>  {
>   struct drm_psb_private *dev_priv = dev->dev_private;
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
>   struct gma_connector *connector;
>   struct psb_state *regs = _priv->regs.psb;
> + int ret;
>  
>   /* Display arbitration + watermarks */
>   PSB_WVDC32(regs->saveDSPARB, DSPARB);
> @@ -223,7 +229,7 @@ static int psb_restore_display_registers(struct 
> drm_device *dev)
>   /*make sure VGA plane is off. it initializes to on after reset!*/
>   PSB_WVDC32(0x8000, VGACNTRL);
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   list_for_each_entry(crtc, >mode_config.crtc_list, head)
>   if (drm_helper_crtc_in_use(crtc))
>   dev_priv->ops->restore_crtc(crtc);
> @@ -232,7 +238,7 @@ static int psb_restore_display_registers(struct 
> drm_device *dev)
>   if (connector->restore)
>   connector->restore(>base);
>  
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>   return 0;

Here too

>  }
>  
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

+76,7 @@ static int i9xx_pipe_crc_auto_source(struct 
> drm_i915_private *dev_priv,
>enum intel_pipe_crc_source *source)
>  {
>   struct drm_device *dev = _priv->drm;
> + struct drm_modeset_acquire_ctx ctx;
>   struct intel_encoder *encoder;
>   struct intel_crtc *crtc;
>   struct intel_digital_port *dig_port;
> @@ -83,7 +84,7 @@ static int i9xx_pipe_crc_auto_source(struct 
> drm_i915_private *dev_priv,
>  
>   *source = INTEL_PIPE_CRC_SOURCE_PIPE;
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   for_each_intel_encoder(dev, encoder) {
>   if (!encoder->base.crtc)
>   continue;
> @@ -120,7 +121,7 @@ static int i9xx_pipe_crc_auto_source(struct 
> drm_i915_private *dev_priv,
>   break;
>   }
>   }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  
>   return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 59fb4c710c8c..7a30e2ff2fed 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1009,31 +1009,35 @@ static void i915_driver_postclose(struct drm_device 
> *dev, struct drm_file *file)
>  static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
>  {
>   struct drm_device *dev = _priv->drm;
> + struct drm_modeset_acquire_ctx ctx;
>   struct intel_encoder *encoder;
> + int ret;
>  
>   if (!HAS_DISPLAY(dev_priv))
>   return;
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   for_each_intel_encoder(dev, encoder)
>   if (encoder->suspend)
>   encoder->suspend(encoder);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  }
>  
>  static void intel_shutdown_encoders(struct drm_i915_private *dev_priv)
>  {
>   struct drm_device *dev = _priv->drm;
> + struct drm_modeset_acquire_ctx ctx;
>   struct intel_encoder *encoder;
> + int ret;
>  
>   if (!HAS_DISPLAY(dev_priv))
>   return;
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   for_each_intel_encoder(dev, encoder)
>   if (encoder->shutdown)
>   encoder->shutdown(encoder);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  }
>  
>  void i915_driver_shutdown(struct drm_i915_private *i915)
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 11/15] drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:48PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 768012243b44..4cbc79eaee17 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1172,14 +1172,16 @@ static int _dpu_debugfs_status_show(struct seq_file 
> *s, void *data)
>   struct drm_display_mode *mode;
>   struct drm_framebuffer *fb;
>   struct drm_plane_state *state;
> + struct drm_modeset_acquire_ctx ctx;
>   struct dpu_crtc_state *cstate;
>  
>   int i, out_width;
> + int ret;

Please put ret with i & out_width

>  
>   dpu_crtc = s->private;
>   crtc = _crtc->base;
>  
> - drm_modeset_lock_all(crtc->dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(crtc->dev, ctx, 0, ret);
>   cstate = to_dpu_crtc_state(crtc->state);
>  
>   mode = >state->adjusted_mode;
> @@ -1263,7 +1265,7 @@ static int _dpu_debugfs_status_show(struct seq_file *s, 
> void *data)
>   dpu_crtc->vblank_cb_time = ktime_set(0, 0);
>   }
>  
> - drm_modeset_unlock_all(crtc->dev);
> + DRM_MODESET_LOCK_ALL_END(crtc->dev, ctx, ret);
>  
>   return 0;

Return ret here

>  }
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 10/15] drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:47PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c 
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index d7b9f7f8c9e3..eb613af4cdd5 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -667,15 +667,17 @@ nv50_audio_component_bind(struct device *kdev, struct 
> device *hda_kdev,
>   struct drm_device *drm_dev = dev_get_drvdata(kdev);
>   struct nouveau_drm *drm = nouveau_drm(drm_dev);
>   struct drm_audio_component *acomp = data;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>  
>   if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
>   return -ENOMEM;
>  
> - drm_modeset_lock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret);
>   acomp->ops = _audio_component_ops;
>   acomp->dev = kdev;
>   drm->audio.component = acomp;
> - drm_modeset_unlock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret);
>   return 0;

Return ret here, with that fixed,

Reviewed-by: Sean Paul 


>  }
>  
> @@ -686,12 +688,14 @@ nv50_audio_component_unbind(struct device *kdev, struct 
> device *hda_kdev,
>   struct drm_device *drm_dev = dev_get_drvdata(kdev);
>   struct nouveau_drm *drm = nouveau_drm(drm_dev);
>   struct drm_audio_component *acomp = data;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>  
> - drm_modeset_lock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret);
>   drm->audio.component = NULL;
>   acomp->ops = NULL;
>   acomp->dev = NULL;
> - drm_modeset_unlock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret);
>  }
>  
>  static const struct component_ops nv50_audio_component_bind_ops = {
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 09/15] drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:46PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c 
> b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 190afc564914..56013c3ef701 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -62,13 +62,15 @@ static int omap_framebuffer_dirty(struct drm_framebuffer 
> *fb,
> unsigned num_clips)
>  {
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>  
> - drm_modeset_lock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(fb->dev, ctx, 0, ret);
>  
>   drm_for_each_crtc(crtc, fb->dev)
>   omap_crtc_flush(crtc);
>  
> - drm_modeset_unlock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret);
>  
>   return 0;

Return ret here, with that,

Reviewed-by: Sean Paul 


>  }
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 09/15] drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:46PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c 
> b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 190afc564914..56013c3ef701 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -62,13 +62,15 @@ static int omap_framebuffer_dirty(struct drm_framebuffer 
> *fb,
> unsigned num_clips)
>  {
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>  
> - drm_modeset_lock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(fb->dev, ctx, 0, ret);
>  
>   drm_for_each_crtc(crtc, fb->dev)
>   omap_crtc_flush(crtc);
>  
> - drm_modeset_unlock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret);
>  
>   return 0;

Return ret here

>  }
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 08/15] drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:45PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/radeon/radeon_device.c | 13 +
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 +--
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c 
> b/drivers/gpu/drm/radeon/radeon_device.c
> index 4f0fbf667431..3feb1fd44409 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1559,7 +1560,9 @@ int radeon_suspend_kms(struct drm_device *dev, bool 
> suspend,
>   struct pci_dev *pdev;
>   struct drm_crtc *crtc;
>   struct drm_connector *connector;
> + struct drm_modeset_acquire_ctx ctx;
>   int i, r;
> + int ret;

Could you please tuck this up with i & r?

>  
>   if (dev == NULL || dev->dev_private == NULL) {
>   return -ENODEV;
> @@ -1573,12 +1576,12 @@ int radeon_suspend_kms(struct drm_device *dev, bool 
> suspend,
>  
>   drm_kms_helper_poll_disable(dev);
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   /* turn off display hw */
>   list_for_each_entry(connector, >mode_config.connector_list, head) {
>   drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
>   }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);

You should check ret here

>  
>   /* unpin the front buffers and cursors */
>   list_for_each_entry(crtc, >mode_config.crtc_list, head) {
> @@ -1663,7 +1666,9 @@ int radeon_resume_kms(struct drm_device *dev, bool 
> resume, bool fbcon)
>   struct radeon_device *rdev = dev->dev_private;
>   struct pci_dev *pdev = to_pci_dev(dev->dev);
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
>   int r;
> + int ret;

Same suggestion here, move up with r

>  
>   if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>   return 0;
> @@ -1741,11 +1746,11 @@ int radeon_resume_kms(struct drm_device *dev, bool 
> resume, bool fbcon)
>   if (fbcon) {
>   drm_helper_resume_force_mode(dev);
>   /* turn on display hw */
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   list_for_each_entry(connector, 
> >mode_config.connector_list, head) {
>   drm_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
>   }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);

Also check ret here


>   }
>  
>   drm_kms_helper_poll_enable(dev);
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c 
> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index ec867fa880a4..04fe7b0a6746 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -4,6 +4,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "atom.h"
>  #include "ni_reg.h"
> @@ -737,11 +738,13 @@ static int radeon_debugfs_mst_info_show(struct seq_file 
> *m, void *unused)
>   struct radeon_device *rdev = (struct radeon_device *)m->private;
>   struct drm_device *dev = rdev->ddev;
>   struct drm_connector *connector;
> + struct drm_modeset_acquire_ctx ctx;
>   struct radeon_connector *radeon_connector;
>   struct radeon_connector_atom_dig *dig_connector;
>   int i;
> + int ret;

Move up with i

>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   list_for_each_entry(connector, >mode_config.connector_list, head) {
>   if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
>   continue;
> @@ -759,7 +762,7 @@ static int radeon_debugfs_mst_info_show(struct seq_file 
> *m, void *unused)
>  radeon_connector->cur_stream_attribs[i].fe,
>  
> radeon_connector->cur_stream_attribs[i].slots);
>   }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>   return 0;
>  }
>  
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 07/15] drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:44PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c 
> b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> index 7db01904d18d..8ee215ab614e 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> @@ -156,10 +156,12 @@ static int shmob_drm_pm_suspend(struct device *dev)
>  static int shmob_drm_pm_resume(struct device *dev)
>  {
>   struct shmob_drm_device *sdev = dev_get_drvdata(dev);
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>  
> - drm_modeset_lock_all(sdev->ddev);
> + DRM_MODESET_LOCK_ALL_BEGIN(sdev->ddev, ctx, 0, ret);
>   shmob_drm_crtc_resume(>crtc);
> - drm_modeset_unlock_all(sdev->ddev);
> + DRM_MODESET_LOCK_ALL_END(sdev->ddev, ctx, ret);
>  
>   drm_kms_helper_poll_enable(sdev->ddev);
>   return 0;
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 06/15] drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:43PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/tegra/dsi.c  |  6 --
>  drivers/gpu/drm/tegra/hdmi.c |  5 +++--
>  drivers/gpu/drm/tegra/sor.c  | 10 ++
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f46d377f0c30..77a496f6a2e9 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -202,10 +202,12 @@ static int tegra_dsi_show_regs(struct seq_file *s, void 
> *data)
>   struct tegra_dsi *dsi = node->info_ent->data;
>   struct drm_crtc *crtc = dsi->output.encoder.crtc;
>   struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
>   unsigned int i;
>   int err = 0;
> + int ret;

You can use err here instead. With that fixed,

Reviewed-by: Sean Paul 


>  
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, ret);
>  
>   if (!crtc || !crtc->state->active) {
>   err = -EBUSY;
> @@ -220,7 +222,7 @@ static int tegra_dsi_show_regs(struct seq_file *s, void 
> *data)
>   }
>  
>  unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, ret);
>   return err;
>  }
>  
> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> index e5d2a4026028..669a2ebb55ae 100644
> --- a/drivers/gpu/drm/tegra/hdmi.c
> +++ b/drivers/gpu/drm/tegra/hdmi.c
> @@ -1031,10 +1031,11 @@ static int tegra_hdmi_show_regs(struct seq_file *s, 
> void *data)
>   struct tegra_hdmi *hdmi = node->info_ent->data;
>   struct drm_crtc *crtc = hdmi->output.encoder.crtc;
>   struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
>   unsigned int i;
>   int err = 0;
>  
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>  
>   if (!crtc || !crtc->state->active) {
>   err = -EBUSY;
> @@ -1049,7 +1050,7 @@ static int tegra_hdmi_show_regs(struct seq_file *s, 
> void *data)
>   }
>  
>  unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
>   return err;
>  }
>  
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index 0ea320c1092b..323d95eb0cac 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -1490,10 +1490,11 @@ static int tegra_sor_show_crc(struct seq_file *s, 
> void *data)
>   struct tegra_sor *sor = node->info_ent->data;
>   struct drm_crtc *crtc = sor->output.encoder.crtc;
>   struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
>   int err = 0;
>   u32 value;
>  
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>  
>   if (!crtc || !crtc->state->active) {
>   err = -EBUSY;
> @@ -1522,7 +1523,7 @@ static int tegra_sor_show_crc(struct seq_file *s, void 
> *data)
>   seq_printf(s, "%08x\n", value);
>  
>  unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
>   return err;
>  }
>  
> @@ -1652,10 +1653,11 @@ static int tegra_sor_show_regs(struct seq_file *s, 
> void *data)
>   struct tegra_sor *sor = node->info_ent->data;
>   struct drm_crtc *crtc = sor->output.encoder.crtc;
>   struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
>   unsigned int i;
>   int err = 0;
>  
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>  
>   if (!crtc || !crtc->state->active) {
>   err = -EBUSY;
> @@ -1670,7 +1672,7 @@ static int tegra_sor_show_regs(struct seq_file *s, void 
> *data)
>   }
>  
>  unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
>   return err;
>  }
>  
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 05/15] drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:42PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 

Reviewed-by: Sean Paul 

> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 +++
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   | 12 
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c 
> b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> index 28af34ab6ed6..7df35c6f1458 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> @@ -28,6 +28,7 @@
>  #include "vmwgfx_drv.h"
>  #include "vmwgfx_devcaps.h"
>  #include 
> +#include 
>  #include "vmwgfx_kms.h"
>  
>  int vmw_getparam_ioctl(struct drm_device *dev, void *data,
> @@ -172,6 +173,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
>   struct drm_vmw_rect __user *clips_ptr;
>   struct drm_vmw_rect *clips = NULL;
>   struct drm_framebuffer *fb;
> + struct drm_modeset_acquire_ctx ctx;
>   struct vmw_framebuffer *vfb;
>   struct vmw_resource *res;
>   uint32_t num_clips;
> @@ -203,7 +205,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
>   goto out_no_copy;
>   }
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  
>   fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
>   if (!fb) {
> @@ -231,7 +233,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
>  out_no_surface:
>   drm_framebuffer_put(fb);
>  out_no_fb:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  out_no_copy:
>   kfree(clips);
>  out_clips:
> @@ -250,6 +252,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, 
> void *data,
>   struct drm_vmw_rect __user *clips_ptr;
>   struct drm_vmw_rect *clips = NULL;
>   struct drm_framebuffer *fb;
> + struct drm_modeset_acquire_ctx ctx;
>   struct vmw_framebuffer *vfb;
>   uint32_t num_clips;
>   int ret;
> @@ -280,7 +283,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, 
> void *data,
>   goto out_no_copy;
>   }
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  
>   fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
>   if (!fb) {
> @@ -303,7 +306,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, 
> void *data,
>  out_no_ttm_lock:
>   drm_framebuffer_put(fb);
>  out_no_fb:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  out_no_copy:
>   kfree(clips);
>  out_clips:
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c 
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 74fa41909213..268095cb8c84 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "vmwgfx_kms.h"
>  
> @@ -243,15 +244,17 @@ void vmw_kms_legacy_hotspot_clear(struct vmw_private 
> *dev_priv)
>   struct drm_device *dev = _priv->drm;
>   struct vmw_display_unit *du;
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   drm_for_each_crtc(crtc, dev) {
>   du = vmw_crtc_to_du(crtc);
>  
>   du->hotspot_x = 0;
>   du->hotspot_y = 0;
>   }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  }
>  
>  void vmw_kms_cursor_post_execbuf(struct vmw_private *dev_priv)
> @@ -1012,9 +1015,10 @@ static int vmw_framebuffer_bo_dirty(struct 
> drm_framebuffer *framebuffer,
>   struct vmw_framebuffer_bo *vfbd =
>   vmw_framebuffer_to_vfbd(framebuffer);
>   struct drm_clip_rect norect;
> + struct drm_modeset_acquire_ctx ctx;
>   int ret, increment = 1;
>  
> - drm_modeset_lock_all(_priv->drm);
> +     DRM_MODESET_LOCK_ALL_BEGIN((_priv->drm), ctx, 0, ret);
>  
>   if (!num_clips) {
>   num_clips = 1;
> @@ -1040,7 +1044,7 @@ static int vmw_framebuffer_bo_dirty(struct 
> drm_framebuffer *framebuffer,
>  
>   vmw_cmd_flush(dev_priv, false);
>  
> - drm_modeset_unlock_all(_priv->drm);
> + DRM_MODESET_LOCK_ALL_END((_priv->drm), ctx, ret);
>  
>   return ret;
>  }
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 04/15] drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:41PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
> 
> Signed-off-by: Fernando Ramos 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_client_modeset.c |  5 +++--
>  drivers/gpu/drm/drm_crtc_helper.c| 18 --
>  drivers/gpu/drm/drm_fb_helper.c  | 10 ++
>  drivers/gpu/drm/drm_framebuffer.c|  6 --
>  4 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c 
> b/drivers/gpu/drm/drm_client_modeset.c
> index 5f5184f071ed..43f772543d2a 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -1062,9 +1062,10 @@ static int drm_client_modeset_commit_legacy(struct 
> drm_client_dev *client)
>   struct drm_device *dev = client->dev;
>   struct drm_mode_set *mode_set;
>   struct drm_plane *plane;
> + struct drm_modeset_acquire_ctx ctx;
>   int ret = 0;
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   drm_for_each_plane(plane, dev) {
>   if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>   drm_plane_force_disable(plane);
> @@ -1093,7 +1094,7 @@ static int drm_client_modeset_commit_legacy(struct 
> drm_client_dev *client)
>   goto out;
>   }
>  out:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  
>   return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
> b/drivers/gpu/drm/drm_crtc_helper.c
> index bff917531f33..f3ce073dff79 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -218,11 +218,14 @@ static void 
> __drm_helper_disable_unused_functions(struct drm_device *dev)
>   */
>  void drm_helper_disable_unused_functions(struct drm_device *dev)
>  {
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
> +
>   WARN_ON(drm_drv_uses_atomic_modeset(dev));
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   __drm_helper_disable_unused_functions(dev);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  }
>  EXPORT_SYMBOL(drm_helper_disable_unused_functions);
>  
> @@ -942,12 +945,14 @@ void drm_helper_resume_force_mode(struct drm_device 
> *dev)
>   struct drm_crtc *crtc;
>   struct drm_encoder *encoder;
>   const struct drm_crtc_helper_funcs *crtc_funcs;
> + struct drm_modeset_acquire_ctx ctx;
>   int encoder_dpms;
>   bool ret;
> + int err;
>  
>   WARN_ON(drm_drv_uses_atomic_modeset(dev));
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
>   drm_for_each_crtc(crtc, dev) {
>  
>   if (!crtc->enabled)
> @@ -982,7 +987,7 @@ void drm_helper_resume_force_mode(struct drm_device *dev)
>  
>   /* disable the unused connectors while restoring the modesetting */
>   __drm_helper_disable_unused_functions(dev);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
>  }
>  EXPORT_SYMBOL(drm_helper_resume_force_mode);
>  
> @@ -1002,9 +1007,10 @@ EXPORT_SYMBOL(drm_helper_resume_force_mode);
>  int drm_helper_force_disable_all(struct drm_device *dev)
>  {
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
>   int ret = 0;
>  
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>   drm_for_each_crtc(crtc, dev)
>   if (crtc->enabled) {
>   struct drm_mode_set set = {
> @@ -1016,7 +1022,7 @@ int drm_helper_force_disable_all(struct drm_device *dev)
>   goto out;
>   }
>  out:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>   return ret;
>  }
>  EXPORT_SYMBOL(drm_helper_force_disable_all);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 3ab078321045..6860223f0068 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -940,10 +940,11 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct 
> fb_info *info)
>   struct drm_fb_helper *fb_helper = info->par;
>   struct drm_mode_set *modeset;
>   struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
>   u16 *r, *g, *b;
>   int ret = 0;
>  
> - drm_modeset_l

Re: [Nouveau] [PATCH 02/15] dmr/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:39PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace the boilerplate code
> surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
> and DRM_MODESET_LOCK_ALL_END()
> 

With the subject fixed (s/dmr/drm),

Reviewed-by: Sean Paul 

> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 17 -
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 134a6acbd8fb..997a16e85c85 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13476,22 +13476,13 @@ void intel_display_resume(struct drm_device *dev)
>   if (state)
>   state->acquire_ctx = 
>  
> - drm_modeset_acquire_init(, 0);
> -
> - while (1) {
> - ret = drm_modeset_lock_all_ctx(dev, );
> - if (ret != -EDEADLK)
> - break;
> -
> - drm_modeset_backoff();
> - }
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  
> - if (!ret)
> - ret = __intel_display_resume(dev, state, );
> + ret = __intel_display_resume(dev, state, );
>  
>   intel_enable_ipc(dev_priv);
> - drm_modeset_drop_locks();
> - drm_modeset_acquire_fini();
> +
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>  
>   if (ret)
>   drm_err(_priv->drm,
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 03/15] dmr/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:40PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace the boilerplate code
> surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
> and DRM_MODESET_LOCK_ALL_END()
> 

With the subject fixed (s/dmr/drm/),

Reviewed-by: Sean Paul 

> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c 
> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> index cabe15190ec1..c83db90b0e02 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> @@ -99,20 +99,18 @@ static void msm_disp_capture_atomic_state(struct 
> msm_disp_state *disp_state)
>  {
>   struct drm_device *ddev;
>   struct drm_modeset_acquire_ctx ctx;
> + int ret;
>  
>   disp_state->timestamp = ktime_get();
>  
>   ddev = disp_state->drm_dev;
>  
> - drm_modeset_acquire_init(, 0);
> -
> - while (drm_modeset_lock_all_ctx(ddev, ) != 0)
> - drm_modeset_backoff();
> + DRM_MODESET_LOCK_ALL_BEGIN(ddev, ctx, 0, ret);
>  
>   disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
>   );
> - drm_modeset_drop_locks();
> - drm_modeset_acquire_fini();
> +
> + DRM_MODESET_LOCK_ALL_END(ddev, ctx, ret);
>  }
>  
>  void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Nouveau] [PATCH 01/15] dmr: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()

[Sean Paul]

On Thu, Sep 16, 2021 at 11:15:38PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace the boilerplate code
> surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
> and DRM_MODESET_LOCK_ALL_END()
> 

Hi Fernando,
Thank you for your patch. Could you please fix the subject, changing dmr to drm?

> Signed-off-by: Fernando Ramos 
> ---
>  drivers/gpu/drm/drm_client_modeset.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c 
> b/drivers/gpu/drm/drm_client_modeset.c
> index ced09c7c06f9..5f5184f071ed 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -574,6 +574,7 @@ static bool drm_client_firmware_config(struct 
> drm_client_dev *client,
>   int num_connectors_detected = 0;
>   int num_tiled_conns = 0;
>   struct drm_modeset_acquire_ctx ctx;
> + int err;

I think you can just reuse 'ret' instead of creating a new variable. That
ensures if the lock fails we return the error from the macros.

Sean

>  
>   if (!drm_drv_uses_atomic_modeset(dev))
>   return false;
> @@ -585,10 +586,7 @@ static bool drm_client_firmware_config(struct 
> drm_client_dev *client,
>   if (!save_enabled)
>   return false;
>  
> - drm_modeset_acquire_init(, 0);
> -
> - while (drm_modeset_lock_all_ctx(dev, ) != 0)
> - drm_modeset_backoff();
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
>  
>   memcpy(save_enabled, enabled, count);
>   mask = GENMASK(count - 1, 0);
> @@ -743,8 +741,7 @@ static bool drm_client_firmware_config(struct 
> drm_client_dev *client,
>   ret = false;
>   }
>  
> - drm_modeset_drop_locks();
> - drm_modeset_acquire_fini();
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
>  
>   kfree(save_enabled);
>   return ret;
> -- 
> 2.33.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[Nouveau] [RESEND PATCH v6 08/14] drm/nouveau: Change debug checks to specifically target syslog

[Sean Paul]

From: Sean Paul 

Since the logs protected by these checks specifically target syslog,
use the new drm_debug_syslog_enabled() call to avoid triggering
these prints when only trace is enabled.

Signed-off-by: Sean Paul 
Link: 
https://patchwork.freedesktop.org/patch/msgid/20200608210505.48519-9-s...@poorly.run
 #v5

Changes in v5:
-Added to the set
Changes in v6:
-Rebased on drm-tip, changes in dispnv50/disp.h were rebased out
---
 drivers/gpu/drm/nouveau/nouveau_drv.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h 
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index ba65f136cf48..2caa5720f451 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -260,11 +260,11 @@ void nouveau_drm_device_remove(struct drm_device *dev);
 #define NV_INFO(drm,f,a...) NV_PRINTK(info, &(drm)->client, f, ##a)
 
 #define NV_DEBUG(drm,f,a...) do {  
\
-   if (drm_debug_enabled(DRM_UT_DRIVER))  \
+   if (drm_debug_syslog_enabled(DRM_UT_DRIVER))
  \
NV_PRINTK(info, &(drm)->client, f, ##a);   \
 } while(0)
 #define NV_ATOMIC(drm,f,a...) do { 
\
-   if (drm_debug_enabled(DRM_UT_ATOMIC))  \
+   if (drm_debug_syslog_enabled(DRM_UT_ATOMIC))
  \
NV_PRINTK(info, &(drm)->client, f, ##a);   \
 } while(0)
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [Intel-gfx] [RFC v2 13/20] drm/i915/dp: Extract drm_dp_downstream_read_info()

[Sean Paul]

On Thu, Aug 20, 2020 at 2:31 PM Lyude Paul  wrote:
>
> We're going to be doing the same probing process in nouveau for
> determining downstream DP port capabilities, so let's deduplicate the
> work by moving i915's code for handling this into a shared helper:
> drm_dp_downstream_read_info().
>
> Note that when we do this, we also do make some functional changes while
> we're at it:
> * We always clear the downstream port info before trying to read it,
>   just to make things easier for the caller
> * We skip reading downstream port info if the DPCD indicates that we
>   don't support downstream port info
> * We only read as many bytes as needed for the reported number of
>   downstream ports, no sense in reading the whole thing every time
>
> v2:
> * Fixup logic for calculating the downstream port length to account for
>   the fact that downstream port caps can be either 1 byte or 4 bytes
>   long. We can actually skip fixing the max_clock/max_bpc helpers here
>   since they all check for DP_DETAILED_CAP_INFO_AVAILABLE anyway.
> * Fix ret code check for drm_dp_dpcd_read
>

Thanks for sorting this out!

Reviewed-by: Sean Paul 

> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 46 +
>  drivers/gpu/drm/i915/display/intel_dp.c | 14 ++--
>  include/drm/drm_dp_helper.h |  3 ++
>  3 files changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 4c21cf69dad5a..4f845995f1f66 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -423,6 +423,52 @@ bool drm_dp_send_real_edid_checksum(struct drm_dp_aux 
> *aux,
>  }
>  EXPORT_SYMBOL(drm_dp_send_real_edid_checksum);
>
> +static u8 drm_dp_downstream_port_count(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> +   u8 port_count = dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_PORT_COUNT_MASK;
> +
> +   if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE 
> && port_count > 4)
> +   port_count = 4;
> +
> +   return port_count;
> +}
> +
> +/**
> + * drm_dp_downstream_read_info() - read DPCD downstream port info if 
> available
> + * @aux: DisplayPort AUX channel
> + * @dpcd: A cached copy of the port's DPCD
> + * @downstream_ports: buffer to store the downstream port info in
> + *
> + * Returns: 0 if either the downstream port info was read successfully or
> + * there was no downstream info to read, or a negative error code otherwise.
> + */
> +int drm_dp_downstream_read_info(struct drm_dp_aux *aux,
> +   const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +   u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS])
> +{
> +   int ret;
> +   u8 len;
> +
> +   memset(downstream_ports, 0, DP_MAX_DOWNSTREAM_PORTS);
> +
> +   /* No downstream info to read */
> +   if (!drm_dp_is_branch(dpcd) ||
> +   dpcd[DP_DPCD_REV] < DP_DPCD_REV_10 ||
> +   !(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> +   return 0;
> +
> +   len = drm_dp_downstream_port_count(dpcd);
> +   if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE)
> +   len *= 4;
> +
> +   ret = drm_dp_dpcd_read(aux, DP_DOWNSTREAM_PORT_0, downstream_ports, 
> len);
> +   if (ret < 0)
> +   return ret;
> +
> +   return ret == len ? 0 : -EIO;
> +}
> +EXPORT_SYMBOL(drm_dp_downstream_read_info);
> +
>  /**
>   * drm_dp_downstream_max_clock() - extract branch device max
>   * pixel rate for legacy VGA
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 1e29d3a012856..984e49194ca31 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4685,18 +4685,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> return false;
> }
>
> -   if (!drm_dp_is_branch(intel_dp->dpcd))
> -   return true; /* native DP sink */
> -
> -   if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> -   return true; /* no per-port downstream info */
> -
> -   if (drm_dp_dpcd_read(_dp->aux, DP_DOWNSTREAM_PORT_0,
> -intel_dp->downstream_ports,
> -DP_MAX_DOWNSTREAM_PORTS) < 0)
> -   return false; /* downstream port status fetch failed */
> -
> -   return true;
> +   return drm_dp_downstream_read_info(_dp->aux, intel_dp->dpcd,
> +

Re: [Nouveau] [RFC 19/20] drm/i915/dp: Extract drm_dp_read_dpcd_caps()

[Sean Paul]

On Tue, Aug 11, 2020 at 04:04:56PM -0400, Lyude Paul wrote:
> Since DP 1.3, it's been possible for DP receivers to specify an
> additional set of DPCD capabilities, which can take precedence over the
> capabilities reported at DP_DPCD_REV.
> 
> Basically any device supporting DP is going to need to read these in an
> identical manner, in particular nouveau, so let's go ahead and just move
> this code out of i915 into a shared DRM DP helper that we can use in
> other drivers.
> 
> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 76 +
>  drivers/gpu/drm/i915/display/intel_dp.c | 60 +---
>  drivers/gpu/drm/i915/display/intel_dp.h |  1 -
>  drivers/gpu/drm/i915/display/intel_lspcon.c |  2 +-
>  include/drm/drm_dp_helper.h |  3 +
>  5 files changed, 82 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 0ff2959c8f8e8..f9445915c6c26 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -423,6 +423,82 @@ bool drm_dp_send_real_edid_checksum(struct drm_dp_aux 
> *aux,
>  }
>  EXPORT_SYMBOL(drm_dp_send_real_edid_checksum);
>  
> +static int drm_dp_read_extended_dpcd_caps(struct drm_dp_aux *aux,
> +   u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> + u8 dpcd_ext[6];
> + int ret;
> +
> + /*
> +  * Prior to DP1.3 the bit represented by
> +  * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved.
> +  * If it is set DP_DPCD_REV at h could be at a value less than
> +  * the true capability of the panel. The only way to check is to
> +  * then compare h and 2200h.
> +  */
> + if (!(dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
> +   DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT))
> + return 0;
> +
> + ret = drm_dp_dpcd_read(aux, DP_DP13_DPCD_REV, _ext,
> +sizeof(dpcd_ext));
> + if (ret != sizeof(dpcd_ext))
> + return -EIO;
> +
> + if (dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) {
> + DRM_DEBUG_KMS("%s: Extended DPCD rev less than base DPCD rev 
> (%d > %d)\n",
> +   aux->name, dpcd[DP_DPCD_REV],
> +   dpcd_ext[DP_DPCD_REV]);

Might be a good opportunity to convert all of these to drm_dbg_dp()?

> + return 0;
> + }
> +
> + if (!memcmp(dpcd, dpcd_ext, sizeof(dpcd_ext)))
> + return 0;
> +
> + DRM_DEBUG_KMS("%s: Base DPCD: %*ph\n",
> +   aux->name, DP_RECEIVER_CAP_SIZE, dpcd);
> +
> + memcpy(dpcd, dpcd_ext, sizeof(dpcd_ext));
> +
> + return 0;
> +}
> +
> +/**
> + * drm_dp_read_dpcd_caps() - read DPCD caps and extended DPCD caps if
> + * available
> + * @aux: DisplayPort AUX channel
> + * @dpcd: Buffer to store the resulting DPCD in
> + *
> + * Attempts to read the base DPCD caps for @aux. Additionally, this function
> + * checks for and reads the extended DPRX caps (%DP_DP13_DPCD_REV) if
> + * present.
> + *
> + * Returns: %0 if the DPCD was read successfully, negative error code
> + * otherwise.
> + */
> +int drm_dp_read_dpcd_caps(struct drm_dp_aux *aux,
> +   u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> + int ret;
> +
> + ret = drm_dp_dpcd_read(aux, DP_DPCD_REV, dpcd, DP_RECEIVER_CAP_SIZE);
> + if (ret != DP_RECEIVER_CAP_SIZE || dpcd[DP_DPCD_REV] == 0)
> + return -EIO;
> +
> + ret = drm_dp_read_extended_dpcd_caps(aux, dpcd);
> + if (ret < 0)
> + return ret;

I wonder if we should just go with the "regular" dpcd caps we just read in this
case?

Regardless of my nits,

Reviewed-by: Sean Paul 

> +
> + DRM_DEBUG_KMS("%s: DPCD: %*ph\n",
> +   aux->name, DP_RECEIVER_CAP_SIZE, dpcd);
> +
> + if (dpcd[DP_DPCD_REV] == 0)
> + ret = -EIO;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_read_dpcd_caps);
> +
>  /**
>   * drm_dp_downstream_read_info() - read DPCD downstream port info if 
> available
>   * @aux: DisplayPort AUX channel
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index e343965a483df..230aa0360dc61 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4449,62 +4449,6 @@ intel_dp_link_down(struct intel_encoder *encoder,
>   }
>  }
>  
> -static void
> -intel_dp_extended_receiver_capabilities(struct intel_dp *intel_dp)
> -{
> - struct drm

Re: [Nouveau] [RFC 16/20] drm/i915/dp: Extract drm_dp_get_sink_count()

[Sean Paul]

On Tue, Aug 11, 2020 at 04:04:53PM -0400, Lyude Paul wrote:
> And of course, we'll also need to read the sink count from other drivers
> as well if we're checking whether or not it's supported. So, let's
> extract the code for this into another helper.
> 
> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 20 
>  drivers/gpu/drm/i915/display/intel_dp.c | 17 +
>  include/drm/drm_dp_helper.h |  1 +
>  3 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 05bb47e589731..0ff2959c8f8e8 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -722,6 +722,26 @@ bool drm_dp_has_sink_count(struct drm_connector 
> *connector,
>  }
>  EXPORT_SYMBOL(drm_dp_has_sink_count);
>  
> +/**
> + * drm_dp_get_sink_count() - Retrieve the sink count for a given sink
> + * @aux: The DP AUX channel to use
> + *
> + * Returns: The current sink count reported by @aux, or a negative error code
> + * otherwise.
> + */
> +int drm_dp_get_sink_count(struct drm_dp_aux *aux)
> +{
> + u8 count;
> + int ret;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_SINK_COUNT, );
> + if (ret < 1)
> + return -EIO;

This should probably be:
if (ret < 0)
return ret;
else if (ret != 1)
return -EIO;


> +
> + return DP_GET_SINK_COUNT(count);
> +}
> +EXPORT_SYMBOL(drm_dp_get_sink_count);
> +
>  /*
>   * I2C-over-AUX implementation
>   */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 35a4779a442e2..e343965a483df 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4648,6 +4648,8 @@ intel_dp_has_sink_count(struct intel_dp *intel_dp)
>  static bool
>  intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  {
> + int ret;
> +
>   if (!intel_dp_read_dpcd(intel_dp))
>   return false;
>  
> @@ -4664,20 +4666,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   }
>  
>   if (intel_dp_has_sink_count(intel_dp)) {
> - u8 count;
> - ssize_t r;
> -
> - r = drm_dp_dpcd_readb(_dp->aux, DP_SINK_COUNT, );
> - if (r < 1)
> + ret = drm_dp_get_sink_count(_dp->aux);
> + if (ret < 0)
>   return false;
>  
> - /*
> -  * Sink count can change between short pulse hpd hence
> -  * a member variable in intel_dp will track any changes
> -  * between short pulse interrupts.
> -  */

I think you could probably keep this comment and relocate the 
intel_dp->sink_count
assignment back.

With these nits (or something like them),

Reviewed-by: Sean Paul 

> - intel_dp->sink_count = DP_GET_SINK_COUNT(count);
> -
>   /*
>* SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
>* a dongle is present but no display. Unless we require to know
> @@ -4685,6 +4677,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>* downstream port information. So, an early return here saves
>* time from performing other operations which are not required.
>*/
> + intel_dp->sink_count = ret;
>   if (!intel_dp->sink_count)
>   return false;
>   }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index a1413a531eaf4..0c141fc81aaa8 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1635,6 +1635,7 @@ struct drm_dp_desc;
>  bool drm_dp_has_sink_count(struct drm_connector *connector,
>  const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  const struct drm_dp_desc *desc);
> +int drm_dp_get_sink_count(struct drm_dp_aux *aux);
>  
>  void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
>  void drm_dp_aux_init(struct drm_dp_aux *aux);
> -- 
> 2.26.2
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [RFC 15/20] drm/i915/dp: Extract drm_dp_has_sink_count()

[Sean Paul]

On Tue, Aug 11, 2020 at 04:04:52PM -0400, Lyude Paul wrote:
> Since other drivers are also going to need to be aware of the sink count
> in order to do proper dongle detection, we might as well steal i915's
> DP_SINK_COUNT helpers and move them into DRM helpers so that other
> dirvers can use them as well.
> 
> Note that this also starts using intel_dp_has_sink_count() in
> intel_dp_detect_dpcd(), which is a functional change.
> 

Reviewed-by: Sean Paul 

> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 22 ++
>  drivers/gpu/drm/i915/display/intel_dp.c | 21 -
>  include/drm/drm_dp_helper.h |  8 +++-
>  3 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9703b33599c3b..05bb47e589731 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -700,6 +700,28 @@ void drm_dp_set_subconnector_property(struct 
> drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_dp_set_subconnector_property);
>  
> +/**
> + * drm_dp_has_sink_count() - Check whether a given connector has a valid sink
> + * count
> + * @connector: The DRM connector to check
> + * @dpcd: A cached copy of the connector's DPCD RX capabilities
> + * @desc: A cached copy of the connector's DP descriptor
> + *
> + * Returns: %True if the (e)DP connector has a valid sink count that should
> + * be probed, %false otherwise.
> + */
> +bool drm_dp_has_sink_count(struct drm_connector *connector,
> +const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +const struct drm_dp_desc *desc)
> +{
> + /* Some eDP panels don't set a valid value for the sink count */
> + return connector->connector_type != DRM_MODE_CONNECTOR_eDP &&
> + dpcd[DP_DPCD_REV] >= DP_DPCD_REV_11 &&
> + dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT &&
> + !drm_dp_has_quirk(desc, 0, DP_DPCD_QUIRK_NO_SINK_COUNT);
> +}
> +EXPORT_SYMBOL(drm_dp_has_sink_count);
> +
>  /*
>   * I2C-over-AUX implementation
>   */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 984e49194ca31..35a4779a442e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4634,6 +4634,16 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>   return true;
>  }
>  
> +static bool
> +intel_dp_has_sink_count(struct intel_dp *intel_dp)
> +{
> + if (!intel_dp->attached_connector)
> + return false;
> +
> + return drm_dp_has_sink_count(_dp->attached_connector->base,
> +  intel_dp->dpcd,
> +  _dp->desc);
> +}
>  
>  static bool
>  intel_dp_get_dpcd(struct intel_dp *intel_dp)
> @@ -4653,13 +4663,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   intel_dp_set_common_rates(intel_dp);
>   }
>  
> - /*
> -  * Some eDP panels do not set a valid value for sink count, that is why
> -  * it don't care about read it here and in intel_edp_init_dpcd().
> -  */
> - if (!intel_dp_is_edp(intel_dp) &&
> - !drm_dp_has_quirk(_dp->desc, 0,
> -   DP_DPCD_QUIRK_NO_SINK_COUNT)) {
> + if (intel_dp_has_sink_count(intel_dp)) {
>   u8 count;
>   ssize_t r;
>  
> @@ -5939,9 +5943,8 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>   return connector_status_connected;
>  
>   /* If we're HPD-aware, SINK_COUNT changes dynamically */
> - if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> + if (intel_dp_has_sink_count(intel_dp) &&
>   intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> -
>   return intel_dp->sink_count ?
>   connector_status_connected : connector_status_disconnected;
>   }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 1349f16564ace..a1413a531eaf4 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1631,6 +1631,11 @@ void drm_dp_set_subconnector_property(struct 
> drm_connector *connector,
> const u8 *dpcd,
> const u8 port_cap[4]);
>  
> +struct drm_dp_desc;
> +bool drm_dp_has_sink_count(struct drm_connector *connector,
> +const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +const struct drm_dp_desc *desc);
> +
>  

Re: [Nouveau] [RFC 13/20] drm/i915/dp: Extract drm_dp_downstream_read_info()

[Sean Paul]

_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>   const u8 port_cap[4]);
>  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> -- 
> 2.26.2
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [RFC 09/20] drm/i915/dp: Extract drm_dp_has_mst()

[Sean Paul]

On Tue, Aug 11, 2020 at 04:04:46PM -0400, Lyude Paul wrote:
> Just a tiny drive-by cleanup, we can consolidate i915's code for
> checking for MST support into a helper to be shared across drivers.
> 

Reviewed-by: Sean Paul 

> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 18 ++
>  include/drm/drm_dp_mst_helper.h | 22 ++
>  2 files changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 79c27f91f42c0..1e29d3a012856 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4699,20 +4699,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   return true;
>  }
>  
> -static bool
> -intel_dp_sink_can_mst(struct intel_dp *intel_dp)
> -{
> - u8 mstm_cap;
> -
> - if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
> - return false;
> -
> - if (drm_dp_dpcd_readb(_dp->aux, DP_MSTM_CAP, _cap) != 1)
> - return false;
> -
> - return mstm_cap & DP_MST_CAP;
> -}
> -
>  static bool
>  intel_dp_can_mst(struct intel_dp *intel_dp)
>  {
> @@ -4720,7 +4706,7 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
>  
>   return i915->params.enable_dp_mst &&
>   intel_dp->can_mst &&
> - intel_dp_sink_can_mst(intel_dp);
> + drm_dp_has_mst(_dp->aux, intel_dp->dpcd);
>  }
>  
>  static void
> @@ -4729,7 +4715,7 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>   struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>   struct intel_encoder *encoder =
>   _to_dig_port(intel_dp)->base;
> - bool sink_can_mst = intel_dp_sink_can_mst(intel_dp);
> + bool sink_can_mst = drm_dp_has_mst(_dp->aux, intel_dp->dpcd);
>  
>   drm_dbg_kms(>drm,
>   "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: 
> %s\n",
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 8b9eb4db3381c..2d8983a713e8c 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -911,4 +911,26 @@ __drm_dp_mst_state_iter_get(struct drm_atomic_state 
> *state,
>   for ((__i) = 0; (__i) < (__state)->num_private_objs; (__i)++) \
>   for_each_if(__drm_dp_mst_state_iter_get((__state), &(mgr), 
> NULL, &(new_state), (__i)))
>  
> +/**
> + * drm_dp_has_mst() - check whether or not a sink supports MST
> + * @aux: The DP AUX channel to use
> + * @dpcd: A cached copy of the DPCD capabilities for this sink
> + *
> + * Returns: %True if the sink supports MST, %false otherwise
> + */
> +static inline bool
> +drm_dp_has_mst(struct drm_dp_aux *aux,
> +const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> + u8 mstm_cap;
> +
> + if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_12)
> + return false;
> +
> + if (drm_dp_dpcd_readb(aux, DP_MSTM_CAP, _cap) != 1)
> + return false;
> +
> + return !!(mstm_cap & DP_MST_CAP);
> +}
> +
>  #endif
> -- 
> 2.26.2
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH v5 08/13] drm/nouveau: Change debug checks to specifically target syslog

[Sean Paul]

From: Sean Paul 

Since the logs protected by these checks specifically target syslog,
use the new drm_debug_syslog_enabled() call to avoid triggering
these prints when only trace is enabled.

Signed-off-by: Sean Paul 

Changes in v5:
-Added to the set
---
 drivers/gpu/drm/nouveau/dispnv50/disp.h | 4 ++--
 drivers/gpu/drm/nouveau/nouveau_drv.h   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.h 
b/drivers/gpu/drm/nouveau/dispnv50/disp.h
index 696e70a6b98b..d60602db2cf0 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.h
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.h
@@ -85,14 +85,14 @@ extern const u64 wndwc57e_modifiers[];
 
 #define evo_mthd(p, m, s) do { \
const u32 _m = (m), _s = (s);   \
-   if (drm_debug_enabled(DRM_UT_KMS))  \
+   if (drm_debug_syslog_enabled(DRM_UT_KMS))   
\
pr_err("%04x %d %s\n", _m, _s, __func__);   \
*((p)++) = ((_s << 18) | _m);   \
 } while(0)
 
 #define evo_data(p, d) do {\
const u32 _d = (d); \
-   if (drm_debug_enabled(DRM_UT_KMS))  \
+   if (drm_debug_syslog_enabled(DRM_UT_KMS))   
\
pr_err("\t%08x\n", _d); \
*((p)++) = _d;  \
 } while(0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h 
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 2a6519737800..b916d1f456cd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -257,11 +257,11 @@ void nouveau_drm_device_remove(struct drm_device *dev);
 #define NV_INFO(drm,f,a...) NV_PRINTK(info, &(drm)->client, f, ##a)
 
 #define NV_DEBUG(drm,f,a...) do {  
\
-   if (drm_debug_enabled(DRM_UT_DRIVER))  \
+   if (drm_debug_syslog_enabled(DRM_UT_DRIVER))
  \
NV_PRINTK(info, &(drm)->client, f, ##a);   \
 } while(0)
 #define NV_ATOMIC(drm,f,a...) do { 
\
-   if (drm_debug_enabled(DRM_UT_ATOMIC))  \
+   if (drm_debug_syslog_enabled(DRM_UT_ATOMIC))
  \
NV_PRINTK(info, &(drm)->client, f, ##a);   \
 } while(0)
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v5 06/14] drm/dp_mst: Protect drm_dp_mst_port members with locking

[Sean Paul]

s of drm_dp_mst_port other than port->connector, we
> simply grab >base.lock in drm_dp_mst_link_probe_work() for already
> registered ports, update said members and drop the lock before
> potentially registering a connector and probing the link address of it's
> children.
> 
> Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock
> acquisition context in order to acquire >base.lock under
> _mutex and convert all it's users over to using the
> .detect_ctx probe hooks.
> 
> With that, we finally have well defined locking.
> 
> Changes since v4:
> * Get rid of port->mutex, stop using connection_mutex and just use our own
>   modesetting lock - mgr->base.lock. Also, add a probe_lock that comes
>   before this patch.
> * Just throw out ports that get changed from an output to an input, and
>   replace them with new ports. This lets us ensure that modesetting
>   contexts never see port->connector go from having a connector to being
>   NULL.
> * Write an extremely detailed explanation of what problems this is
>   trying to fix, since there's a _lot_ of context here and I honestly
>   forgot some of it myself a couple times.
> * Don't grab mgr->lock when reading port->mstb in
>   drm_dp_mst_handle_link_address_port(). It's not needed.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Cc: Sean Paul 
> Signed-off-by: Lyude Paul 

Overall makes sense to me. Thanks for the comprehensive commit message and
comments, they definitely help :)

Just one nit below,

Reviewed-by: Sean Paul 


> ---
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  28 +--
>  drivers/gpu/drm/drm_dp_mst_topology.c | 230 --
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  28 ++-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  32 +--
>  drivers/gpu/drm/radeon/radeon_dp_mst.c|  24 +-
>  include/drm/drm_dp_mst_helper.h   |  38 ++-
>  6 files changed, 240 insertions(+), 140 deletions(-)
> 

/snip

> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 11d842f0bff5..7bf4db91ff90 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c

/snip

> @@ -1912,35 +1984,40 @@ drm_dp_mst_handle_link_address_port(struct 
> drm_dp_mst_branch *mstb,
>  {
>   struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
>   struct drm_dp_mst_port *port;
> - bool created = false;
> - int old_ddps = 0;
> + int old_ddps = 0, ret;
> + u8 new_pdt = DP_PEER_DEVICE_NONE;
> + bool created = false, send_link_addr = false;
>  
>   port = drm_dp_get_port(mstb, port_msg->port_number);
>   if (!port) {
> - port = kzalloc(sizeof(*port), GFP_KERNEL);
> + port = drm_dp_mst_add_port(dev, mgr, mstb,
> +port_msg->port_number);
>   if (!port)
>   return;
> - kref_init(>topology_kref);
> - kref_init(>malloc_kref);
> - port->parent = mstb;
> - port->port_num = port_msg->port_number;
> - port->mgr = mgr;
> - port->aux.name = "DPMST";
> - port->aux.dev = dev->dev;
> - port->aux.is_remote = true;
> -
> - /*
> -  * Make sure the memory allocation for our parent branch stays
> -  * around until our own memory allocation is released
> + created = true;
> + } else if (port_msg->input_port && !port->input && port->connector) {
> + /* Destroying the connector is impossible in this context, so
> +  * replace the port with a new one
>*/
> - drm_dp_mst_get_mstb_malloc(mstb);
> + drm_dp_mst_topology_unlink_port(mgr, port);
> + drm_dp_mst_topology_put_port(port);
>  
> + port = drm_dp_mst_add_port(dev, mgr, mstb,
> +port_msg->port_number);
> + if (!port)
> + return;
>   created = true;
>   } else {
> + /* Locking is only needed when the port has a connector
> +  * exposed to userspace
> +  */
> + drm_modeset_lock(>base.lock, NULL);

Random musing: It's kind of unfortunate that we don't have a void varient of
drm_modeset_lock for when there's no acquire_ctx since we end up with a mix of
drm_modeset_lock calls with and without return checking. 

/snip

> @@ -3441,22 +3516,31 @@ EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
> 

Re: [Nouveau] [PATCH v2 24/27] drm/amdgpu/dm: Resume short HPD IRQs before resuming MST topology

[Sean Paul]

On Tue, Sep 03, 2019 at 04:46:02PM -0400, Lyude Paul wrote:
> Since we're going to be reprobing the entire topology state on resume
> now using sideband transactions, we need to ensure that we actually have
> short HPD irqs enabled before calling drm_dp_mst_topology_mgr_resume().
> So, do that.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 73630e2940d4..4d3c8bff77da 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1185,15 +1185,15 @@ static int dm_resume(void *handle)
>   /* program HPD filter */
>   dc_resume(dm->dc);
>  
> - /* On resume we need to  rewrite the MSTM control bits to enamble MST*/
> - s3_handle_mst(ddev, false);
> -
>   /*
>* early enable HPD Rx IRQ, should be done before set mode as short
>* pulse interrupts are used for MST
>*/
>   amdgpu_dm_irq_resume_early(adev);
>  
> + /* On resume we need to  rewrite the MSTM control bits to enamble MST*/

While we're here,

s/  / / && s/enamble/enable/ && s_*/_ */_

> + s3_handle_mst(ddev, false);
> +
>   /* Do detection*/
>   drm_connector_list_iter_begin(ddev, );
>   drm_for_each_connector_iter(connector, ) {
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 25/27] drm/dp_mst: Add basic topology reprobing when resuming

[Sean Paul]

;dpcd, 
> DP_RECEIVER_CAP_SIZE);
> - if (sret != DP_RECEIVER_CAP_SIZE) {
> - DRM_DEBUG_KMS("dpcd read failed - undocked during 
> suspend?\n");
> - ret = -1;
> - goto out_unlock;
> - }
> + ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
> +  DP_MST_EN |
> +  DP_UP_REQ_EN |
> +  DP_UPSTREAM_IS_SRC);
> + if (ret < 0) {
> + DRM_DEBUG_KMS("mst write failed - undocked during suspend?\n");
> + goto out_fail;
> + }
>  
> - ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
> -  DP_MST_EN | DP_UP_REQ_EN | 
> DP_UPSTREAM_IS_SRC);
> - if (ret < 0) {
> - DRM_DEBUG_KMS("mst write failed - undocked during 
> suspend?\n");
> - ret = -1;
> - goto out_unlock;
> - }
> + /* Some hubs forget their guids after they resume */
> + ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
> + if (ret != 16) {
> + DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n");
> + goto out_fail;
> + }
> + drm_dp_check_mstb_guid(mgr->mst_primary, guid);
>  
> - /* Some hubs forget their guids after they resume */
> - sret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
> - if (sret != 16) {
> - DRM_DEBUG_KMS("dpcd read failed - undocked during 
> suspend?\n");
> - ret = -1;
> - goto out_unlock;
> - }
> - drm_dp_check_mstb_guid(mgr->mst_primary, guid);
> + /* For the final step of resuming the topology, we need to bring the
> +  * state of our in-memory topology back into sync with reality. So,
> +  * restart the probing process as if we're probing a new hub
> +  */
> + queue_work(system_long_wq, >work);
> + mutex_unlock(>lock);
>  
> - ret = 0;
> - } else
> - ret = -1;
> + if (sync) {
> + DRM_DEBUG_KMS("Waiting for link probe work to finish re-syncing 
> topology...\n");
> + flush_work(>work);
> + }

It took me longer than I'd like to admit to realize that most of the diff in
this function is just removing the indent. Would you mind splitting that out
into a separate patch so the reprobe change is more obvious?

With these nits fixed,

Reviewed-by: Sean Paul 


>  
> -out_unlock:
> + return 0;
> +
> +out_fail:
>   mutex_unlock(>lock);
> - return ret;
> + return -1;
>  }
>  EXPORT_SYMBOL(drm_dp_mst_topology_mgr_resume);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5673ed75e428..b78364dcdef9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7400,7 +7400,8 @@ void intel_dp_mst_resume(struct drm_i915_private 
> *dev_priv)
>   if (!intel_dp->can_mst)
>   continue;
>  
> - ret = drm_dp_mst_topology_mgr_resume(_dp->mst_mgr);
> + ret = drm_dp_mst_topology_mgr_resume(_dp->mst_mgr,
> +  true);
>   if (ret) {
>   intel_dp->is_mst = false;
>   drm_dp_mst_topology_mgr_set_mst(_dp->mst_mgr,
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c 
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 307584107d77..e459e2a79d78 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1309,14 +1309,14 @@ nv50_mstm_fini(struct nv50_mstm *mstm)
>  }
>  
>  static void
> -nv50_mstm_init(struct nv50_mstm *mstm)
> +nv50_mstm_init(struct nv50_mstm *mstm, bool runtime)
>  {
>   int ret;
>  
>   if (!mstm || !mstm->mgr.mst_state)
>   return;
>  
> - ret = drm_dp_mst_topology_mgr_resume(>mgr);
> + ret = drm_dp_mst_topology_mgr_resume(>mgr, !runtime);
>   if (ret == -1) {
>   drm_dp_mst_topology_mgr_set_mst(>mgr, false);
>   drm_kms_helper_hotplug_event(mstm->mgr.dev);
> @@ -2262,7 +2262,7 @@ nv50_display_init(struct drm_device *dev, bool resume, 
> bool runtime)
>   if (encoder->encoder_type != DRM_MODE_ENCODER_DPMST) {
>   struct nouveau_encoder *nv_encoder =
>   nouveau_encoder(encoder);
> - nv50_mstm_init(nv_encoder->dp.mstm);
> + nv50_mstm_init(nv_encoder->dp.mstm, runtime);
>   }
>   }
>  
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 1efbb086f7ac..1bdee5ee6dcd 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -685,7 +685,8 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
>  
>  void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
>  int __must_check
> -drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> +drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
> +bool sync);
>  
>  ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
>unsigned int offset, void *buffer, size_t size);
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v5 07/14] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()

[Sean Paul]

On Mon, Oct 21, 2019 at 10:36:02PM -0400, Lyude Paul wrote:
> This probably hasn't caused any problems up until now since it's
> probably nearly impossible to encounter this in the wild, however if we
> were to receive a connection status notification from the MST hub after
> resume while we're in the middle of reprobing the link addresses for a
> topology then there's a much larger chance that a port could have
> changed from being an output port to input port (or vice versa). If we
> forget to update this bit of information, we'll potentially ignore a
> valid PDT change on a downstream port because we think it's an input
> port.
> 
> So, make sure we read the input_port field in connection status
> notifications in drm_dp_mst_handle_conn_stat() to prevent this from
> happening once we've implemented suspend/resume reprobing.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 
> Reviewed-by: Sean Paul 
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 52 +++
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 7bf4db91ff90..c8e218b902ae 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2079,18 +2079,40 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch 
> *mstb,
>  {
>   struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
>   struct drm_dp_mst_port *port;
> - int old_ddps;
> - bool dowork = false;
> + int old_ddps, ret;
> + u8 new_pdt;
> + bool dowork = false, create_connector = false;
>  
>   port = drm_dp_get_port(mstb, conn_stat->port_number);
>   if (!port)
>   return;
>  
> - /* Locking is only needed if the port's exposed to userspace */
> - if (port->connector)
> + if (port->connector) {
> + if (!port->input && conn_stat->input_port) {
> + /*
> +  * We can't remove a connector from an already exposed
> +  * port, so just throw the port out and make sure we
> +  * reprobe the link address of it's parent MSTB
> +  */
> + drm_dp_mst_topology_unlink_port(mgr, port);
> + mstb->link_address_sent = false;
> + dowork = true;
> + goto out;
> + }
> +
> + /*
> +  * Locking is only needed if the port's exposed to userspace
> +  */

optional nit: this will still fit on one line

>   drm_modeset_lock(>base.lock, NULL);
> + } else if (port->input && !conn_stat->input_port) {
> + create_connector = true;
> + /* Reprobe link address so we get num_sdp_streams */
> + mstb->link_address_sent = false;
> + dowork = true;
> + }
>  
>   old_ddps = port->ddps;
> + port->input = conn_stat->input_port;
>   port->mcs = conn_stat->message_capability_status;
>   port->ldps = conn_stat->legacy_device_plug_status;
>   port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2103,21 +2125,23 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch 
> *mstb,
>   }
>   }
>  
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> -   conn_stat->peer_device_type);
> - if (ret == 1) {
> - dowork = true;
> - } else if (ret < 0) {
> - DRM_ERROR("Failed to change PDT for port %p: %d\n",
> -   port, ret);
> - dowork = false;
> - }
> + new_pdt = port->input ? DP_PEER_DEVICE_NONE : 
> conn_stat->peer_device_type;
> +
> + ret = drm_dp_port_set_pdt(port, new_pdt);
> + if (ret == 1) {
> + dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> +   port, ret);
> + dowork = false;
>   }
>  
>   if (port->connector)
>   drm_modeset_unlock(>base.lock);
> + else if (create_connector)
> + drm_dp_mst_port_add_connector(mstb, port);
>  
> +out:
>   drm_dp_mst_topology_put_port(port);
>   if (dowork)
>   queue_work(system_long_wq, >mgr->work);
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v5 05/14] drm/dp_mst: Add probe_lock

[Sean Paul]

On Mon, Oct 21, 2019 at 10:36:00PM -0400, Lyude Paul wrote:
> Currently, MST lacks locking in a lot of places that really should have
> some sort of locking. Hotplugging and link address code paths are some
> of the offenders here, as there is actually nothing preventing us from
> running a link address probe while at the same time handling a
> connection status update request - something that's likely always been
> possible but never seen in the wild because hotplugging has been broken
> for ages now (with the exception of amdgpu, for reasons I don't think
> are worth digging into very far).
> 
> Note: I'm going to start using the term "in-memory topology layout" here
> to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
> 
> Locking in these places is a little tougher then it looks though.
> Generally we protect anything having to do with the in-memory topology
> layout under >lock. But this becomes nearly impossible to do from
> the context of link address probes due to the fact that >lock is
> usually grabbed under random various modesetting locks, meaning that
> there's no way we can just invert the >lock order and keep it
> locked throughout the whole process of updating the topology.
> 
> Luckily there are only two workers which can modify the in-memory
> topology layout: drm_dp_mst_up_req_work() and
> drm_dp_mst_link_probe_work(), meaning as long as we prevent these two
> workers from traveling the topology layout in parallel with the intent
> of updating it we don't need to worry about grabbing >lock in these
> workers for reads. We only need to grab >lock in these workers for
> writes, so that readers outside these two workers are still protected
> from the topology layout changing beneath them.
> 
> So, add the new >probe_lock and use it in both
> drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally,
> add some more detailed explanations for how this locking is intended to
> work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.

This seems like a good solution to me, thanks for working this through!

Any chance we could add a WARN_ON(!mutex_is_locked(>probe_lock)); somewhere
centrally called by all paths modifying the in-memory topology layout?
drm_dp_add_port perhaps? That way we have a fallback in case something else
starts mucking with the topology.

Other than that,

Reviewed-by: Sean Paul 


> 
> Signed-off-by: Lyude Paul 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Cc: Sean Paul 
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++-
>  include/drm/drm_dp_mst_helper.h   | 32 +++
>  2 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 08c316a727df..11d842f0bff5 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct 
> drm_dp_mst_topology_mgr *m
>  struct drm_dp_mst_branch *mstb)
>  {
>   struct drm_dp_mst_port *port;
> - struct drm_dp_mst_branch *mstb_child;
> +
>   if (!mstb->link_address_sent)
>   drm_dp_send_link_address(mgr, mstb);
>  
>   list_for_each_entry(port, >ports, next) {
> - if (port->input)
> - continue;
> + struct drm_dp_mst_branch *mstb_child = NULL;
>  
> - if (!port->ddps)
> + if (port->input || !port->ddps)
>   continue;
>  
>   if (!port->available_pbn)
>   drm_dp_send_enum_path_resources(mgr, mstb, port);
>  
> - if (port->mstb) {
> + if (port->mstb)
>   mstb_child = drm_dp_mst_topology_get_mstb_validated(
>   mgr, port->mstb);
> - if (mstb_child) {
> - drm_dp_check_and_send_link_address(mgr, 
> mstb_child);
> - drm_dp_mst_topology_put_mstb(mstb_child);
> - }
> +
> + if (mstb_child) {
> + drm_dp_check_and_send_link_address(mgr, mstb_child);
> + drm_dp_mst_topology_put_mstb(mstb_child);
>   }
>   }
>  }
>  
>  static void drm_dp_mst_link_probe_work(struct work_struct *work)
>  {
> - struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct 
> drm_dp_mst_topology_mgr, work);
> + struct drm_dp_mst_topology_mgr *mgr =
> +

Re: [Nouveau] [PATCH v2 27/27] drm/dp_mst: Add topology ref history tracking for debugging

[Sean Paul]

On Tue, Sep 03, 2019 at 04:46:05PM -0400, Lyude Paul wrote:
> For very subtle mistakes with topology refs, it can be rather difficult
> to trace them down with the debugging info that we already have. I had
> one such issue recently while trying to implement suspend/resume
> reprobing for MST, and ended up coming up with this.
> 
> Inspired by Chris Wilson's wakeref tracking for i915, this adds a very
> similar feature to the DP MST helpers, which allows for partial tracking
> of topology refs for both ports and branch devices. This is a lot less
> advanced then wakeref tracking: we merely keep a count of all of the
> spots where a topology ref has been grabbed or dropped, then dump out
> that history in chronological order when a port or branch device's
> topology refcount reaches 0. So far, I've found this incredibly useful
> for debugging topology refcount errors.
> 
> Since this has the potential to be somewhat slow and loud, we add an
> expert kernel config option to enable or disable this feature,
> CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS.
> 

Looks very useful indeed! 

My only nit is that we could probably grow the list a little more aggressively
(or start it off at some size > 1) and avoid a bunch of reallocs. That said,
I'm not sure how often it's reallocated so it might not be an issue. Either
way, 

Reviewed-by: Sean Paul 


> Changes since v1:
> * Don't forget to destroy topology_ref_history_lock
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/Kconfig   |  14 ++
>  drivers/gpu/drm/drm_dp_mst_topology.c | 233 +-
>  include/drm/drm_dp_mst_helper.h   |  45 +
>  3 files changed, 288 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e67c194c2aca..44fc2c2a6e2c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -93,6 +93,20 @@ config DRM_KMS_FB_HELPER
>   help
> FBDEV helpers for KMS drivers.
>  
> +config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
> +bool "Enable refcount backtrace history in the DP MST helpers"
> +select STACKDEPOT
> +depends on DRM_KMS_HELPER
> +depends on DEBUG_KERNEL
> +depends on EXPERT
> +help
> +  Enables debug tracing for topology refs in DRM's DP MST helpers. A
> +  history of each topology reference/dereference will be printed to 
> the
> +  kernel log once a port or branch device's topology refcount 
> reaches 0.
> +
> +  This has the potential to use a lot of memory and print some very
> +  large kernel messages. If in doubt, say "N".
> +
>  config DRM_FBDEV_EMULATION
>   bool "Enable legacy fbdev support for your modesetting driver"
>   depends on DRM
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 5b5c0b3b3c0e..18f9a02927d9 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -28,6 +28,13 @@
>  #include 
>  #include 
>  
> +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> +#include 
> +#include 
> +#include 
> +#include 
> +#endif
> +
>  #include 
>  #include 
>  #include 
> @@ -1405,12 +1412,189 @@ drm_dp_mst_put_port_malloc(struct drm_dp_mst_port 
> *port)
>  }
>  EXPORT_SYMBOL(drm_dp_mst_put_port_malloc);
>  
> +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> +
> +#define STACK_DEPTH 8
> +
> +static noinline void
> +__topology_ref_save(struct drm_dp_mst_topology_mgr *mgr,
> + struct drm_dp_mst_topology_ref_history *history,
> + enum drm_dp_mst_topology_ref_type type)
> +{
> + struct drm_dp_mst_topology_ref_entry *entry = NULL;
> + depot_stack_handle_t backtrace;
> + ulong stack_entries[STACK_DEPTH];
> + uint n;
> + int i;
> +
> + n = stack_trace_save(stack_entries, ARRAY_SIZE(stack_entries), 1);
> + backtrace = stack_depot_save(stack_entries, n, GFP_KERNEL);
> + if (!backtrace)
> + goto fail_alloc;
> +
> + /* Try to find an existing entry for this backtrace */
> + for (i = 0; i < history->len; i++) {
> + if (history->entries[i].backtrace == backtrace) {
> + entry = >entries[i];
> + break;
> + }
> + }
> +
> + /* Otherwise add one */
> + if (!entry) {
> + struct drm_dp_mst_topology_ref_entry *new;
> + int new_len = history->len + 1;
> +
> +  

Re: [Nouveau] [PATCH v2 26/27] drm/dp_mst: Also print unhashed pointers for malloc/topology references

[Sean Paul]

gt; *port)
>   int ret = kref_get_unless_zero(>topology_kref);
>  
>   if (ret)
> - DRM_DEBUG("port %p (%d)\n", port,
> -   kref_read(>topology_kref));
> + DRM_DEBUG("port %p/%px (%d)\n",
> +   port, port, kref_read(>topology_kref));
>  
>   return ret;
>  }
> @@ -1569,7 +1574,8 @@ static void drm_dp_mst_topology_get_port(struct 
> drm_dp_mst_port *port)
>  {
>   WARN_ON(kref_read(>topology_kref) == 0);
>   kref_get(>topology_kref);
> - DRM_DEBUG("port %p (%d)\n", port, kref_read(>topology_kref));
> + DRM_DEBUG("port %p/%px (%d)\n",
> +   port, port, kref_read(>topology_kref));
>  }
>  
>  /**
> @@ -1585,8 +1591,8 @@ static void drm_dp_mst_topology_get_port(struct 
> drm_dp_mst_port *port)
>   */
>  static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port)
>  {
> - DRM_DEBUG("port %p (%d)\n",
> -   port, kref_read(>topology_kref) - 1);
> + DRM_DEBUG("port %p/%px (%d)\n",
> +   port, port, kref_read(>topology_kref) - 1);
>   kref_put(>topology_kref, drm_dp_destroy_port);
>  }
>  
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously

[Sean Paul]

On Wed, Sep 25, 2019 at 04:08:22PM -0400, Lyude Paul wrote:
> On Wed, 2019-09-25 at 14:16 -0400, Sean Paul wrote:
> > On Tue, Sep 03, 2019 at 04:45:41PM -0400, Lyude Paul wrote:
> > > When reprobing an MST topology during resume, we have to account for the
> > > fact that while we were suspended it's possible that mstbs may have been
> > > removed from any ports in the topology. Since iterating downwards in the
> > > topology requires that we hold >lock, destroying MSTBs from this
> > > context would result in attempting to lock >lock a second time and
> > > deadlocking.
> > > 
> > > So, fix this by first moving destruction of MSTBs into
> > > destroy_connector_work, then rename destroy_connector_work and friends
> > > to reflect that they now destroy both ports and mstbs.
> > > 
> > > Changes since v1:
> > > * s/destroy_connector_list/destroy_port_list/
> > >   s/connector_destroy_lock/delayed_destroy_lock/
> > >   s/connector_destroy_work/delayed_destroy_work/
> > >   s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/
> > >   s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/
> > >   - danvet
> > > * Use two loops in drm_dp_delayed_destroy_work() - danvet
> > > * Better explain why we need to do this - danvet
> > > * Use cancel_work_sync() instead of flush_work() - flush_work() doesn't
> > >   account for work requeing
> > > 
> > > Cc: Juston Li 
> > > Cc: Imre Deak 
> > > Cc: Ville Syrjälä 
> > > Cc: Harry Wentland 
> > > Cc: Daniel Vetter 
> > > Signed-off-by: Lyude Paul 
> > 
> > Took me a while to grok this, and I'm still not 100% confident my mental
> > model
> > is correct, so please bear with me while I ask silly questions :)
> > 
> > Now that the destroy is delayed, and the port remains in the topology, is it
> > possible we will underflow the topology kref by calling put_mstb multiple
> > times?
> > It looks like that would result in a WARN from refcount.c, and wouldn't call
> > the
> > destroy function multiple times, so that's nice :)
> > 
> > Similarly, is there any defense against calling get_mstb() between destroy()
> > and
> > the delayed destroy worker running?
> > 
> Good question! There's only one instance where we unconditionally grab an MSTB
> ref, drm_dp_mst_topology_mgr_set_mst(), and in that location we're guaranteed
> to be the only one with access to that mstb until we drop >lock.
> Everywhere else we use drm_dp_mst_topology_try_get_mstb(), which uses
> kref_get_unless_zero() to protect against that kind of situation (and forces
> the caller to check with __must_check)

Awesome, thanks for the breakdown!


Reviewed-by: Sean Paul 


> 
> > Sean
> > 
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 162 +-
> > >  include/drm/drm_dp_mst_helper.h   |  26 +++--
> > >  2 files changed, 127 insertions(+), 61 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 3054ec622506..738f260d4b15 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1113,34 +1113,17 @@ static void
> > > drm_dp_destroy_mst_branch_device(struct kref *kref)
> > >   struct drm_dp_mst_branch *mstb =
> > >   container_of(kref, struct drm_dp_mst_branch, topology_kref);
> > >   struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > > - struct drm_dp_mst_port *port, *tmp;
> > > - bool wake_tx = false;
> > >  
> > > - mutex_lock(>lock);
> > > - list_for_each_entry_safe(port, tmp, >ports, next) {
> > > - list_del(>next);
> > > - drm_dp_mst_topology_put_port(port);
> > > - }
> > > - mutex_unlock(>lock);
> > > -
> > > - /* drop any tx slots msg */
> > > - mutex_lock(>mgr->qlock);
> > > - if (mstb->tx_slots[0]) {
> > > - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> > > - mstb->tx_slots[0] = NULL;
> > > - wake_tx = true;
> > > - }
> > > - if (mstb->tx_slots[1]) {
> > > - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> > > - mstb->tx_slots[1] = NULL;
> > > - wake_tx = true;
> > > - }
> > > - mutex_unlock(>mgr->qlock);
> > >

Re: [Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking

[Sean Paul]

On Wed, Sep 25, 2019 at 05:00:00PM -0400, Lyude Paul wrote:
> On Wed, 2019-09-25 at 15:27 -0400, Sean Paul wrote:
> > On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> > > Since we're going to be implementing suspend/resume reprobing very soon,
> > > we need to make sure we are extra careful to ensure that our locking
> > > actually protects the topology state where we expect it to. Turns out
> > > this isn't the case with drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt(), both of which change port->mstb without
> > > grabbing >lock.
> > > 
> > > Additionally, since most callers of these functions are just using it to
> > > teardown the port's previous PDT and setup a new one we can simplify
> > > things a bit and combine drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt() into a single function:
> > > drm_dp_port_set_pdt(). This function also handles actually ensuring that
> > > we grab the correct locks when we need to modify port->mstb.
> > > 
> > > Cc: Juston Li 
> > > Cc: Imre Deak 
> > > Cc: Ville Syrjälä 
> > > Cc: Harry Wentland 
> > > Cc: Daniel Vetter 
> > > Signed-off-by: Lyude Paul 
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
> > >  include/drm/drm_dp_mst_helper.h   |   6 +-
> > >  2 files changed, 110 insertions(+), 77 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index d1610434a0cb..9944ef2ce885 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
> > > drm_dp_mst_branch *mstb)
> > >   kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
> > >  }
> > >  
> > > -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> > > old_pdt)
> > > -{
> > > - struct drm_dp_mst_branch *mstb;
> > > -
> > > - switch (old_pdt) {
> > > - case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > - case DP_PEER_DEVICE_SST_SINK:
> > > - /* remove i2c over sideband */
> > > - drm_dp_mst_unregister_i2c_bus(>aux);
> > > - break;
> > > - case DP_PEER_DEVICE_MST_BRANCHING:
> > > - mstb = port->mstb;
> > > - port->mstb = NULL;
> > > - drm_dp_mst_topology_put_mstb(mstb);
> > > - break;
> > > - }
> > > -}
> > > -
> > >  static void drm_dp_destroy_port(struct kref *kref)
> > >  {
> > >   struct drm_dp_mst_port *port =
> > > @@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct
> > > drm_dp_mst_port *port,
> > >   return parent_lct + 1;
> > >  }
> > >  
> > > -/*
> > > - * return sends link address for new mstb
> > > - */
> > > -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> > > +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> > >  {
> > > - int ret;
> > > - u8 rad[6], lct;
> > > - bool send_link = false;
> > > + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > > + struct drm_dp_mst_branch *mstb;
> > > + u8 rad[8], lct;
> > > + int ret = 0;
> > > +
> > > + if (port->pdt == new_pdt)
> > 
> > Shouldn't we also ensure that access to port->pdt is also locked?
> > 
> 
> It's specifically port->mstb that needs to be protected under lock. We don't
> use port->pdt for traversing the topology at all, so keeping it under
> connection_mutex is sufficient.
> 

I hadn't gotten to the connection_mutex patch yet when I made that comment :)

Reviewed-by: Sean Paul 


> > Sean
> > 
> > > + return 0;
> > > +
> > > + /* Teardown the old pdt, if there is one */
> > > + switch (port->pdt) {
> > > + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > + case DP_PEER_DEVICE_SST_SINK:
> > > + /*
> > > +  * If the new PDT would also have an i2c bus, don't bother
> > > +  * with reregistering it
> > > +  */
> > > + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > > + new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > > + port->pdt = new_pdt;
> > > + return 0;
> > > + 

Re: [Nouveau] [PATCH v2 22/27] drm/nouveau: Don't grab runtime PM refs for HPD IRQs

[Sean Paul]

On Tue, Sep 03, 2019 at 04:46:00PM -0400, Lyude Paul wrote:
> In order for suspend/resume reprobing to work, we need to be able to
> perform sideband communications during suspend/resume, along with
> runtime PM suspend/resume. In order to do so, we also need to make sure
> that nouveau doesn't bother grabbing a runtime PM reference to do so,
> since otherwise we'll start deadlocking runtime PM again.
> 
> Note that we weren't able to do this before, because of the DP MST
> helpers processing UP requests from topologies in the same context as
> drm_dp_mst_hpd_irq() which would have caused us to open ourselves up to
> receiving hotplug events and deadlocking with runtime suspend/resume.
> Now that those requests are handled asynchronously, this change should
> be completely safe.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Seems reasonable to me, but would feel better if a nouveau person confirmed

Reviewed-by: Sean Paul 


> ---
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 33 +++--
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c 
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 56871d34e3fb..f276918d3f3b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1131,6 +1131,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
>   const char *name = connector->name;
>   struct nouveau_encoder *nv_encoder;
>   int ret;
> + bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
> +
> + if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
> + NV_DEBUG(drm, "service %s\n", name);
> + drm_dp_cec_irq(_connector->aux);
> + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
> + nv50_mstm_service(nv_encoder->dp.mstm);
> +
> + return NVIF_NOTIFY_KEEP;
> + }
>  
>   ret = pm_runtime_get(drm->dev->dev);
>   if (ret == 0) {
> @@ -1151,25 +1161,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
>   return NVIF_NOTIFY_DROP;
>   }
>  
> - if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
> - NV_DEBUG(drm, "service %s\n", name);
> - drm_dp_cec_irq(_connector->aux);
> - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
> - nv50_mstm_service(nv_encoder->dp.mstm);
> - } else {
> - bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
> -
> + if (!plugged)
> + drm_dp_cec_unset_edid(_connector->aux);
> + NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
> + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
>   if (!plugged)
> - drm_dp_cec_unset_edid(_connector->aux);
> - NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
> - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
> - if (!plugged)
> - nv50_mstm_remove(nv_encoder->dp.mstm);
> - }
> -
> -     drm_helper_hpd_irq_event(connector->dev);
> + nv50_mstm_remove(nv_encoder->dp.mstm);
>   }
>  
> + drm_helper_hpd_irq_event(connector->dev);
> +
>   pm_runtime_mark_last_busy(drm->dev->dev);
>   pm_runtime_put_autosuspend(drm->dev->dev);
>   return NVIF_NOTIFY_KEEP;
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 21/27] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:59PM -0400, Lyude Paul wrote:
> This probably hasn't caused any problems up until now since it's
> probably nearly impossible to encounter this in the wild, however if we
> were to receive a connection status notification from the MST hub after
> resume while we're in the middle of reprobing the link addresses for a
> topology then there's a much larger chance that a port could have
> changed from being an output port to input port (or vice versa). If we
> forget to update this bit of information, we'll potentially ignore a
> valid PDT change on a downstream port because we think it's an input
> port.
> 
> So, make sure we read the input_port field in connection status
> notifications in drm_dp_mst_handle_conn_stat() to prevent this from
> happening once we've implemented suspend/resume reprobing.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Nice catch! Same comment here re: port->mutex, but we can sort that out on the
other thread

Reviewed-by: Sean Paul 


> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 51 +++
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 259634c5d6dc..e407aba1fbd2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2078,18 +2078,23 @@ static void
>  drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
>   struct drm_dp_connection_status_notify *conn_stat)
>  {
> - struct drm_device *dev = mstb->mgr->dev;
> + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> + struct drm_device *dev = mgr->dev;
>   struct drm_dp_mst_port *port;
> - int old_ddps;
> - bool dowork = false;
> + struct drm_connector *connector_to_destroy = NULL;
> + int old_ddps, ret;
> + u8 new_pdt;
> + bool dowork = false, create_connector = false;
>  
>   port = drm_dp_get_port(mstb, conn_stat->port_number);
>   if (!port)
>   return;
>  
> + mutex_lock(>lock);
>   drm_modeset_lock(>mode_config.connection_mutex, NULL);
>  
>   old_ddps = port->ddps;
> + port->input = conn_stat->input_port;
>   port->mcs = conn_stat->message_capability_status;
>   port->ldps = conn_stat->legacy_device_plug_status;
>   port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2102,23 +2107,41 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch 
> *mstb,
>   }
>   }
>  
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> -   conn_stat->peer_device_type);
> - if (ret == 1) {
> - dowork = true;
> - } else if (ret < 0) {
> - DRM_ERROR("Failed to change PDT for port %p: %d\n",
> -   port, ret);
> - dowork = false;
> - }
> + new_pdt = port->input ? DP_PEER_DEVICE_NONE : 
> conn_stat->peer_device_type;
> +
> + ret = drm_dp_port_set_pdt(port, new_pdt);
> + if (ret == 1) {
> + dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> +   port, ret);
> + dowork = false;
> + }
> +
> + /*
> +  * We unset port->connector before dropping connection_mutex so that
> +  * there's no chance any of the atomic MST helpers can accidentally
> +  * associate a to-be-destroyed connector with a port.
> +  */
> + if (port->connector && port->input) {
> + connector_to_destroy = port->connector;
> + port->connector = NULL;
> + } else if (!port->connector && !port->input) {
> + create_connector = true;
>   }
>  
>   drm_modeset_unlock(>mode_config.connection_mutex);
> +
> + if (connector_to_destroy)
> + mgr->cbs->destroy_connector(mgr, connector_to_destroy);
> + else if (create_connector)
> + drm_dp_mst_port_add_connector(mstb, port);
> +
> + mutex_unlock(>lock);
> +
>   drm_dp_mst_topology_put_port(port);
>   if (dowork)
>   queue_work(system_long_wq, >mgr->work);
> -
>  }
>  
>  static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct 
> drm_dp_mst_topology_mgr *mgr,
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex

[Sean Paul]

deset_unlock(>mode_config.connection_mutex);
>   drm_dp_mst_topology_put_port(port);
>   if (dowork)
>   queue_work(system_long_wq, >mgr->work);
> @@ -2147,28 +2207,34 @@ drm_dp_get_mst_branch_device_by_guid(struct 
> drm_dp_mst_topology_mgr *mgr,
>  static void drm_dp_check_and_send_link_address(struct 
> drm_dp_mst_topology_mgr *mgr,
>  struct drm_dp_mst_branch *mstb)
>  {
> + struct drm_device *dev = mgr->dev;
>   struct drm_dp_mst_port *port;
> - struct drm_dp_mst_branch *mstb_child;
> +
>   if (!mstb->link_address_sent)
>   drm_dp_send_link_address(mgr, mstb);
>  
>   list_for_each_entry(port, >ports, next) {
> - if (port->input)
> - continue;
> + struct drm_dp_mst_branch *mstb_child = NULL;
> +
> + drm_modeset_lock(>mode_config.connection_mutex, NULL);
>  
> - if (!port->ddps)
> + if (port->input || !port->ddps) {
> + drm_modeset_unlock(>mode_config.connection_mutex);
>   continue;
> + }
>  
>   if (!port->available_pbn)
>   drm_dp_send_enum_path_resources(mgr, mstb, port);
>  
> - if (port->mstb) {
> + if (port->mstb)
>   mstb_child = drm_dp_mst_topology_get_mstb_validated(
>   mgr, port->mstb);
> - if (mstb_child) {
> - drm_dp_check_and_send_link_address(mgr, 
> mstb_child);
> - drm_dp_mst_topology_put_mstb(mstb_child);
> - }
> +
> + drm_modeset_unlock(>mode_config.connection_mutex);
> +
> + if (mstb_child) {
> + drm_dp_check_and_send_link_address(mgr, mstb_child);
> + drm_dp_mst_topology_put_mstb(mstb_child);
>   }
>   }
>  }
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 7d80c38ee00e..1efbb086f7ac 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -45,23 +45,34 @@ struct drm_dp_vcpi {
>  /**
>   * struct drm_dp_mst_port - MST port
>   * @port_num: port number
> - * @input: if this port is an input port.
> - * @mcs: message capability status - DP 1.2 spec.
> - * @ddps: DisplayPort Device Plug Status - DP 1.2
> - * @pdt: Peer Device Type
> - * @ldps: Legacy Device Plug Status
> - * @dpcd_rev: DPCD revision of device on this port
> - * @num_sdp_streams: Number of simultaneous streams
> - * @num_sdp_stream_sinks: Number of stream sinks
> - * @available_pbn: Available bandwidth for this port.
> + * @input: if this port is an input port. Protected by
> + * _device.mode_config.connection_mutex.
> + * @mcs: message capability status - DP 1.2 spec. Protected by
> + * _device.mode_config.connection_mutex.
> + * @ddps: DisplayPort Device Plug Status - DP 1.2. Protected by
> + * _device.mode_config.connection_mutex.
> + * @pdt: Peer Device Type. Protected by
> + * _device.mode_config.connection_mutex.
> + * @ldps: Legacy Device Plug Status. Protected by
> + * _device.mode_config.connection_mutex.
> + * @dpcd_rev: DPCD revision of device on this port. Protected by
> + * _device.mode_config.connection_mutex.
> + * @num_sdp_streams: Number of simultaneous streams. Protected by
> + * _device.mode_config.connection_mutex.
> + * @num_sdp_stream_sinks: Number of stream sinks. Protected by
> + * _device.mode_config.connection_mutex.
> + * @available_pbn: Available bandwidth for this port. Protected by
> + * _device.mode_config.connection_mutex.
>   * @next: link to next port on this branch device
>   * @mstb: branch device on this port, protected by
>   * _dp_mst_topology_mgr.lock
>   * @aux: i2c aux transport to talk to device connected to this port, 
> protected
> - * by _dp_mst_topology_mgr.lock
> + * by _device.mode_config.connection_mutex.
>   * @parent: branch device parent of this port
>   * @vcpi: Virtual Channel Payload info for this port.
> - * @connector: DRM connector this port is connected to.
> + * @connector: DRM connector this port is connected to. Protected by @lock.
> + * When there is already a connector registered for this port, this is also
> + * protected by _device.mode_config.connection_mutex.
>   * @mgr: topology manager this port lives under.
>   *
>   * This structure represents an MST port endpoint on a device somewhere
> @@ -100,6 +111,12 @@ struct drm_dp_mst_port {
>   struct drm_connector *connector;
>   struct drm_dp_mst_topology_mgr *mgr;
>  
> + /**
> +  * @lock: Protects @connector. If needed, this lock should be grabbed
> +  * before _device.mode_config.connection_mutex.
> +  */
> + struct mutex lock;
> +
>   /**
>* @cached_edid: for DP logical ports - make tiling work by ensuring
>* that the EDID for all connectors is read immediately.
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 19/27] drm/dp_mst: Handle UP requests asynchronously

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:57PM -0400, Lyude Paul wrote:
> Once upon a time, hotplugging devices on MST branches actually worked in
> DRM. Now, it only works in amdgpu (likely because of how it's hotplug
> handlers are implemented). On both i915 and nouveau, hotplug
> notifications from MST branches are noticed - but trying to respond to
> them causes messaging timeouts and causes the whole topology state to go
> out of sync with reality, usually resulting in the user needing to
> replug the entire topology in hopes that it actually fixes things.
> 
> The reason for this is because the way we currently handle UP requests
> in MST is completely bogus. drm_dp_mst_handle_up_req() is called from
> drm_dp_mst_hpd_irq(), which is usually called from the driver's hotplug
> handler. Because we handle sending the hotplug event from this function,
> we actually cause the driver's hotplug handler (and in turn, all
> sideband transactions) to block on
> drm_device->mode_config.connection_mutex. This makes it impossible to
> send any sideband messages from the driver's connector probing
> functions, resulting in the aforementioned sideband message timeout.
> 
> There's even more problems with this beyond breaking hotplugging on MST
> branch devices. It also makes it almost impossible to protect
> drm_dp_mst_port struct members under a lock because we then have to
> worry about dealing with all of the lock dependency issues that ensue.
> 
> So, let's finally actually fix this issue by handling the processing of
> up requests asyncronously. This way we can send sideband messages from
> most contexts without having to deal with getting blocked if we hold
> connection_mutex. This also fixes MST branch device hotplugging on i915,
> finally!
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Looks really good!

Reviewed-by: Sean Paul 


> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 146 +++---
>  include/drm/drm_dp_mst_helper.h   |  16 +++
>  2 files changed, 122 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index cfaf9eb7ace9..5101eeab4485 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -46,6 +46,12 @@
>   * protocol. The helpers contain a topology manager and bandwidth manager.
>   * The helpers encapsulate the sending and received of sideband msgs.
>   */
> +struct drm_dp_pending_up_req {
> + struct drm_dp_sideband_msg_hdr hdr;
> + struct drm_dp_sideband_msg_req_body msg;
> + struct list_head next;
> +};
> +
>  static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
> char *buf);
>  
> @@ -3109,6 +3115,7 @@ void drm_dp_mst_topology_mgr_suspend(struct 
> drm_dp_mst_topology_mgr *mgr)
>   drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
>  DP_MST_EN | DP_UPSTREAM_IS_SRC);
>   mutex_unlock(>lock);
> + flush_work(>up_req_work);
>   flush_work(>work);
>   flush_work(>delayed_destroy_work);
>  }
> @@ -3281,12 +3288,70 @@ static int drm_dp_mst_handle_down_rep(struct 
> drm_dp_mst_topology_mgr *mgr)
>   return 0;
>  }
>  
> +static inline void
> +drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
> +   struct drm_dp_pending_up_req *up_req)
> +{
> + struct drm_dp_mst_branch *mstb = NULL;
> + struct drm_dp_sideband_msg_req_body *msg = _req->msg;
> + struct drm_dp_sideband_msg_hdr *hdr = _req->hdr;
> +
> + if (hdr->broadcast) {
> + const u8 *guid = NULL;
> +
> + if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY)
> + guid = msg->u.conn_stat.guid;
> + else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY)
> + guid = msg->u.resource_stat.guid;
> +
> + mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
> + } else {
> + mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad);
> + }
> +
> + if (!mstb) {
> + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
> +   hdr->lct);
> + return;
> + }
> +
> + /* TODO: Add missing handler for DP_RESOURCE_STATUS_NOTIFY events */
> + if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
> + drm_dp_mst_handle_conn_stat(mstb, >u.conn_stat);
> + drm_kms_helper_hotplug_event(mgr->dev);
> + }
> +
> + drm_dp_mst_topology_pu

Re: [Nouveau] [PATCH v2 18/27] drm/dp_mst: Remove lies in {up, down}_rep_recv documentation

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:56PM -0400, Lyude Paul wrote:
> These are most certainly accessed from far more than the mgr work. In
> fact, up_req_recv is -only- ever accessed from outside the mgr work.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Reviewed-by: Sean Paul 

> ---
>  include/drm/drm_dp_mst_helper.h | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index f253ee43e9d9..8ba2a01324bb 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -489,15 +489,11 @@ struct drm_dp_mst_topology_mgr {
>   int conn_base_id;
>  
>   /**
> -  * @down_rep_recv: Message receiver state for down replies. This and
> -  * @up_req_recv are only ever access from the work item, which is
> -  * serialised.
> +  * @down_rep_recv: Message receiver state for down replies.
>*/
>   struct drm_dp_sideband_msg_rx down_rep_recv;
>   /**
> -  * @up_req_recv: Message receiver state for up requests. This and
> -  * @down_rep_recv are only ever access from the work item, which is
> -  * serialised.
> +  * @up_req_recv: Message receiver state for up requests.
>*/
>   struct drm_dp_sideband_msg_rx up_req_recv;
>  
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 17/27] drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:55PM -0400, Lyude Paul wrote:
> The names for these functions are rather confusing. drm_dp_add_port()
> sounds like a function that would simply create a port and add it to a
> topology, and do nothing more. Similarly, drm_dp_update_port() would be
> assumed to be the function that should be used to update port
> information after initial creation.
> 
> While those assumptions are currently correct in how these functions are
> used, a quick glance at drm_dp_add_port() reveals that drm_dp_add_port()
> can also update the information on a port, and seems explicitly designed
> to do so. This can be explained pretty simply by the fact that there's
> more situations that would involve updating the port information based
> on a link address response as opposed to a connection status
> notification than the driver's initial topology probe. Case in point:
> reprobing link addresses after suspend/resume.
> 
> Since we're about to start using drm_dp_add_port() differently for
> suspend/resume reprobing, let's rename both functions to clarify what
> they actually do.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 9944ef2ce885..cfaf9eb7ace9 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1900,9 +1900,10 @@ void drm_dp_mst_connector_early_unregister(struct 
> drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>  
> -static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
> - struct drm_device *dev,
> - struct drm_dp_link_addr_reply_port *port_msg)
> +static void
> +drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> + struct drm_device *dev,
> + struct drm_dp_link_addr_reply_port 
> *port_msg)
>  {
>   struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
>   struct drm_dp_mst_port *port;
> @@ -2011,8 +2012,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch 
> *mstb,
>   drm_dp_mst_topology_put_port(port);
>  }
>  
> -static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
> -struct drm_dp_connection_status_notify 
> *conn_stat)
> +static void
> +drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> + struct drm_dp_connection_status_notify *conn_stat)
>  {
>   struct drm_dp_mst_port *port;
>   int old_ddps;
> @@ -2464,7 +2466,8 @@ static void drm_dp_send_link_address(struct 
> drm_dp_mst_topology_mgr *mgr,
>   drm_dp_check_mstb_guid(mstb, reply->guid);
>  
>   for (i = 0; i < reply->nports; i++)
> - drm_dp_add_port(mstb, mgr->dev, >ports[i]);
> + drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
> + >ports[i]);
>  
>   drm_kms_helper_hotplug_event(mgr->dev);
>  
> @@ -3324,7 +3327,7 @@ static int drm_dp_mst_handle_up_req(struct 
> drm_dp_mst_topology_mgr *mgr)
>   }
>  
>   if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
> - drm_dp_update_port(mstb, _stat);
> + drm_dp_mst_handle_conn_stat(mstb, _stat);
>  
>   DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d 
> pdt: %d\n",
> msg.u.conn_stat.port_number,
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking

[Sean Paul]

gt;connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
> -port,
> -proppath);
> - if (!port->connector) {
> - /* remove it from the port list */
> - mutex_lock(>mgr->lock);
> - list_del(>next);
> - mutex_unlock(>mgr->lock);
> - /* drop port list reference */
> - drm_dp_mst_topology_put_port(port);
> - goto out;
> - }
> + port->connector = (*mgr->cbs->add_connector)(mgr, port,
> +  proppath);
> + if (!port->connector)
> + goto fail;
> +
>   if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
>port->pdt == DP_PEER_DEVICE_SST_SINK) &&
>   port->port_num >= DP_MST_LOGICAL_PORT_0) {
> @@ -1974,28 +1991,38 @@ static void drm_dp_add_port(struct drm_dp_mst_branch 
> *mstb,
>>aux.ddc);
>   drm_connector_set_tile_property(port->connector);
>   }
> - (*mstb->mgr->cbs->register_connector)(port->connector);
> +
> + (*mgr->cbs->register_connector)(port->connector);
>   }
>  
> -out:
>   /* put reference to this port */
>   drm_dp_mst_topology_put_port(port);
> + return;
> +
> +fail:
> + /* Remove it from the port list */
> + mutex_lock(>lock);
> + list_del(>next);
> + mutex_unlock(>lock);
> +
> + /* Drop the port list reference */
> + drm_dp_mst_topology_put_port(port);
> + /* And now drop our reference */
> + drm_dp_mst_topology_put_port(port);
>  }
>  
>  static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
>  struct drm_dp_connection_status_notify 
> *conn_stat)
>  {
>   struct drm_dp_mst_port *port;
> - int old_pdt;
>   int old_ddps;
>   bool dowork = false;
> +
>   port = drm_dp_get_port(mstb, conn_stat->port_number);
>   if (!port)
>   return;
>  
>   old_ddps = port->ddps;
> - old_pdt = port->pdt;
> - port->pdt = conn_stat->peer_device_type;
>   port->mcs = conn_stat->message_capability_status;
>   port->ldps = conn_stat->legacy_device_plug_status;
>   port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2007,11 +2034,17 @@ static void drm_dp_update_port(struct 
> drm_dp_mst_branch *mstb,
>   port->available_pbn = 0;
>   }
>   }
> - if (old_pdt != port->pdt && !port->input) {
> - drm_dp_port_teardown_pdt(port, old_pdt);
>  
> - if (drm_dp_port_setup_pdt(port))
> + if (!port->input) {
> + int ret = drm_dp_port_set_pdt(port,
> +   conn_stat->peer_device_type);
> + if (ret == 1) {
>   dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> +   port, ret);
> + dowork = false;
> + }
>   }
>  
>   drm_dp_mst_topology_put_port(port);
> @@ -4003,9 +4036,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port 
> *port)
>   if (port->connector)
>   port->mgr->cbs->destroy_connector(port->mgr, port->connector);
>  
> - drm_dp_port_teardown_pdt(port, port->pdt);
> - port->pdt = DP_PEER_DEVICE_NONE;
> -
> + drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
>   drm_dp_mst_put_port_malloc(port);
>  }
>  
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 5423a8adda78..f253ee43e9d9 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -55,8 +55,10 @@ struct drm_dp_vcpi {
>   * @num_sdp_stream_sinks: Number of stream sinks
>   * @available_pbn: Available bandwidth for this port.
>   * @next: link to next port on this branch device
> - * @mstb: branch device attach below this port
> - * @aux: i2c aux transport to talk to device connected to this port.
> + * @mstb: branch device on this port, protected by
> + * _dp_mst_topology_mgr.lock
> + * @aux: i2c aux transport to talk to device connected to this port, 
> protected
> + * by _dp_mst_topology_mgr.lock
>   * @parent: branch device parent of this port
>   * @vcpi: Virtual Channel Payload info for this port.
>   * @connector: DRM connector this port is connected to.
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 14/27] drm/dp_mst: Destroy topology_mgr mutexes

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:52PM -0400, Lyude Paul wrote:
> Turns out we've been forgetting for a while now to actually destroy any
> of the mutexes that we create in drm_dp_mst_topology_mgr. So, let's do
> that.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Cleanup is overrated :)

Reviewed-by: Sean Paul 


> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 74161f442584..2f88cc173500 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4339,6 +4339,11 @@ void drm_dp_mst_topology_mgr_destroy(struct 
> drm_dp_mst_topology_mgr *mgr)
>   mgr->aux = NULL;
>   drm_atomic_private_obj_fini(>base);
>   mgr->funcs = NULL;
> +
> + mutex_destroy(>delayed_destroy_lock);
> + mutex_destroy(>payload_lock);
> + mutex_destroy(>qlock);
> + mutex_destroy(>lock);
>  }
>  EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
>  
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 08/27] drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:46PM -0400, Lyude Paul wrote:
> This will allow us to add some locking for port->* members, in
> particular the PDT and ->connector, which can't be done from
> drm_dp_destroy_port() since we don't know what locks the caller might be
> holding.

Might be nice to mention that this is already done in the delayed destroy worker
so readers don't need to go looking for it. Perhaps update this when you apply
the patch.

> 
> Changes since v2:
> * Clarify commit message
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Cc: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 40 +++
>  1 file changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index f5f1d8b50fb6..af3189df28aa 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1511,31 +1511,22 @@ static void drm_dp_destroy_port(struct kref *kref)
>   container_of(kref, struct drm_dp_mst_port, topology_kref);
>   struct drm_dp_mst_topology_mgr *mgr = port->mgr;
>  
> - if (!port->input) {
> - kfree(port->cached_edid);
> + /* There's nothing that needs locking to destroy an input port yet */
> + if (port->input) {
> + drm_dp_mst_put_port_malloc(port);
> + return;
> + }
>  
> - /*
> -  * The only time we don't have a connector
> -  * on an output port is if the connector init
> -  * fails.
> -  */
> - if (port->connector) {
> - /* we can't destroy the connector here, as
> -  * we might be holding the mode_config.mutex
> -  * from an EDID retrieval */
> + kfree(port->cached_edid);
>  
> - mutex_lock(>delayed_destroy_lock);
> - list_add(>next, >destroy_port_list);
> - mutex_unlock(>delayed_destroy_lock);
> - schedule_work(>delayed_destroy_work);
> - return;
> - }
> - /* no need to clean up vcpi
> -  * as if we have no connector we never setup a vcpi */
> - drm_dp_port_teardown_pdt(port, port->pdt);
> - port->pdt = DP_PEER_DEVICE_NONE;
> - }
> - drm_dp_mst_put_port_malloc(port);
> + /*
> +  * we can't destroy the connector here, as we might be holding the
> +  * mode_config.mutex from an EDID retrieval
> +  */
> + mutex_lock(>delayed_destroy_lock);
> + list_add(>next, >destroy_port_list);
> + mutex_unlock(>delayed_destroy_lock);
> + schedule_work(>delayed_destroy_work);
>  }
>  
>  /**
> @@ -3998,7 +3989,8 @@ static void drm_dp_tx_work(struct work_struct *work)
>  static inline void
>  drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
>  {
> - port->mgr->cbs->destroy_connector(port->mgr, port->connector);
> + if (port->connector)
> + port->mgr->cbs->destroy_connector(port->mgr, port->connector);
>  
>   drm_dp_port_teardown_pdt(port, port->pdt);
>   port->pdt = DP_PEER_DEVICE_NONE;
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 04/27] drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:42PM -0400, Lyude Paul wrote:
> Yes, apparently we've been testing this for every single driver load for
> quite a long time now. At least that means our PBN calculation is solid!
> 
> Anyway, introduce self tests for MST and move this into there.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Reviewed-by: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 27 ---
>  drivers/gpu/drm/selftests/Makefile|  2 +-
>  .../gpu/drm/selftests/drm_modeset_selftests.h |  1 +
>  .../drm/selftests/test-drm_dp_mst_helper.c| 34 +++
>  .../drm/selftests/test-drm_modeset_common.h   |  1 +
>  5 files changed, 37 insertions(+), 28 deletions(-)
>  create mode 100644 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 738f260d4b15..6f7f449ca12b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -47,7 +47,6 @@
>   */
>  static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
> char *buf);
> -static int test_calc_pbn_mode(void);
>  
>  static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
>  
> @@ -3561,30 +3560,6 @@ int drm_dp_calc_pbn_mode(int clock, int bpp)
>  }
>  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>  
> -static int test_calc_pbn_mode(void)
> -{
> - int ret;
> - ret = drm_dp_calc_pbn_mode(154000, 30);
> - if (ret != 689) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, 
> expected PBN %d, actual PBN %d.\n",
> - 154000, 30, 689, ret);
> - return -EINVAL;
> - }
> - ret = drm_dp_calc_pbn_mode(234000, 30);
> - if (ret != 1047) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, 
> expected PBN %d, actual PBN %d.\n",
> - 234000, 30, 1047, ret);
> - return -EINVAL;
> - }
> - ret = drm_dp_calc_pbn_mode(297000, 24);
> - if (ret != 1063) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d, 
> expected PBN %d, actual PBN %d.\n",
> - 297000, 24, 1063, ret);
> - return -EINVAL;
> - }
> - return 0;
> -}
> -
>  /* we want to kick the TX after we've ack the up/down IRQs. */
>  static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
>  {
> @@ -4033,8 +4008,6 @@ int drm_dp_mst_topology_mgr_init(struct 
> drm_dp_mst_topology_mgr *mgr,
>   if (!mgr->proposed_vcpis)
>   return -ENOMEM;
>   set_bit(0, >payload_mask);
> - if (test_calc_pbn_mode() < 0)
> - DRM_ERROR("MST PBN self-test failed\n");
>  
>   mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>   if (mst_state == NULL)
> diff --git a/drivers/gpu/drm/selftests/Makefile 
> b/drivers/gpu/drm/selftests/Makefile
> index aae88f8a016c..d2137342b371 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
>test-drm_format.o test-drm_framebuffer.o \
> -   test-drm_damage_helper.o
> +   test-drm_damage_helper.o test-drm_dp_mst_helper.o
>  
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o 
> test-drm_cmdline_parser.o
> diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h 
> b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> index 464753746013..dec3ee3ec96f 100644
> --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> @@ -32,3 +32,4 @@ selftest(damage_iter_damage_one_intersect, 
> igt_damage_iter_damage_one_intersect)
>  selftest(damage_iter_damage_one_outside, igt_damage_iter_damage_one_outside)
>  selftest(damage_iter_damage_src_moved, igt_damage_iter_damage_src_moved)
>  selftest(damage_iter_damage_not_visible, igt_damage_iter_damage_not_visible)
> +selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode)
> diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c 
> b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> new file mode 100644
> index ..9baa5171988d
> --- /dev/null
> +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> @@ -0,0 +1,34 @@
>

Re: [Nouveau] [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously

[Sean Paul]

; + if (port)
> + list_del(>next);
> + mutex_unlock(>delayed_destroy_lock);
> +
> + if (!port)
> + break;
> +
> + drm_dp_delayed_destroy_port(port);
> + send_hotplug = true;
> + go_again = true;
> + }
> + } while (go_again);
>  
> - drm_dp_mst_put_port_malloc(port);
> - send_hotplug = true;
> - }
>   if (send_hotplug)
>   drm_kms_helper_hotplug_event(mgr->dev);
>  }
> @@ -3957,12 +4010,13 @@ int drm_dp_mst_topology_mgr_init(struct 
> drm_dp_mst_topology_mgr *mgr,
>   mutex_init(>lock);
>   mutex_init(>qlock);
>   mutex_init(>payload_lock);
> - mutex_init(>destroy_connector_lock);
> + mutex_init(>delayed_destroy_lock);
>   INIT_LIST_HEAD(>tx_msg_downq);
> - INIT_LIST_HEAD(>destroy_connector_list);
> + INIT_LIST_HEAD(>destroy_port_list);
> + INIT_LIST_HEAD(>destroy_branch_device_list);
>   INIT_WORK(>work, drm_dp_mst_link_probe_work);
>   INIT_WORK(>tx_work, drm_dp_tx_work);
> - INIT_WORK(>destroy_connector_work, drm_dp_destroy_connector_work);
> + INIT_WORK(>delayed_destroy_work, drm_dp_delayed_destroy_work);
>   init_waitqueue_head(>tx_waitq);
>   mgr->dev = dev;
>   mgr->aux = aux;
> @@ -4005,7 +4059,7 @@ void drm_dp_mst_topology_mgr_destroy(struct 
> drm_dp_mst_topology_mgr *mgr)
>  {
>   drm_dp_mst_topology_mgr_set_mst(mgr, false);
>   flush_work(>work);
> - flush_work(>destroy_connector_work);
> + cancel_work_sync(>delayed_destroy_work);
>   mutex_lock(>payload_lock);
>   kfree(mgr->payloads);
>   mgr->payloads = NULL;
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index fc349204a71b..4a4507fe928d 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -143,6 +143,12 @@ struct drm_dp_mst_branch {
>*/
>   struct kref malloc_kref;
>  
> + /**
> +  * @destroy_next: linked-list entry used by
> +  * drm_dp_delayed_destroy_work()
> +  */
> + struct list_head destroy_next;
> +
>   u8 rad[8];
>   u8 lct;
>   int num_ports;
> @@ -575,18 +581,24 @@ struct drm_dp_mst_topology_mgr {
>   struct work_struct tx_work;
>  
>   /**
> -  * @destroy_connector_list: List of to be destroyed connectors.
> +  * @destroy_port_list: List of to be destroyed connectors.
> +  */
> + struct list_head destroy_port_list;
> + /**
> +  * @destroy_branch_device_list: List of to be destroyed branch
> +  * devices.
>*/
> - struct list_head destroy_connector_list;
> + struct list_head destroy_branch_device_list;
>   /**
> -  * @destroy_connector_lock: Protects @connector_list.
> +  * @delayed_destroy_lock: Protects @destroy_port_list and
> +  * @destroy_branch_device_list.
>*/
> - struct mutex destroy_connector_lock;
> + struct mutex delayed_destroy_lock;
>   /**
> -  * @destroy_connector_work: Work item to destroy connectors. Needed to
> -  * avoid locking inversion.
> +  * @delayed_destroy_work: Work item to destroy MST port and branch
> +  * devices, needed to avoid locking inversion.
>*/
> - struct work_struct destroy_connector_work;
> + struct work_struct delayed_destroy_work;
>  };
>  
>  int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 02/27] drm/dp_mst: Get rid of list clear in destroy_connector_work

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:40PM -0400, Lyude Paul wrote:
> This seems to be some leftover detritus from before the port/mstb kref
> cleanup and doesn't do anything anymore, so get rid of it.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Reviewed-by: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 36db66a0ddb1..3054ec622506 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3760,8 +3760,6 @@ static void drm_dp_destroy_connector_work(struct 
> work_struct *work)
>   list_del(>next);
>   mutex_unlock(>destroy_connector_lock);
>  
> - INIT_LIST_HEAD(>next);
> -
>   mgr->cbs->destroy_connector(mgr, port->connector);
>  
>   drm_dp_port_teardown_pdt(port, port->pdt);
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 01/27] drm/dp_mst: Move link address dumping into a function

[Sean Paul]

On Tue, Sep 03, 2019 at 04:45:39PM -0400, Lyude Paul wrote:
> Makes things easier to read.
> 
> Cc: Juston Li 
> Cc: Imre Deak 
> Cc: Ville Syrjälä 
> Cc: Harry Wentland 
> Reviewed-by: Daniel Vetter 
> Signed-off-by: Lyude Paul 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 35 ++-
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 82add736e17d..36db66a0ddb1 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2103,6 +2103,28 @@ static void drm_dp_queue_down_tx(struct 
> drm_dp_mst_topology_mgr *mgr,
>   mutex_unlock(>qlock);
>  }
>  
> +static void
> +drm_dp_dump_link_address(struct drm_dp_link_address_ack_reply *reply)
> +{
> + struct drm_dp_link_addr_reply_port *port_reply;
> + int i;
> +
> + for (i = 0; i < reply->nports; i++) {
> + port_reply = >ports[i];
> + DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn: %d, dpcd_rev: 
> %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n",
> +   i,
> +   port_reply->input_port,
> +   port_reply->peer_device_type,
> +   port_reply->port_number,
> +   port_reply->dpcd_revision,
> +   port_reply->mcs,
> +   port_reply->ddps,
> +   port_reply->legacy_device_plug_status,
> +   port_reply->num_sdp_streams,
> +   port_reply->num_sdp_stream_sinks);
> + }
> +}
> +
>  static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>struct drm_dp_mst_branch *mstb)
>  {
> @@ -2128,18 +2150,7 @@ static void drm_dp_send_link_address(struct 
> drm_dp_mst_topology_mgr *mgr,
>   DRM_DEBUG_KMS("link address nak received\n");
>   } else {
>   DRM_DEBUG_KMS("link address reply: %d\n", 
> txmsg->reply.u.link_addr.nports);
> - for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) {
> - DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn: 
> %d, dpcd_rev: %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n", i,
> -
> txmsg->reply.u.link_addr.ports[i].input_port,
> -
> txmsg->reply.u.link_addr.ports[i].peer_device_type,
> -
> txmsg->reply.u.link_addr.ports[i].port_number,
> -
> txmsg->reply.u.link_addr.ports[i].dpcd_revision,
> -txmsg->reply.u.link_addr.ports[i].mcs,
> -txmsg->reply.u.link_addr.ports[i].ddps,
> -
> txmsg->reply.u.link_addr.ports[i].legacy_device_plug_status,
> -
> txmsg->reply.u.link_addr.ports[i].num_sdp_streams,
> -
> txmsg->reply.u.link_addr.ports[i].num_sdp_stream_sinks);
> - }
> + drm_dp_dump_link_address(>reply.u.link_addr);
>  
>   drm_dp_check_mstb_guid(mstb, 
> txmsg->reply.u.link_addr.guid);
>  
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH v5 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state

[Sean Paul]

From: Sean Paul 

Everyone who implements connector_helper_funcs->atomic_check reaches
into the connector state to get the atomic state. Instead of continuing
this pattern, change the callback signature to just give atomic state
and let the driver determine what it does and does not need from it.

Eventually all atomic functions should do this, but that's just too much
busy work for me.

Changes in v3:
- Added to the set
Changes in v4:
- None
Changes in v5:
- intel_digital_connector_atomic_check declaration moved to i915_atomic.h

Link to v3: 
https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-5-s...@poorly.run
Link to v4: 
https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-5-s...@poorly.run

Cc: Daniel Vetter 
Cc: Ville Syrjälä 
Cc: Jani Nikula 
Cc: Joonas Lahtinen 
Cc: Rodrigo Vivi 
Cc: Ben Skeggs 
Cc: Laurent Pinchart 
Cc: Kieran Bingham 
Cc: Eric Anholt 
Tested-by: Heiko Stuebner 
Acked-by: Daniel Vetter 
Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/drm_atomic_helper.c  |  4 ++--
 drivers/gpu/drm/i915/intel_atomic.c  |  8 +---
 drivers/gpu/drm/i915/intel_atomic.h  |  2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  7 ---
 drivers/gpu/drm/i915/intel_sdvo.c|  9 +
 drivers/gpu/drm/i915/intel_tv.c  |  8 +---
 drivers/gpu/drm/nouveau/dispnv50/disp.c  |  5 +++--
 drivers/gpu/drm/rcar-du/rcar_lvds.c  | 12 +++-
 drivers/gpu/drm/vc4/vc4_txp.c|  7 ---
 include/drm/drm_modeset_helper_vtables.h |  2 +-
 10 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 2133f62539176..e58be69960692 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -686,7 +686,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
 
if (funcs->atomic_check)
-   ret = funcs->atomic_check(connector, 
new_connector_state);
+   ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
 
@@ -728,7 +728,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
continue;
 
if (funcs->atomic_check)
-   ret = funcs->atomic_check(connector, 
new_connector_state);
+   ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
b/drivers/gpu/drm/i915/intel_atomic.c
index 58b8049649a0f..ab40448a19d56 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -106,12 +106,14 @@ int intel_digital_connector_atomic_set_property(struct 
drm_connector *connector,
 }
 
 int intel_digital_connector_atomic_check(struct drm_connector *conn,
-struct drm_connector_state *new_state)
+struct drm_atomic_state *state)
 {
+   struct drm_connector_state *new_state =
+   drm_atomic_get_new_connector_state(state, conn);
struct intel_digital_connector_state *new_conn_state =
to_intel_digital_connector_state(new_state);
struct drm_connector_state *old_state =
-   drm_atomic_get_old_connector_state(new_state->state, conn);
+   drm_atomic_get_old_connector_state(state, conn);
struct intel_digital_connector_state *old_conn_state =
to_intel_digital_connector_state(old_state);
struct drm_crtc_state *crtc_state;
@@ -121,7 +123,7 @@ int intel_digital_connector_atomic_check(struct 
drm_connector *conn,
if (!new_state->crtc)
return 0;
 
-   crtc_state = drm_atomic_get_new_crtc_state(new_state->state, 
new_state->crtc);
+   crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
 
/*
 * These properties are handled by fastset, and might not end
diff --git a/drivers/gpu/drm/i915/intel_atomic.h 
b/drivers/gpu/drm/i915/intel_atomic.h
index 1c8507da1a690..58065d3161a34 100644
--- a/drivers/gpu/drm/i915/intel_atomic.h
+++ b/drivers/gpu/drm/i915/intel_atomic.h
@@ -28,7 +28,7 @@ int intel_digital_connector_atomic_set_property(struct 
drm_connector *connector,
struct drm_property *property,
u64 val);
 int intel_digital_connector_atomic_check(struct drm_connector *conn,
-struct drm_connector_state *new_state);
+struct drm_atomic_state *state);
 struct drm_connector_state *
 intel_digital_connector_duplicate_state(struct drm_connector *connector);
 
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 0caf645fbbb84..60

Re: [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state

[Sean Paul]

On Thu, May 16, 2019 at 03:00:01PM +0300, Laurent Pinchart wrote:
> Hi Sean,
> 
> On Mon, May 13, 2019 at 10:38:58AM -0400, Sean Paul wrote:
> > On Sat, May 11, 2019 at 3:12 PM Laurent Pinchart wrote:
> > > On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote:
> > >> From: Sean Paul 
> > >>
> > >> Everyone who implements connector_helper_funcs->atomic_check reaches
> > >> into the connector state to get the atomic state. Instead of continuing
> > >> this pattern, change the callback signature to just give atomic state
> > >> and let the driver determine what it does and does not need from it.
> > >>
> > >> Eventually all atomic functions should do this, but that's just too much
> > >> busy work for me.
> > >
> > > Given that drivers also access the connector state, isn't this slightly
> > > more inefficient ?
> > 
> > Inefficient in terms of what?
> 
> In terms of the operation having to lookup the connector state, when the
> caller has it and can easily pass it. As Daniel commented, this may be
> the price to pay for a cleaner API, but I wonder how much overhead all
> the state tracking is costing.
> 

It'd be interesting to quantify, but iirc the last time we benchmarked
atomic check commits they were virtually free (~thousands/s).

> > Agree that in isolation this patch might seem unnecessary, but it ties
> > in with the encoder and bridge CLs which accept drm_atomic_state in
> 
> CLs ?

Googleism for patches, I usually catch these before sending... guess I missed
one.

Sean

> 
> > their hooks. In general the idea is to convert all atomic functions to
> > take overall atomic state instead of just their object state. Reality
> > has proven to be more complicated and we need more access than what
> > the current implementation provides.
> > 
> > Sean
> > 
> > >> Changes in v3:
> > >> - Added to the set
> > >>
> > >> Cc: Daniel Vetter 
> > >> Cc: Ville Syrjälä 
> > >> Cc: Jani Nikula 
> > >> Cc: Joonas Lahtinen 
> > >> Cc: Rodrigo Vivi 
> > >> Cc: Ben Skeggs 
> > >> Cc: Laurent Pinchart 
> > >> Cc: Kieran Bingham 
> > >> Cc: Eric Anholt 
> > >> Signed-off-by: Sean Paul 
> > >> ---
> > >>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++--
> > >>  drivers/gpu/drm/i915/intel_atomic.c  |  8 +---
> > >>  drivers/gpu/drm/i915/intel_dp_mst.c  |  7 ---
> > >>  drivers/gpu/drm/i915/intel_drv.h |  2 +-
> > >>  drivers/gpu/drm/i915/intel_sdvo.c|  9 +
> > >>  drivers/gpu/drm/i915/intel_tv.c  |  8 +---
> > >>  drivers/gpu/drm/nouveau/dispnv50/disp.c  |  5 +++--
> > >>  drivers/gpu/drm/rcar-du/rcar_lvds.c  | 12 +++-
> > >>  drivers/gpu/drm/vc4/vc4_txp.c|  7 ---
> > >>  include/drm/drm_modeset_helper_vtables.h |  2 +-
> > >>  10 files changed, 37 insertions(+), 27 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > >> b/drivers/gpu/drm/drm_atomic_helper.c
> > >> index 9d9e47276839..fa5a367507c1 100644
> > >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > >> @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device 
> > >> *dev,
> > >>   }
> > >>
> > >>   if (funcs->atomic_check)
> > >> - ret = funcs->atomic_check(connector, 
> > >> new_connector_state);
> > >> + ret = funcs->atomic_check(connector, state);
> > >>   if (ret)
> > >>   return ret;
> > >>
> > >> @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device 
> > >> *dev,
> > >>   continue;
> > >>
> > >>   if (funcs->atomic_check)
> > >> - ret = funcs->atomic_check(connector, 
> > >> new_connector_state);
> > >> + ret = funcs->atomic_check(connector, state);
> > >>   if (ret)
> > >>   return ret;
> > >>   }
> > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
> > >> b/drivers/gpu/drm/i915/intel_atomic.c
&

Re: [Nouveau] [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state

[Sean Paul]

On Sat, May 11, 2019 at 3:12 PM Laurent Pinchart
 wrote:
>
> Hi Sean,
>
> Thank you for the patch.
>

Hey Laurent,
Thanks for looking!


> On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote:
> > From: Sean Paul 
> >
> > Everyone who implements connector_helper_funcs->atomic_check reaches
> > into the connector state to get the atomic state. Instead of continuing
> > this pattern, change the callback signature to just give atomic state
> > and let the driver determine what it does and does not need from it.
> >
> > Eventually all atomic functions should do this, but that's just too much
> > busy work for me.
>
> Given that drivers also access the connector state, isn't this slightly
> more inefficient ?
>

Inefficient in terms of what?

Agree that in isolation this patch might seem unnecessary, but it ties
in with the encoder and bridge CLs which accept drm_atomic_state in
their hooks. In general the idea is to convert all atomic functions to
take overall atomic state instead of just their object state. Reality
has proven to be more complicated and we need more access than what
the current implementation provides.

Sean

> > Changes in v3:
> > - Added to the set
> >
> > Cc: Daniel Vetter 
> > Cc: Ville Syrjälä 
> > Cc: Jani Nikula 
> > Cc: Joonas Lahtinen 
> > Cc: Rodrigo Vivi 
> > Cc: Ben Skeggs 
> > Cc: Laurent Pinchart 
> > Cc: Kieran Bingham 
> > Cc: Eric Anholt 
> > Signed-off-by: Sean Paul 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++--
> >  drivers/gpu/drm/i915/intel_atomic.c  |  8 +---
> >  drivers/gpu/drm/i915/intel_dp_mst.c  |  7 ---
> >  drivers/gpu/drm/i915/intel_drv.h |  2 +-
> >  drivers/gpu/drm/i915/intel_sdvo.c|  9 +
> >  drivers/gpu/drm/i915/intel_tv.c  |  8 +---
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c  |  5 +++--
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c  | 12 +++-
> >  drivers/gpu/drm/vc4/vc4_txp.c|  7 ---
> >  include/drm/drm_modeset_helper_vtables.h |  2 +-
> >  10 files changed, 37 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 9d9e47276839..fa5a367507c1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >   }
> >
> >   if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector, 
> > new_connector_state);
> > + ret = funcs->atomic_check(connector, state);
> >   if (ret)
> >   return ret;
> >
> > @@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >   continue;
> >
> >   if (funcs->atomic_check)
> > - ret = funcs->atomic_check(connector, 
> > new_connector_state);
> > + ret = funcs->atomic_check(connector, state);
> >   if (ret)
> >   return ret;
> >   }
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
> > b/drivers/gpu/drm/i915/intel_atomic.c
> > index b844e8840c6f..e8a5b82e9242 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -103,12 +103,14 @@ int 
> > intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> >  }
> >
> >  int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > -  struct drm_connector_state 
> > *new_state)
> > +  struct drm_atomic_state *state)
> >  {
> > + struct drm_connector_state *new_state =
> > + drm_atomic_get_new_connector_state(state, conn);
> >   struct intel_digital_connector_state *new_conn_state =
> >   to_intel_digital_connector_state(new_state);
> >   struct drm_connector_state *old_state =
> > - drm_atomic_get_old_connector_state(new_state->state, conn);
> > + drm_atomic_get_old_connector_state(state, conn);
> >   struct intel_digital_connector_state *old_conn_state =
> >   to_intel_digital_connector_state(old_state);
> >   struct drm_crtc_state *crtc_state;
> > @@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct 
> > drm_connector *conn,
> >   if (!n

[Nouveau] [PATCH v4 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state

[Sean Paul]

From: Sean Paul 

Everyone who implements connector_helper_funcs->atomic_check reaches
into the connector state to get the atomic state. Instead of continuing
this pattern, change the callback signature to just give atomic state
and let the driver determine what it does and does not need from it.

Eventually all atomic functions should do this, but that's just too much
busy work for me.

Changes in v3:
- Added to the set
Changes in v4:
- None

Link to v3: 
https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-5-s...@poorly.run

Cc: Daniel Vetter 
Cc: Ville Syrjälä 
Cc: Jani Nikula 
Cc: Joonas Lahtinen 
Cc: Rodrigo Vivi 
Cc: Ben Skeggs 
Cc: Laurent Pinchart 
Cc: Kieran Bingham 
Cc: Eric Anholt 
Acked-by: Daniel Vetter 
Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/drm_atomic_helper.c  |  4 ++--
 drivers/gpu/drm/i915/intel_atomic.c  |  8 +---
 drivers/gpu/drm/i915/intel_dp_mst.c  |  7 ---
 drivers/gpu/drm/i915/intel_drv.h |  2 +-
 drivers/gpu/drm/i915/intel_sdvo.c|  9 +
 drivers/gpu/drm/i915/intel_tv.c  |  8 +---
 drivers/gpu/drm/nouveau/dispnv50/disp.c  |  5 +++--
 drivers/gpu/drm/rcar-du/rcar_lvds.c  | 12 +++-
 drivers/gpu/drm/vc4/vc4_txp.c|  7 ---
 include/drm/drm_modeset_helper_vtables.h |  2 +-
 10 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index e8b7187a8494..ee945d6f1cba 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
 
if (funcs->atomic_check)
-   ret = funcs->atomic_check(connector, 
new_connector_state);
+   ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
 
@@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
continue;
 
if (funcs->atomic_check)
-   ret = funcs->atomic_check(connector, 
new_connector_state);
+   ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
b/drivers/gpu/drm/i915/intel_atomic.c
index b844e8840c6f..e8a5b82e9242 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct 
drm_connector *connector,
 }
 
 int intel_digital_connector_atomic_check(struct drm_connector *conn,
-struct drm_connector_state *new_state)
+struct drm_atomic_state *state)
 {
+   struct drm_connector_state *new_state =
+   drm_atomic_get_new_connector_state(state, conn);
struct intel_digital_connector_state *new_conn_state =
to_intel_digital_connector_state(new_state);
struct drm_connector_state *old_state =
-   drm_atomic_get_old_connector_state(new_state->state, conn);
+   drm_atomic_get_old_connector_state(state, conn);
struct intel_digital_connector_state *old_conn_state =
to_intel_digital_connector_state(old_state);
struct drm_crtc_state *crtc_state;
@@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct 
drm_connector *conn,
if (!new_state->crtc)
return 0;
 
-   crtc_state = drm_atomic_get_new_crtc_state(new_state->state, 
new_state->crtc);
+   crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
 
/*
 * These properties are handled by fastset, and might not end
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 19d81cef2ab6..89cfec128ba0 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct 
intel_encoder *encoder,
 
 static int
 intel_dp_mst_atomic_check(struct drm_connector *connector,
- struct drm_connector_state *new_conn_state)
+ struct drm_atomic_state *state)
 {
-   struct drm_atomic_state *state = new_conn_state->state;
+   struct drm_connector_state *new_conn_state =
+   drm_atomic_get_new_connector_state(state, connector);
struct drm_connector_state *old_conn_state =
drm_atomic_get_old_connector_state(state, connector);
struct intel_connector *intel_connector =
@@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
struct drm_dp_mst_topology_mgr *mgr;
int ret;
 
-   ret = intel_digital_connector_atomic_check(connector, new_conn_state);
+   ret = intel_d

[Nouveau] [PATCH v3 04/10] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state

[Sean Paul]

From: Sean Paul 

Everyone who implements connector_helper_funcs->atomic_check reaches
into the connector state to get the atomic state. Instead of continuing
this pattern, change the callback signature to just give atomic state
and let the driver determine what it does and does not need from it.

Eventually all atomic functions should do this, but that's just too much
busy work for me.

Changes in v3:
- Added to the set

Cc: Daniel Vetter 
Cc: Ville Syrjälä 
Cc: Jani Nikula 
Cc: Joonas Lahtinen 
Cc: Rodrigo Vivi 
Cc: Ben Skeggs 
Cc: Laurent Pinchart 
Cc: Kieran Bingham 
Cc: Eric Anholt 
Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/drm_atomic_helper.c  |  4 ++--
 drivers/gpu/drm/i915/intel_atomic.c  |  8 +---
 drivers/gpu/drm/i915/intel_dp_mst.c  |  7 ---
 drivers/gpu/drm/i915/intel_drv.h |  2 +-
 drivers/gpu/drm/i915/intel_sdvo.c|  9 +
 drivers/gpu/drm/i915/intel_tv.c  |  8 +---
 drivers/gpu/drm/nouveau/dispnv50/disp.c  |  5 +++--
 drivers/gpu/drm/rcar-du/rcar_lvds.c  | 12 +++-
 drivers/gpu/drm/vc4/vc4_txp.c|  7 ---
 include/drm/drm_modeset_helper_vtables.h |  2 +-
 10 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 9d9e47276839..fa5a367507c1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -683,7 +683,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
 
if (funcs->atomic_check)
-   ret = funcs->atomic_check(connector, 
new_connector_state);
+   ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
 
@@ -725,7 +725,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
continue;
 
if (funcs->atomic_check)
-   ret = funcs->atomic_check(connector, 
new_connector_state);
+   ret = funcs->atomic_check(connector, state);
if (ret)
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
b/drivers/gpu/drm/i915/intel_atomic.c
index b844e8840c6f..e8a5b82e9242 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -103,12 +103,14 @@ int intel_digital_connector_atomic_set_property(struct 
drm_connector *connector,
 }
 
 int intel_digital_connector_atomic_check(struct drm_connector *conn,
-struct drm_connector_state *new_state)
+struct drm_atomic_state *state)
 {
+   struct drm_connector_state *new_state =
+   drm_atomic_get_new_connector_state(state, conn);
struct intel_digital_connector_state *new_conn_state =
to_intel_digital_connector_state(new_state);
struct drm_connector_state *old_state =
-   drm_atomic_get_old_connector_state(new_state->state, conn);
+   drm_atomic_get_old_connector_state(state, conn);
struct intel_digital_connector_state *old_conn_state =
to_intel_digital_connector_state(old_state);
struct drm_crtc_state *crtc_state;
@@ -118,7 +120,7 @@ int intel_digital_connector_atomic_check(struct 
drm_connector *conn,
if (!new_state->crtc)
return 0;
 
-   crtc_state = drm_atomic_get_new_crtc_state(new_state->state, 
new_state->crtc);
+   crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
 
/*
 * These properties are handled by fastset, and might not end
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 19d81cef2ab6..89cfec128ba0 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -143,9 +143,10 @@ static int intel_dp_mst_compute_config(struct 
intel_encoder *encoder,
 
 static int
 intel_dp_mst_atomic_check(struct drm_connector *connector,
- struct drm_connector_state *new_conn_state)
+ struct drm_atomic_state *state)
 {
-   struct drm_atomic_state *state = new_conn_state->state;
+   struct drm_connector_state *new_conn_state =
+   drm_atomic_get_new_connector_state(state, connector);
struct drm_connector_state *old_conn_state =
drm_atomic_get_old_connector_state(state, connector);
struct intel_connector *intel_connector =
@@ -155,7 +156,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
struct drm_dp_mst_topology_mgr *mgr;
int ret;
 
-   ret = intel_digital_connector_atomic_check(connector, new_conn_state);
+   ret = intel_digital_connector_atomic_check(connector, state);
if (ret)
return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b

Re: [Nouveau] [PATCH 4/7] drm: Move the legacy kms disable_all helper to crtc helpers

[Sean Paul]

On Mon, Dec 10, 2018 at 10:58:20AM -0500, Alex Deucher wrote:
> On Mon, Dec 10, 2018 at 5:04 AM Daniel Vetter  wrote:
> >
> > It's not a core function, and the matching atomic functions are also
> > not in the core. Plus the suspend/resume helper is also already there.
> >
> > Needs a tiny bit of open-coding, but less midlayer beats that I think.
> >
> > Cc: Sam Bobroff 
> > Signed-off-by: Daniel Vetter 
> > Cc: Maarten Lankhorst 
> > Cc: Maxime Ripard 
> > Cc: Sean Paul 
> > Cc: David Airlie 
> > Cc: Ben Skeggs 
> > Cc: Alex Deucher 
> > Cc: "Christian König" 
> > Cc: "David (ChunMing) Zhou" 
> > Cc: Rex Zhu 
> > Cc: Andrey Grodzovsky 
> > Cc: Huang Rui 
> > Cc: Shaoyun Liu 
> > Cc: Monk Liu 
> > Cc: nouveau@lists.freedesktop.org
> > Cc: amd-...@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 +-
> >  drivers/gpu/drm/drm_crtc.c | 31 ---
> >  drivers/gpu/drm/drm_crtc_helper.c  | 35 ++
> >  drivers/gpu/drm/nouveau/nouveau_display.c  |  2 +-
> >  drivers/gpu/drm/radeon/radeon_display.c|  2 +-
> >  include/drm/drm_crtc.h |  2 --
> >  include/drm/drm_crtc_helper.h  |  1 +
> >  7 files changed, 39 insertions(+), 36 deletions(-)
> >

/snip

> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
> > b/drivers/gpu/drm/drm_crtc_helper.c
> > index a3c81850e755..23159eb494f1 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -984,3 +984,38 @@ void drm_helper_resume_force_mode(struct drm_device 
> > *dev)
> > drm_modeset_unlock_all(dev);
> >  }
> >  EXPORT_SYMBOL(drm_helper_resume_force_mode);
> > +
> > +/**
> > + * drm_helper_force_disable_all - Forcibly turn off all enabled CRTCs
> > + * @dev: DRM device whose CRTCs to turn off
> > + *
> > + * Drivers may want to call this on unload to ensure that all displays are
> > + * unlit and the GPU is in a consistent, low power state. Takes modeset 
> > locks.
> > + *
> > + * Note: This should only be used by non-atomic legacy drivers. For an 
> > atomic
> > + * version look at drm_atomic_helper_shutdown().
> > + *
> > + * Returns:
> > + * Zero on success, error code on failure.
> > + */
> > +int drm_helper_force_disable_all(struct drm_device *dev)
> 
> Maybe put crtc somewhere in the function name so it's clear what we
> are disabling.

FWIW, I think it's more clear this way. set_config_internal will turn off
everything attached to the crtc, so _everything_ will be disabled in this case.

Either way,

Reviewed-by: Sean Paul 

Sean

> With that fixed:
> Reviewed-by: Alex Deucher 
> 
> > +{
> > +   struct drm_crtc *crtc;
> > +   int ret = 0;
> > +
> > +   drm_modeset_lock_all(dev);
> > +   drm_for_each_crtc(crtc, dev)
> > +   if (crtc->enabled) {
> > +   struct drm_mode_set set = {
> > +   .crtc = crtc,
> > +   };
> > +
> > +   ret = drm_mode_set_config_internal();
> > +   if (ret)
> > +   goto out;
> > +   }
> > +out:
> > +   drm_modeset_unlock_all(dev);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL(drm_helper_force_disable_all);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
> > b/drivers/gpu/drm/nouveau/nouveau_display.c
> > index f326ffd86766..5d273a655479 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> > @@ -453,7 +453,7 @@ nouveau_display_fini(struct drm_device *dev, bool 
> > suspend, bool runtime)
> > if (drm_drv_uses_atomic_modeset(dev))
> > drm_atomic_helper_shutdown(dev);
> > else
> > -   drm_crtc_force_disable_all(dev);
> > +   drm_helper_force_disable_all(dev);
> > }
> >
> > /* disable flip completion events */
> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
> > b/drivers/gpu/drm/radeon/radeon_display.c
> > index e6912eb99b42..92332226e5cf 100644
> > --- a/drivers/gpu/drm/radeon/radeon_display.c
> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
> > @@ -1643,7 +1643,7 @@ void radeon_modeset_fini(struct radeon_device *rdev)
> > if (rdev->mode_info.m

Re: [Nouveau] [PATCH 2/2] drm/atomic: Create and use __drm_atomic_helper_crtc_reset() everywhere

[Sean Paul]

On Mon, Nov 12, 2018 at 04:01:14PM +0100, Maarten Lankhorst wrote:
> We already have __drm_atomic_helper_connector_reset() and
> __drm_atomic_helper_plane_reset(), extend this to crtc as well.
> 
> Most drivers already have a gpu reset hook, correct it.
> Nouveau already implemented its own __drm_atomic_helper_crtc_reset(),
> convert it to the common one.
> 
> Signed-off-by: Maarten Lankhorst 
> Cc: Harry Wentland 
> Cc: Leo Li 
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "David (ChunMing) Zhou" 
> Cc: David Airlie 
> Cc: Liviu Dudau 
> Cc: Brian Starkey 
> Cc: Mali DP Maintainers 
> Cc: Boris Brezillon 
> Cc: Nicolas Ferre 
> Cc: Alexandre Belloni 
> Cc: Ludovic Desroches 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> Cc: Philipp Zabel 
> Cc: CK Hu 
> Cc: Matthias Brugger 
> Cc: Rob Clark 
> Cc: Ben Skeggs 
> Cc: Tomi Valkeinen 
> Cc: Laurent Pinchart 
> Cc: Kieran Bingham 
> Cc: Sandy Huang 
> Cc: "Heiko Stübner" 
> Cc: Thierry Reding 
> Cc: Jonathan Hunter 
> Cc: Eric Anholt 
> Cc: VMware Graphics 
> Cc: Sinclair Yeh 
> Cc: Thomas Hellstrom 
> Cc: Tony Cheng 
> Cc: Shirish S 
> Cc: Mikita Lipski 
> Cc: Bhawanpreet Lakha 
> Cc: David Francis 
> Cc: Anthony Koo 
> Cc: Jeykumar Sankaran 
> Cc: Jordan Crouse 
> Cc: Bruce Wang 
> Cc: Sravanthi Kollukuduru 
> Cc: Archit Taneja 
> Cc: Steve Kowalik 
> Cc: Carsten Behling 
> Cc: Haneen Mohammed 
> Cc: Daniel Vetter 
> Cc: Rodrigo Siqueira 
> Cc: Mahesh Kumar 
> Cc: amd-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouveau@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-te...@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +--
>  drivers/gpu/drm/arm/malidp_crtc.c |  5 +--
>  .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c|  5 +--
>  drivers/gpu/drm/drm_atomic_state_helper.c | 31 ---
>  drivers/gpu/drm/i915/intel_display.c  |  2 +-
>  drivers/gpu/drm/imx/ipuv3-crtc.c  |  5 +--
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c   |  5 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 12 ++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c |  6 +---
>  drivers/gpu/drm/nouveau/dispnv50/head.c   | 13 ++--
>  drivers/gpu/drm/omapdrm/omap_crtc.c   |  7 ++---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c|  4 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  7 +++--
>  drivers/gpu/drm/tegra/dc.c|  5 +--
>  drivers/gpu/drm/vc4/vc4_crtc.c|  8 ++---
>  drivers/gpu/drm/vkms/vkms_crtc.c  |  7 +
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   |  9 +-
>  include/drm/drm_atomic_state_helper.h |  2 ++
>  18 files changed, 56 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 5064768642f3..770a71726cd1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2802,9 +2802,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc)
>   if (WARN_ON(!state))

Can you give this the same treatment as the other allocation checks?

>   return;
>  
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> -
> + __drm_atomic_helper_crtc_reset(crtc, >base);
>  }
>  
>  static struct drm_crtc_state *
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c 
> b/drivers/gpu/drm/arm/malidp_crtc.c
> index e1b72782848c..9a924ff27148 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -474,10 +474,7 @@ static void malidp_crtc_reset(struct drm_crtc *crtc)
>  
>   kfree(state);
>   state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> - }

You're changing behavior slightly here. If the allocation fails in the old code,
you just continue on (and presumably use-after-free on the next crtc->state
access). Whereas now you're going to just deref NULL. Neither one are really
desireable :)

So you probably want to continue checking the allocation and clear crtc->s

Re: [Nouveau] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Sean Paul]

On Wed, Feb 14, 2018 at 03:43:56PM +0100, Michel Dänzer wrote:
> On 2018-02-14 03:08 PM, Sean Paul wrote:
> > On Wed, Feb 14, 2018 at 10:26:35AM +0100, Maarten Lankhorst wrote:
> >> Op 14-02-18 om 09:46 schreef Lukas Wunner:
> >>> Dear drm-misc maintainers,
> >>>
> >>> On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> >>>> Fix a deadlock on hybrid graphics laptops that's been present since 2013:
> >>> This series has been reviewed, consent has been expressed by the most
> >>> interested parties, patch [1/5] which touches files outside drivers/gpu
> >>> has been acked and I've just out a v2 addressing the only objection
> >>> raised.  My plan is thus to wait another two days for comments and,
> >>> barring further objections, push to drm-misc this weekend.
> >>>
> >>> However I'm struggling with the decision whether to push to next or
> >>> fixes.  The series is marked for stable, however the number of
> >>> affected machines is limited and for an issue that's been present
> >>> for 5 years it probably doesn't matter if it soaks another two months
> >>> in linux-next befor it gets backported.  Hence I tend to err on the
> >>> side of caution and push to next, however a case could be made that
> >>> fixes is more appropriate.
> >>>
> >>> I'm lacking experience making such decisions and would be interested
> >>> to learn how you'd handle this.
> >>>
> >>> Thanks,
> >>>
> >>> Lukas
> >>
> >> I would say fixes, it doesn't look particularly scary. :)
> > 
> > Agreed. If it's good enough for stable, it's good enough for -fixes!
> 
> It's not that simple, is it? Fast-tracking patches (some of which appear
> to be untested) to stable without an immediate cause for urgency seems
> risky to me.
> 

/me should be more careful what he says

Given where we are in the release cycle, it's barely a fast track. If these go
in -fixes, they'll get in -rc2 and will have plenty of time to bake. If we were
at rc5, it might be a different story.

Sean

> 
> -- 
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Sean Paul]

On Wed, Feb 14, 2018 at 10:26:35AM +0100, Maarten Lankhorst wrote:
> Op 14-02-18 om 09:46 schreef Lukas Wunner:
> > Dear drm-misc maintainers,
> >
> > On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> >> Fix a deadlock on hybrid graphics laptops that's been present since 2013:
> > This series has been reviewed, consent has been expressed by the most
> > interested parties, patch [1/5] which touches files outside drivers/gpu
> > has been acked and I've just out a v2 addressing the only objection
> > raised.  My plan is thus to wait another two days for comments and,
> > barring further objections, push to drm-misc this weekend.
> >
> > However I'm struggling with the decision whether to push to next or
> > fixes.  The series is marked for stable, however the number of
> > affected machines is limited and for an issue that's been present
> > for 5 years it probably doesn't matter if it soaks another two months
> > in linux-next befor it gets backported.  Hence I tend to err on the
> > side of caution and push to next, however a case could be made that
> > fixes is more appropriate.
> >
> > I'm lacking experience making such decisions and would be interested
> > to learn how you'd handle this.
> >
> > Thanks,
> >
> > Lukas
> 
> I would say fixes, it doesn't look particularly scary. :)

Agreed. If it's good enough for stable, it's good enough for -fixes!

Sean

> 
> ~Maarten
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH 14/24] drm/nouveau: Merge pre/postclose hooks

[Sean Paul]

On Wed, Mar 08, 2017 at 03:12:47PM +0100, Daniel Vetter wrote:
> Again no apparent explanation for the split except hysterical raisins.
> Merging them also makes it a bit more obviuos what's going on wrt the
> runtime pm refdancing.

Yeah, holding the pm reference from preclose to postclose had me a little
nervous. However, it seems like the reference is just needed for
nouveau_abi16_fini() and nouveau_cli_fini(), as opposed to being needed while
drm_release() is active.

Reviewed-by: Sean Paul <seanp...@chromium.org>

> 
> Cc: Ben Skeggs <bske...@redhat.com>
> Cc: nouveau@lists.freedesktop.org
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index f0bb7606eb8b..0e26be017de7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -881,7 +881,7 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file 
> *fpriv)
>  }
>  
>  static void
> -nouveau_drm_preclose(struct drm_device *dev, struct drm_file *fpriv)
> +nouveau_drm_postclose(struct drm_device *dev, struct drm_file *fpriv)
>  {
>   struct nouveau_cli *cli = nouveau_cli(fpriv);
>   struct nouveau_drm *drm = nouveau_drm(dev);
> @@ -897,12 +897,6 @@ nouveau_drm_preclose(struct drm_device *dev, struct 
> drm_file *fpriv)
>   list_del(>head);
>   mutex_unlock(>client.mutex);
>  
> -}
> -
> -static void
> -nouveau_drm_postclose(struct drm_device *dev, struct drm_file *fpriv)
> -{
> - struct nouveau_cli *cli = nouveau_cli(fpriv);
>   nouveau_cli_fini(cli);
>   kfree(cli);
>   pm_runtime_mark_last_busy(dev->dev);
> @@ -974,7 +968,6 @@ driver_stub = {
>   .load = nouveau_drm_load,
>   .unload = nouveau_drm_unload,
>   .open = nouveau_drm_open,
> - .preclose = nouveau_drm_preclose,
>   .postclose = nouveau_drm_postclose,
>   .lastclose = nouveau_vga_lastclose,
>  
> -- 
> 2.11.0
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [Intel-gfx] [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc.

[Sean Paul]

LL);
> - if (ret)
> - return ret;
> -
> - crtc_state = drm_atomic_get_crtc_state(state, crtc);
> - if (IS_ERR(crtc_state))
> - return PTR_ERR(crtc_state);
> -
> - ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> - if (ret)
> - return ret;
> -
> - crtc_state->active = false;
> -
> - drm_for_each_plane_mask(plane, connector->dev, crtc_state->plane_mask) {
> - plane_state = drm_atomic_get_plane_state(state, plane);
> - if (IS_ERR(plane_state))
> - return PTR_ERR(plane_state);
> -
> - ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> - if (ret)
> - return ret;
> -
> - drm_atomic_set_fb_for_plane(plane_state, NULL);
> - }
> -
> - return 0;
> -}
> -
> -static int
> -nouveau_atomic_disable(struct drm_device *dev,
> -struct drm_modeset_acquire_ctx *ctx)
> -{
> - struct drm_atomic_state *state;
> - struct drm_connector *connector;
> - int ret;
> -
> - state = drm_atomic_state_alloc(dev);
> - if (!state)
> - return -ENOMEM;
> -
> - state->acquire_ctx = ctx;
> -
> - drm_for_each_connector(connector, dev) {
> - ret = nouveau_atomic_disable_connector(state, connector);
> - if (ret)
> - break;
> - }
> -
> - if (ret == 0)
> - ret = drm_atomic_commit(state);
> - drm_atomic_state_put(state);
> - return ret;
> -}
> -
> -static struct drm_atomic_state *
> -nouveau_atomic_suspend(struct drm_device *dev)
> -{
> - struct drm_modeset_acquire_ctx ctx;
> - struct drm_atomic_state *state;
> - int ret;
> -
> - drm_modeset_acquire_init(, 0);
> -
> -retry:
> - ret = drm_modeset_lock_all_ctx(dev, );
> - if (ret < 0) {
> - state = ERR_PTR(ret);
> - goto unlock;
> - }
> -
> - state = drm_atomic_helper_duplicate_state(dev, );
> - if (IS_ERR(state))
> - goto unlock;
> -
> - ret = nouveau_atomic_disable(dev, );
> - if (ret < 0) {
> - drm_atomic_state_put(state);
> - state = ERR_PTR(ret);
> - goto unlock;
> - }
> -
> -unlock:
> - if (PTR_ERR(state) == -EDEADLK) {
> - drm_modeset_backoff();
> - goto retry;
> - }
> -
> - drm_modeset_drop_locks();
> - drm_modeset_acquire_fini();
> - return state;
> -}
> -
>  int
>  nouveau_display_suspend(struct drm_device *dev, bool runtime)
>  {
> @@ -744,7 +633,7 @@ nouveau_display_suspend(struct drm_device *dev, bool 
> runtime)
>  
>   if (drm_drv_uses_atomic_modeset(dev)) {
>   if (!runtime) {
> - disp->suspend = nouveau_atomic_suspend(dev);
> + disp->suspend = drm_atomic_helper_suspend(dev);
>   if (IS_ERR(disp->suspend)) {
>   int ret = PTR_ERR(disp->suspend);
>   disp->suspend = NULL;
> -- 
> 2.7.4
> 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2] drm/nouveau/gr: mark gm107_grctx_generate_tpcid() static

[Sean Paul]

On Mon, Aug 29, 2016 at 9:02 AM, Baoyou Xie  wrote:
> We get 1 warning when build kernel with W=1:
> drivers/gpu/drm/nouveau/nvkm/engine/gr/ctxgm107.c:937:1: warning: no previous 
> prototype for 'gm107_grctx_generate_tpcid' [-Wmissing-prototypes]
>
> In fact, this function is only used in the file in which it is
> declared and don't need a declaration, but can be made static.
> so this patch marks this function with 'static'.
>

It would make things a lot easier if you either consolidated all of
these static changes into one patch, or released them as a series.

Sean


> Signed-off-by: Baoyou Xie 
> ---
>  drivers/gpu/drm/nouveau/nvkm/engine/gr/ctxgm107.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/gr/ctxgm107.c 
> b/drivers/gpu/drm/nouveau/nvkm/engine/gr/ctxgm107.c
> index 6d3c501..4c4b5ab 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/gr/ctxgm107.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/gr/ctxgm107.c
> @@ -933,7 +933,7 @@ gm107_grctx_generate_attrib(struct gf100_grctx *info)
> }
>  }
>
> -void
> +static void
>  gm107_grctx_generate_tpcid(struct gf100_gr *gr)
>  {
> struct nvkm_device *device = gr->base.engine.subdev.device;
> --
> 2.7.4
>
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau