date:20211013

Re: [PATCH] drm/bridge: Ignore -EPROBE_DEFER when bridge attach fails

2021-10-13 Thread Guido Günther

Hi,
On Wed, Oct 13, 2021 at 08:48:32AM +0200, Andrzej Hajda wrote:
> On 12.10.2021 22:47, Guido Günther wrote:
> > Hi Laurent,
> > On Tue, Oct 12, 2021 at 11:17:07PM +0300, Laurent Pinchart wrote:
> > > Hi Guido,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Tue, Oct 12, 2021 at 09:58:58PM +0200, Guido Günther wrote:
> > > > Otherwise logs are filled with
> > > > 
> > > >[drm:drm_bridge_attach] *ERROR* failed to attach bridge 
> > > > /soc@0/bus@3080/mipi-dsi@30a0  to encoder None-34: -517
> > > > 
> > > > when the bridge isn't ready yet.
> > > > 
> > > > Fixes: fb8d617f8fd6 ("drm/bridge: Centralize error message when bridge 
> > > > attach fails")
> > > > Signed-off-by: Guido Günther 
> > > > ---
> > > >   drivers/gpu/drm/drm_bridge.c | 11 ++-
> > > >   1 file changed, 6 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > > index a8ed66751c2d..f0508e85ae98 100644
> > > > --- a/drivers/gpu/drm/drm_bridge.c
> > > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > > @@ -227,14 +227,15 @@ int drm_bridge_attach(struct drm_encoder 
> > > > *encoder, struct drm_bridge *bridge,
> > > > bridge->encoder = NULL;
> > > > list_del(&bridge->chain_node);
> > > > +   if (ret != -EPROBE_DEFER) {
> > > >   #ifdef CONFIG_OF
> > > > -   DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
> > > > - bridge->of_node, encoder->name, ret);
> > > > +   DRM_ERROR("failed to attach bridge %pOF to encoder %s: 
> > > > %d\n",
> > > > + bridge->of_node, encoder->name, ret);
> > > >   #else
> > > > -   DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
> > > > - encoder->name, ret);
> > > > +   DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
> > > > + encoder->name, ret);
> > > >   #endif
> > > > -
> > > > +   }
> > > 
> > > This looks fine as such, but I'm concerned about the direction it's
> > > taking. Ideally, probe deferral should happen at probe time, way before
> > > the bridge is attached. Doing otherwise is a step in the wrong direction
> > > in my opinion, and something we'll end up regretting when we'll feel the
> > > pain it inflicts.
> > 
> > The particular case I'm seeing this is the nwl driver probe deferrals if
> > the panel bridge isn't ready (which needs a bunch of components
> > (dsi, panel, backlight wrapped led, ...) and it probes fine later on so I
> > wonder where you see the actual error cause? That downstream of the
> > bridge isn't ready or that the display controller is already attaching
> > the bridge?
> 
> So it is something wrong there, nwl should not publish bridge interface
> until it gather its resources (the panel in this case).

That helps, I'll look at that. Thanks!
 -- Guido

> 
> Regards
> Andrzej
> 
> 
> > 
> > Cheers,
> >   -- Guido
> > 
> > > 
> > > > return ret;
> > > >   }
> > > >   EXPORT_SYMBOL(drm_bridge_attach);
> > > 
> > > -- 
> > > Regards,
> > > 
> > > Laurent Pinchart
> > > 
>

Re: [RFC v2 01/22] drm: RFC for Plane Color Hardware Pipeline

2021-10-13 Thread Pekka Paalanen

On Tue, 12 Oct 2021 19:11:29 +
"Shankar, Uma"  wrote:

> > -Original Message-
> > From: Pekka Paalanen 
> > Sent: Tuesday, October 12, 2021 5:30 PM
> > To: Simon Ser 
> > Cc: Shankar, Uma ; intel-...@lists.freedesktop.org; 
> > dri-
> > de...@lists.freedesktop.org; harry.wentl...@amd.com;
> > ville.syrj...@linux.intel.com; brian.star...@arm.com;
> > sebast...@sebastianwick.net; shashank.sha...@amd.com
> > Subject: Re: [RFC v2 01/22] drm: RFC for Plane Color Hardware Pipeline
> > 
> > On Tue, 12 Oct 2021 10:35:37 +
> > Simon Ser  wrote:
> >   
> > > On Tuesday, October 12th, 2021 at 12:30, Pekka Paalanen  
> >  wrote:  
> > >  
> > > > is there a practise of landing proposal documents in the kernel? How
> > > > does that work, will a kernel tree carry the patch files?
> > > > Or should this document be worded like documentation for an accepted
> > > > feature, and then the patches either land or don't?  
> > >
> > > Once everyone agrees, the RFC can land. I don't think a kernel tree is
> > > necessary. See:
> > >
> > > https://dri.freedesktop.org/docs/drm/gpu/rfc/index.html  
> > 
> > Does this mean the RFC doc patch will land, but the code patches will 
> > remain in the
> > review cycles waiting for userspace proving vehicles?
> > Rather than e.g. committed as files that people would need to apply 
> > themselves? Or
> > how does one find the code patches corresponding to RFC docs?  
> 
> As I understand, this section was added to finalize the design and debate on 
> the UAPI,
> structures, headers and design etc. Once a general agreement is in place with 
> all the
> stakeholders, we can have ack on design and approach and get it merged. This 
> hence
> serves as an approved reference for the UAPI, accepted and agreed by 
> community at large.
> 
> Once the code lands, all the documentation will be added to the right driver 
> sections and
> helpers, like it's been done currently.

I'm just wondering: someone browses a kernel tree, and discovers this
RFC doc in there. They want to see or test the latest (WIP) kernel
implementation of it. How will they find the code / patches?


Thanks,
pq


pgpOXzW3pj3W9.pgp
Description: OpenPGP digital signature

[PATCH v3] lib/stackdepot: allow optional init and stack_table allocation by kvmalloc()

2021-10-13 Thread Vlastimil Babka

Currently, enabling CONFIG_STACKDEPOT means its stack_table will be allocated
from memblock, even if stack depot ends up not actually used. The default size
of stack_table is 4MB on 32-bit, 8MB on 64-bit.

This is fine for use-cases such as KASAN which is also a config option and
has overhead on its own. But it's an issue for functionality that has to be
actually enabled on boot (page_owner) or depends on hardware (GPU drivers)
and thus the memory might be wasted. This was raised as an issue [1] when
attempting to add stackdepot support for SLUB's debug object tracking
functionality. It's common to build kernels with CONFIG_SLUB_DEBUG and enable
slub_debug on boot only when needed, or create only specific kmem caches with
debugging for testing purposes.

It would thus be more efficient if stackdepot's table was allocated only when
actually going to be used. This patch thus makes the allocation (and whole
stack_depot_init() call) optional:

- Add a CONFIG_STACKDEPOT_ALWAYS_INIT flag to keep using the current
  well-defined point of allocation as part of mem_init(). Make CONFIG_KASAN
  select this flag.
- Other users have to call stack_depot_init() as part of their own init when
  it's determined that stack depot will actually be used. This may depend on
  both config and runtime conditions. Convert current users which are
  page_owner and several in the DRM subsystem. Same will be done for SLUB
  later.
- Because the init might now be called after the boot-time memblock allocation
  has given all memory to the buddy allocator, change stack_depot_init() to
  allocate stack_table with kvmalloc() when memblock is no longer available.
  Also handle allocation failure by disabling stackdepot (could have
  theoretically happened even with memblock allocation previously), and don't
  unnecessarily align the memblock allocation to its own size anymore.

[1] 
https://lore.kernel.org/all/CAMuHMdW=eovzm1re5fvoen87nkfilmm2+ah7enu2kxehcvb...@mail.gmail.com/

Signed-off-by: Vlastimil Babka 
Acked-by: Dmitry Vyukov 
Reviewed-by: Marco Elver  # stackdepot
Cc: Marco Elver 
Cc: Vijayanand Jitta 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Thomas Zimmermann 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Andrey Ryabinin 
Cc: Alexander Potapenko 
Cc: Andrey Konovalov 
Cc: Dmitry Vyukov 
Cc: Geert Uytterhoeven 
Cc: Oliver Glitta 
Cc: Imran Khan 
---
Changes in v3:
- stack_depot_init_mutex made static and moved inside stack_depot_init()
  Reported-by: kernel test robot 
- use !stack_table condition instead of stack_table == NULL
  reported by checkpatch on freedesktop.org patchwork
 drivers/gpu/drm/drm_dp_mst_topology.c   |  1 +
 drivers/gpu/drm/drm_mm.c|  4 +++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
 include/linux/stackdepot.h  | 25 ---
 init/main.c |  2 +-
 lib/Kconfig |  4 +++
 lib/Kconfig.kasan   |  2 +-
 lib/stackdepot.c| 33 +
 mm/page_owner.c |  2 ++
 9 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 86d13d6bc463..b0ebdc843a00 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -5493,6 +5493,7 @@ int drm_dp_mst_topology_mgr_init(struct 
drm_dp_mst_topology_mgr *mgr,
mutex_init(&mgr->probe_lock);
 #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
mutex_init(&mgr->topology_ref_history_lock);
+   stack_depot_init();
 #endif
INIT_LIST_HEAD(&mgr->tx_msg_downq);
INIT_LIST_HEAD(&mgr->destroy_port_list);
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 93d48a6f04ab..5916228ea0c9 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -983,6 +983,10 @@ void drm_mm_init(struct drm_mm *mm, u64 start, u64 size)
add_hole(&mm->head_node);
 
mm->scan_active = 0;
+
+#ifdef CONFIG_DRM_DEBUG_MM
+   stack_depot_init();
+#endif
 }
 EXPORT_SYMBOL(drm_mm_init);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index eaf7688f517d..d083506986e1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -78,6 +78,9 @@ static void __print_depot_stack(depot_stack_handle_t stack,
 static void init_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm)
 {
spin_lock_init(&rpm->debug.lock);
+
+   if (rpm->available)
+   stack_depot_init();
 }
 
 static noinline depot_stack_handle_t
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index 6bb4bc1a5f54..40fc5e92194f 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -13,6 +13,22 @@
 
 typedef u32 depot_stack_handle_t;
 
+/*
+ * Every user of stack depot has to call this during its own init when it's
+ * decided that

Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings

2021-10-13 Thread Maxime Ripard

Hi,

On Tue, Oct 12, 2021 at 08:48:42AM +0200, Alexander Stein wrote:
> Add a VCC regulator which needs to be enabled before the EN pin is
> released.
> 
> Reviewed-by: Sam Ravnborg 
> Signed-off-by: Alexander Stein 
> ---
>  .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml 
> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> index a5779bf17849..49ace6f312d5 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> @@ -32,6 +32,9 @@ properties:
>  maxItems: 1
>  description: GPIO specifier for bridge_en pin (active high).
>  
> +  vcc-supply:
> +description: A 1.8V power supply (see regulator/regulator.yaml).
> +
>ports:
>  $ref: /schemas/graph.yaml#/properties/ports
>  
> @@ -93,6 +96,7 @@ properties:
>  required:
>- compatible
>- reg
> +  - vcc-supply

This isn't a backward-compatible change. All the previous users of that
binding will now require a vcc-supply property even though it was
working fine for them before.

You handle that nicely in the code, but you can't make that new property
required.

Maxime


signature.asc
Description: PGP signature

[PATCH] drm/bridge: display-connector: fix an uninitialized pointer in probe()

2021-10-13 Thread Dan Carpenter

The "label" pointer is used for debug output.  The code assumes that it
is either NULL or valid, but it is never set to NULL.  It is either
valid or uninitialized.

Fixes: 0c275c30176b ("drm/bridge: Add bridge driver for display connectors")
Signed-off-by: Dan Carpenter 
---
 drivers/gpu/drm/bridge/display-connector.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/display-connector.c 
b/drivers/gpu/drm/bridge/display-connector.c
index 05eb759da6fc..847a0dce7f1d 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -107,7 +107,7 @@ static int display_connector_probe(struct platform_device 
*pdev)
 {
struct display_connector *conn;
unsigned int type;
-   const char *label;
+   const char *label = NULL;
int ret;
 
conn = devm_kzalloc(&pdev->dev, sizeof(*conn), GFP_KERNEL);
-- 
2.20.1

[PATCH 1/2] drm/msm: fix potential NULL dereference in cleanup

2021-10-13 Thread Dan Carpenter

The "msm_obj->node" list needs to be initialized earlier so that the
list_del() in msm_gem_free_object() doesn't experience a NULL pointer
dereference.

Fixes: 6ed0897cd800 ("drm/msm: Fix debugfs deadlock")
Signed-off-by: Dan Carpenter 
---
 drivers/gpu/drm/msm/msm_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 40a9863f5951..49185d524be3 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1132,6 +1132,7 @@ static int msm_gem_new_impl(struct drm_device *dev,
msm_obj->flags = flags;
msm_obj->madv = MSM_MADV_WILLNEED;
 
+   INIT_LIST_HEAD(&msm_obj->node);
INIT_LIST_HEAD(&msm_obj->vmas);
 
*obj = &msm_obj->base;
-- 
2.20.1

[PATCH 2/2] drm/msm: uninitialized variable in msm_gem_import()

2021-10-13 Thread Dan Carpenter

The msm_gem_new_impl() function cleans up after itself so there is no
need to call drm_gem_object_put().  Conceptually, it does not make sense
to call a kref_put() function until after the reference counting has
been initialized which happens immediately after this call in the
drm_gem_(private_)object_init() functions.

In the msm_gem_import() function the "obj" pointer is uninitialized, so
it will lead to a crash.

Fixes: 05b849111c07 ("drm/msm: prime support")
Signed-off-by: Dan Carpenter 
---
 drivers/gpu/drm/msm/msm_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 49185d524be3..0e491cd21c53 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1167,7 +1167,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device 
*dev, uint32_t size, uint32
 
ret = msm_gem_new_impl(dev, size, flags, &obj);
if (ret)
-   goto fail;
+   return ERR_PTR(ret);
 
msm_obj = to_msm_bo(obj);
 
@@ -1251,7 +1251,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device 
*dev,
 
ret = msm_gem_new_impl(dev, size, MSM_BO_WC, &obj);
if (ret)
-   goto fail;
+   return ERR_PTR(ret);
 
drm_gem_private_object_init(dev, obj, size);
 
-- 
2.20.1

Re: [RFC v2 01/22] drm: RFC for Plane Color Hardware Pipeline

2021-10-13 Thread Pekka Paalanen

On Tue, 12 Oct 2021 20:58:27 +
"Shankar, Uma"  wrote:

> > -Original Message-
> > From: Pekka Paalanen 
> > Sent: Tuesday, October 12, 2021 4:01 PM
> > To: Shankar, Uma 
> > Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
> > harry.wentl...@amd.com; ville.syrj...@linux.intel.com; 
> > brian.star...@arm.com; sebast...@sebastianwick.net; 
> > shashank.sha...@amd.com
> > Subject: Re: [RFC v2 01/22] drm: RFC for Plane Color Hardware Pipeline
> > 
> > On Tue,  7 Sep 2021 03:08:43 +0530
> > Uma Shankar  wrote:
> >   
> > > This is a RFC proposal for plane color hardware blocks.
> > > It exposes the property interface to userspace and calls out the 
> > > details or interfaces created and the intended purpose.
> > >
> > > Credits: Ville Syrjälä 
> > > Signed-off-by: Uma Shankar 
> > > ---
> > >  Documentation/gpu/rfc/drm_color_pipeline.rst | 167
> > > +++
> > >  1 file changed, 167 insertions(+)
> > >  create mode 100644 Documentation/gpu/rfc/drm_color_pipeline.rst
> > >
> > > diff --git a/Documentation/gpu/rfc/drm_color_pipeline.rst
> > > b/Documentation/gpu/rfc/drm_color_pipeline.rst
> > > new file mode 100644
> > > index ..0d1ca858783b
> > > --- /dev/null
> > > +++ b/Documentation/gpu/rfc/drm_color_pipeline.rst
> > > @@ -0,0 +1,167 @@
> > > +==
> > > +Display Color Pipeline: Proposed DRM Properties  

...

> > > +Proposal is to have below properties for a plane:
> > > +
> > > +* Plane Degamma or Pre-Curve:
> > > + * This will be used to linearize the input framebuffer data.
> > > + * It will apply the reverse of the color transfer function.
> > > + * It can be a degamma curve or OETF for HDR.  
> > 
> > As you want to produce light-linear values, you use EOTF or inverse OETF.
> > 
> > The term OETF has a built-in assumption that that happens in a camera:
> > it takes in light and produces and electrical signal. Lately I have 
> > personally started talking about non-linear encoding of color values, 
> > since EOTF is often associated with displays if nothing else is said 
> > (taking in an electrical signal and producing light).
> > 
> > So this would be decoding the color values into light-linear color 
> > values. That is what an EOTF does, yes, but I feel there is a nuanced 
> > difference. A piece of equipment implements an EOTF by turning an 
> > electrical signal into light, hence EOTF often refers to specific 
> > equipment. You could talk about content EOTF to denote content value 
> > encoding, as opposed to output or display EOTF, but that might be 
> > confusing if you look at e.g. the diagrams in BT.2100: is it the EOTF or is 
> > it the inverse OETF? Is the (inverse?) OOTF included?
> > 
> > So I try to side-step those questions by talking about encoding.  
> 
> The idea here is that frame buffer presented to display plane engine will be 
> non-linear.
> So output of a media decode should result in content with EOTF applied.

Hi,

sure, but the question is: which EOTF. There can be many different
things called "EOTF" in a single pipeline, and then it's up to the
document writer to make the difference between them. Comparing two
documents with different conventions causes a lot of confusion in my
personal experience, so it is good to define the concepts more
carefully.

> So output of a media decode should result in content with EOTF applied.

I suspect you have it backwards. Media decode produces electrical
(non-linear) pixel color values. If EOTF was applied, they would be
linear instead (and require more memory to achieve the same visual
precision).

If you want to put it this way, you could say "with inverse EOTF
applied", but that might be slightly confusing because it is already
baked in to the video, it's not something a media decoder has to
specifically apply, I think. However, the (inverse) EOTF in this case
is the content EOTF, not the display EOTF.

If content and display EOTF differ, then one must apply first content
EOTF and then inverse display EOTF to get values that are correctly
encoded for the display. (This is necessary but not sufficient in
general.) Mind, that this is not an OOTF nor an artistic adjustment,
this is purely a value encoding conversion.

> Playback transfer function (EOTF): inverse OETF plus rendering intent gamma. 

Does "rendering intent gamma" refer to artistic adjustments, not OOTF?

cf. BT.2100 Annex 1, "The relationship between the OETF, the EOTF and
the OOTF", although I find those diagrams somewhat confusing still. It
does not seem to clearly account for transmission non-linear encoding
being different from the display EOTF.

Different documents use OOTF to refer to different things. Then there
is also the fundamental difference between PQ and HLG systems, where
OOTF is by definition in different places of the
camera-transmission-display pipeline.

> 
> To make it linear, we should apply the OETF. Confusion is whether OETF is 
> equivalent to
> in

AW: (EXT) Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support

2021-10-13 Thread Alexander Stein

Hello Laurent,

On Tue, Oct 12, 2021 at 10:43 +0200, Laurent Pinchart wrote:
> On Tue, Oct 12, 2021 at 08:48:43AM +0200, Alexander Stein wrote:
> > VCC needs to be enabled before releasing the enable GPIO.
> > 
> > Reviewed-by: Sam Ravnborg 
> > Signed-off-by: Alexander Stein 
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c 
> > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index 9072342566f3..a6b1fd71dfee 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -143,6 +144,7 @@ struct sn65dsi83 {
> > struct mipi_dsi_device  *dsi;
> > struct drm_bridge   *panel_bridge;
> > struct gpio_desc*enable_gpio;
> > +   struct regulator*vcc;
> > int dsi_lanes;
> > boollvds_dual_link;
> > boollvds_dual_link_even_odd_swap;
> > @@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx,
> enum sn65dsi83_model model)
> >  
> > ctx->panel_bridge = panel_bridge;
> >  
> > +   ctx->vcc = devm_regulator_get(dev, "vcc");
> > +   if (IS_ERR(ctx->vcc))
> > +   return dev_err_probe(dev, PTR_ERR(ctx->vcc),
> > +"Failed to get supply 'vcc': %pe\n",
> > +ctx->vcc);
> > +
> > return 0;
> >  }
> >  
> > @@ -691,7 +699,11 @@ static int sn65dsi83_probe(struct i2c_client *client,
> > ctx->bridge.of_node = dev->of_node;
> > drm_bridge_add(&ctx->bridge);
> >  
> > -   return 0;
> > +   ret = regulator_enable(ctx->vcc);
> > +   if (ret)
> > +   dev_err(dev, "Failed to enable vcc: %i\n", ret);
> 
> I think this should move to sn65dsi83_atomic_pre_enable() (and similarly
> for regulator_disable()) as keeping the regulator enabled at all times
> will cost power.

I get your idea. The thing is that unless 1V8 is provided the bridge is not
even accessible on I2C. So any access to sn65dsi83.regmap without the vcc
regulator enabled will fail. AFAICS this is not an issue right now, as regmap
is only used in sn65dsi83_atomic_enable(), sn65dsi83_atomic_disable() and
sn65dsi83_atomic_pre_enable(), so your sugestion would work, but I'm
hestitating a bit. The driver then has to ensure all regmap uses are done
only when vcc is enabled.

Best regards,
Alexander

Re: (EXT) Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support

2021-10-13 Thread Laurent Pinchart

Hi Alexander,

On Wed, Oct 13, 2021 at 08:59:22AM +, Alexander Stein wrote:
> On Tue, Oct 12, 2021 at 10:43 +0200, Laurent Pinchart wrote:
> > On Tue, Oct 12, 2021 at 08:48:43AM +0200, Alexander Stein wrote:
> > > VCC needs to be enabled before releasing the enable GPIO.
> > > 
> > > Reviewed-by: Sam Ravnborg 
> > > Signed-off-by: Alexander Stein 
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c 
> > > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > index 9072342566f3..a6b1fd71dfee 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > @@ -33,6 +33,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include 
> > >  #include 
> > > @@ -143,6 +144,7 @@ struct sn65dsi83 {
> > >   struct mipi_dsi_device  *dsi;
> > >   struct drm_bridge   *panel_bridge;
> > >   struct gpio_desc*enable_gpio;
> > > + struct regulator*vcc;
> > >   int dsi_lanes;
> > >   boollvds_dual_link;
> > >   boollvds_dual_link_even_odd_swap;
> > > @@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx,
> > enum sn65dsi83_model model)
> > >  
> > >   ctx->panel_bridge = panel_bridge;
> > >  
> > > + ctx->vcc = devm_regulator_get(dev, "vcc");
> > > + if (IS_ERR(ctx->vcc))
> > > + return dev_err_probe(dev, PTR_ERR(ctx->vcc),
> > > +  "Failed to get supply 'vcc': %pe\n",
> > > +  ctx->vcc);
> > > +
> > >   return 0;
> > >  }
> > >  
> > > @@ -691,7 +699,11 @@ static int sn65dsi83_probe(struct i2c_client *client,
> > >   ctx->bridge.of_node = dev->of_node;
> > >   drm_bridge_add(&ctx->bridge);
> > >  
> > > - return 0;
> > > + ret = regulator_enable(ctx->vcc);
> > > + if (ret)
> > > + dev_err(dev, "Failed to enable vcc: %i\n", ret);
> > 
> > I think this should move to sn65dsi83_atomic_pre_enable() (and similarly
> > for regulator_disable()) as keeping the regulator enabled at all times
> > will cost power.
> 
> I get your idea. The thing is that unless 1V8 is provided the bridge is not
> even accessible on I2C. So any access to sn65dsi83.regmap without the vcc
> regulator enabled will fail. AFAICS this is not an issue right now, as regmap
> is only used in sn65dsi83_atomic_enable(), sn65dsi83_atomic_disable() and
> sn65dsi83_atomic_pre_enable(), so your sugestion would work, but I'm
> hestitating a bit. The driver then has to ensure all regmap uses are done
> only when vcc is enabled.

Correct, and that's the usual pattern, drivers need to call
pm_runtime_get_sync() before accessing registers. For all you know, even
if the power to the bridge is on, the I2C controller it is connected to
could be suspended.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings

2021-10-13 Thread Laurent Pinchart

Hi Maxime,

On Wed, Oct 13, 2021 at 09:47:22AM +0200, Maxime Ripard wrote:
> On Tue, Oct 12, 2021 at 08:48:42AM +0200, Alexander Stein wrote:
> > Add a VCC regulator which needs to be enabled before the EN pin is
> > released.
> > 
> > Reviewed-by: Sam Ravnborg 
> > Signed-off-by: Alexander Stein 
> > ---
> >  .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml 
> > b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> > index a5779bf17849..49ace6f312d5 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> > @@ -32,6 +32,9 @@ properties:
> >  maxItems: 1
> >  description: GPIO specifier for bridge_en pin (active high).
> >  
> > +  vcc-supply:
> > +description: A 1.8V power supply (see regulator/regulator.yaml).
> > +
> >ports:
> >  $ref: /schemas/graph.yaml#/properties/ports
> >  
> > @@ -93,6 +96,7 @@ properties:
> >  required:
> >- compatible
> >- reg
> > +  - vcc-supply
> 
> This isn't a backward-compatible change. All the previous users of that
> binding will now require a vcc-supply property even though it was
> working fine for them before.
> 
> You handle that nicely in the code, but you can't make that new property
> required.

We can't make it required in the driver, but can't we make it required
in the bindings ? This indicates that all new DTs need to set the
property. We also need to mass-patch the in-tree DTs to avoid validation
failures, but apart from that, I don't see any issue.

-- 
Regards,

Laurent Pinchart

[PATCH] drm/i915: Use dma_resv_iter for waiting in i915_gem_object_wait_reservation.

2021-10-13 Thread Maarten Lankhorst

No memory should be allocated when calling i915_gem_object_wait,
because it may be called to idle a BO when evicting memory.

Fix this by using dma_resv_iter helpers to call
i915_gem_object_wait_fence() on each fence, which cleans up the code a lot.
Also remove dma_resv_prune, it's questionably.

This will result in the following lockdep splat.

<4> [83.538517] ==
<4> [83.538520] WARNING: possible circular locking dependency detected
<4> [83.538522] 5.15.0-rc5-CI-Trybot_8062+ #1 Not tainted
<4> [83.538525] --
<4> [83.538527] gem_render_line/5242 is trying to acquire lock:
<4> [83.538530] 8275b1e0 (fs_reclaim){+.+.}-{0:0}, at: 
__kmalloc_track_caller+0x56/0x270
<4> [83.538538]
but task is already holding lock:
<4> [83.538540] 88813471d1e0 (&vm->mutex/1){+.+.}-{3:3}, at: 
i915_vma_pin_ww+0x1c7/0x970 [i915]
<4> [83.538638]
which lock already depends on the new lock.
<4> [83.538642]
the existing dependency chain (in reverse order) is:
<4> [83.538645]
-> #1 (&vm->mutex/1){+.+.}-{3:3}:
<4> [83.538649]lock_acquire+0xd3/0x310
<4> [83.538654]i915_gem_shrinker_taints_mutex+0x2d/0x50 [i915]
<4> [83.538730]i915_address_space_init+0xf5/0x1b0 [i915]
<4> [83.538794]ppgtt_init+0x55/0x70 [i915]
<4> [83.538856]gen8_ppgtt_create+0x44/0x5d0 [i915]
<4> [83.538912]i915_ppgtt_create+0x28/0xf0 [i915]
<4> [83.538971]intel_gt_init+0x130/0x3b0 [i915]
<4> [83.539029]i915_gem_init+0x14b/0x220 [i915]
<4> [83.539100]i915_driver_probe+0x97e/0xdd0 [i915]
<4> [83.539149]i915_pci_probe+0x43/0x1d0 [i915]
<4> [83.539197]pci_device_probe+0x9b/0x110
<4> [83.539201]really_probe+0x1b0/0x3b0
<4> [83.539205]__driver_probe_device+0xf6/0x170
<4> [83.539208]driver_probe_device+0x1a/0x90
<4> [83.539210]__driver_attach+0x93/0x160
<4> [83.539213]bus_for_each_dev+0x72/0xc0
<4> [83.539216]bus_add_driver+0x14b/0x1f0
<4> [83.539220]driver_register+0x66/0xb0
<4> [83.539222]hdmi_get_spk_alloc+0x1f/0x50 [snd_hda_codec_hdmi]
<4> [83.539227]do_one_initcall+0x53/0x2e0
<4> [83.539230]do_init_module+0x55/0x200
<4> [83.539234]load_module+0x2700/0x2980
<4> [83.539237]__do_sys_finit_module+0xaa/0x110
<4> [83.539241]do_syscall_64+0x37/0xb0
<4> [83.539244]entry_SYSCALL_64_after_hwframe+0x44/0xae
<4> [83.539247]
-> #0 (fs_reclaim){+.+.}-{0:0}:
<4> [83.539251]validate_chain+0xb37/0x1e70
<4> [83.539254]__lock_acquire+0x5a1/0xb70
<4> [83.539258]lock_acquire+0xd3/0x310
<4> [83.539260]fs_reclaim_acquire+0x9d/0xd0
<4> [83.539264]__kmalloc_track_caller+0x56/0x270
<4> [83.539267]krealloc+0x48/0xa0
<4> [83.539270]dma_resv_get_fences+0x1c3/0x280
<4> [83.539274]i915_gem_object_wait+0x1ff/0x410 [i915]
<4> [83.539342]i915_gem_evict_for_node+0x16b/0x440 [i915]
<4> [83.539412]i915_gem_gtt_reserve+0xff/0x130 [i915]
<4> [83.539482]i915_vma_pin_ww+0x765/0x970 [i915]
<4> [83.539556]eb_validate_vmas+0x6fe/0x8e0 [i915]
<4> [83.539626]i915_gem_do_execbuffer+0x9a6/0x20a0 [i915]
<4> [83.539693]i915_gem_execbuffer2_ioctl+0x11f/0x2c0 [i915]
<4> [83.539759]drm_ioctl_kernel+0xac/0x140
<4> [83.539763]drm_ioctl+0x201/0x3d0
<4> [83.539766]__x64_sys_ioctl+0x6a/0xa0
<4> [83.539769]do_syscall_64+0x37/0xb0
<4> [83.539772]entry_SYSCALL_64_after_hwframe+0x44/0xae
<4> [83.539775]
other info that might help us debug this:
<4> [83.539778]  Possible unsafe locking scenario:
<4> [83.539781]CPU0CPU1
<4> [83.539783]
<4> [83.539785]   lock(&vm->mutex/1);
<4> [83.539788]lock(fs_reclaim);
<4> [83.539791]lock(&vm->mutex/1);
<4> [83.539794]   lock(fs_reclaim);
<4> [83.539796]
 *** DEADLOCK ***
<4> [83.539799] 3 locks held by gem_render_line/5242:
<4> [83.539802]  #0: c9d4bbf0 
(reservation_ww_class_acquire){+.+.}-{0:0}, at: 
i915_gem_do_execbuffer+0x8e5/0x20a0 [i915]
<4> [83.539870]  #1: 88811e48bae8 (reservation_ww_class_mutex){+.+.}-{3:3}, 
at: eb_validate_vmas+0x81/0x8e0 [i915]
<4> [83.539936]  #2: 88813471d1e0 (&vm->mutex/1){+.+.}-{3:3}, at: 
i915_vma_pin_ww+0x1c7/0x970 [i915]
<4> [83.540011]
stack backtrace:
<4> [83.540014] CPU: 2 PID: 5242 Comm: gem_render_line Not tainted 
5.15.0-rc5-CI-Trybot_8062+ #1
<4> [83.540019] Hardware name: Intel(R) Client Systems NUC11TNHi3/NUC11TNBi3, 
BIOS TNTGL357.0038.2020.1124.1648 11/24/2020
<4> [83.540023] Call Trace:
<4> [83.540026]  dump_stack_lvl+0x56/0x7b
<4> [83.540030]  check_noncircular+0x12e/0x150
<4> [83.540034]  ? _raw_spin_unlock_irqrestore+0x50/0x60
<4> [83.540038]  validate_chain+0xb37/0x1e70
<4> [83.540042]  __lock_acquire+0x5a1/0xb70
<4> [83.540046]  lock_acquire+0

Re: mmotm 2021-10-05-19-53 uploaded (drivers/gpu/drm/msm/hdmi/hdmi_phy.o)

2021-10-13 Thread Arnd Bergmann

On Thu, Oct 7, 2021 at 11:51 AM Geert Uytterhoeven  wrote:
> On Wed, Oct 6, 2021 at 9:28 AM Christian König  
> wrote:
> > Am 06.10.21 um 09:20 schrieb Stephen Rothwell:
> > > On Tue, 5 Oct 2021 22:48:03 -0700 Randy Dunlap  
> > > wrote:
> > >> on i386:
> > >>
> > >> ld: drivers/gpu/drm/msm/hdmi/hdmi_phy.o:(.rodata+0x3f0): undefined 
> > >> reference to `msm_hdmi_phy_8996_cfg'

I ran into the same thing now as well.
E_TEST) && COMMON_CLK
>
> I'd make that:
>
> -depends on DRM
> +   depends on COMMON_CLK && DRM && IOMMU_SUPPORT
> depends on ARCH_QCOM || SOC_IMX5 || COMPILE_TEST
> -depends on IOMMU_SUPPORT
> -   depends on (OF && COMMON_CLK) || COMPILE_TEST
> +   depends on OF || COMPILE_TEST
>
> to keep a better separation between hard and soft dependencies.
>
> Note that the "depends on OF || COMPILE_TEST" can even be
> deleted, as the dependency on ARCH_QCOM || SOC_IMX5 implies OF.

Looks good to me, I would also drop that last line in this case, and maybe
add this change as building without COMMON_CLK is no longer possible:

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 904535eda0c4..a5d87e03812f 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -116,10 +116,10 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
  dp/dp_power.o \
  dp/dp_audio.o

-msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
-msm-$(CONFIG_COMMON_CLK) += disp/mdp4/mdp4_lvds_pll.o
-msm-$(CONFIG_COMMON_CLK) += hdmi/hdmi_pll_8960.o
-msm-$(CONFIG_COMMON_CLK) += hdmi/hdmi_phy_8996.o
+msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o \
+ disp/mdp4/mdp4_lvds_pll.o \
+ hdmi/hdmi_pll_8960.o \
+ hdmi/hdmi_phy_8996.o

 msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o

Has anyone submitted a patch already, or should I send the version
that I am using locally now?

Arnd

Re: [PATCH v7, 00/15] Support multi hardware decode using of_platform_populate

2021-10-13 Thread Andrzej Pietrasiewicz

Hi,

W dniu 13.10.2021 o 03:08, yunfei.d...@mediatek.com pisze:

Hi Andrzej,

On Tue, 2021-10-12 at 16:27 +0200, Andrzej Pietrasiewicz wrote:

Hi Yunfei Dong,

W dniu 11.10.2021 o 09:02, Yunfei Dong pisze:

This series adds support for multi hardware decode into mtk-vcodec,
by first
adding use of_platform_populate to manage each hardware
information: interrupt,
clock, register bases and power. Secondly add core thread to deal
with core
hardware message, at the same time, add msg queue for different
hardware
share messages. Lastly, the architecture of different specs are not
the same,
using specs type to separate them.

This series has been tested with both MT8183 and MT8173. Decoding
was working
for both chips.

Patches 1~3 rewrite get register bases and power on/off interface.

Patch 4 add to support multi hardware.

Patch 5 separate video encoder and decoder document

Patches 6-15 add interfaces to support core hardware.

Which tree does the series apply to?

I don't understand your mean clearly. Media tree?

You can get the patches from this link:

https://patchwork.linuxtv.org/project/linux-media/cover/20211011070247.792-1-yunfei.d...@mediatek.com/

Here's what I get:

$ git remote update media_tree
Fetching media_tree

$ git branch
master
* media_tree
mediatek-master

$ git-pw --server https://patchwork.linuxtv.org/api/1.1 --project linux-media
series apply 6465 -3

Failed to apply patch:
Applying: media: mtk-vcodec: Get numbers of register bases from DT
Applying: media: mtk-vcodec: Align vcodec wake up interrupt interface
Applying: media: mtk-vcodec: Refactor vcodec pm interface
Applying: media: mtk-vcodec: Manage multi hardware information
error: sha1 information is lacking or useless
(drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c).

error: could not build fake ancestor
Patch failed at 0004 media: mtk-vcodec: Manage multi hardware information
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Regards,

Andrzej

[PATCH RFC] virtio: wrap config->reset calls

2021-10-13 Thread Michael S. Tsirkin

This will enable cleanups down the road.
The idea is to disable cbs, then add "flush_queued_cbs" callback
as a parameter, this way drivers can flush any work
queued after callbacks have been disabled.

Signed-off-by: Michael S. Tsirkin 
---
 arch/um/drivers/virt-pci.c | 2 +-
 drivers/block/virtio_blk.c | 4 ++--
 drivers/bluetooth/virtio_bt.c  | 2 +-
 drivers/char/hw_random/virtio-rng.c| 2 +-
 drivers/char/virtio_console.c  | 4 ++--
 drivers/crypto/virtio/virtio_crypto_core.c | 8 
 drivers/firmware/arm_scmi/virtio.c | 2 +-
 drivers/gpio/gpio-virtio.c | 2 +-
 drivers/gpu/drm/virtio/virtgpu_kms.c   | 2 +-
 drivers/i2c/busses/i2c-virtio.c| 2 +-
 drivers/iommu/virtio-iommu.c   | 2 +-
 drivers/net/caif/caif_virtio.c | 2 +-
 drivers/net/virtio_net.c   | 4 ++--
 drivers/net/wireless/mac80211_hwsim.c  | 2 +-
 drivers/nvdimm/virtio_pmem.c   | 2 +-
 drivers/rpmsg/virtio_rpmsg_bus.c   | 2 +-
 drivers/scsi/virtio_scsi.c | 2 +-
 drivers/virtio/virtio.c| 5 +
 drivers/virtio/virtio_balloon.c| 2 +-
 drivers/virtio/virtio_input.c  | 2 +-
 drivers/virtio/virtio_mem.c| 2 +-
 fs/fuse/virtio_fs.c| 4 ++--
 include/linux/virtio.h | 1 +
 net/9p/trans_virtio.c  | 2 +-
 net/vmw_vsock/virtio_transport.c   | 4 ++--
 sound/virtio/virtio_card.c | 4 ++--
 26 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
index c08066633023..22c4d87c9c15 100644
--- a/arch/um/drivers/virt-pci.c
+++ b/arch/um/drivers/virt-pci.c
@@ -616,7 +616,7 @@ static void um_pci_virtio_remove(struct virtio_device *vdev)
int i;
 
 /* Stop all virtqueues */
-vdev->config->reset(vdev);
+virtio_reset_device(vdev);
 vdev->config->del_vqs(vdev);
 
device_set_wakeup_enable(&vdev->dev, false);
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 303caf2d17d0..83d0af3fbf30 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -910,7 +910,7 @@ static void virtblk_remove(struct virtio_device *vdev)
mutex_lock(&vblk->vdev_mutex);
 
/* Stop all the virtqueues. */
-   vdev->config->reset(vdev);
+   virtio_reset_device(vdev);
 
/* Virtqueues are stopped, nothing can use vblk->vdev anymore. */
vblk->vdev = NULL;
@@ -929,7 +929,7 @@ static int virtblk_freeze(struct virtio_device *vdev)
struct virtio_blk *vblk = vdev->priv;
 
/* Ensure we don't receive any more interrupts */
-   vdev->config->reset(vdev);
+   virtio_reset_device(vdev);
 
/* Make sure no work handler is accessing the device. */
flush_work(&vblk->config_work);
diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 57908ce4fae8..24a9258962fa 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -364,7 +364,7 @@ static void virtbt_remove(struct virtio_device *vdev)
struct hci_dev *hdev = vbt->hdev;
 
hci_unregister_dev(hdev);
-   vdev->config->reset(vdev);
+   virtio_reset_device(vdev);
 
hci_free_dev(hdev);
vbt->hdev = NULL;
diff --git a/drivers/char/hw_random/virtio-rng.c 
b/drivers/char/hw_random/virtio-rng.c
index a90001e02bf7..95980489514b 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -134,7 +134,7 @@ static void remove_common(struct virtio_device *vdev)
vi->hwrng_removed = true;
vi->data_avail = 0;
complete(&vi->have_data);
-   vdev->config->reset(vdev);
+   virtio_reset_device(vdev);
vi->busy = false;
if (vi->hwrng_register_done)
hwrng_unregister(&vi->hwrng);
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7eaf303a7a86..08bbd693436f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1957,7 +1957,7 @@ static void virtcons_remove(struct virtio_device *vdev)
spin_unlock_irq(&pdrvdata_lock);
 
/* Disable interrupts for vqs */
-   vdev->config->reset(vdev);
+   virtio_reset_device(vdev);
/* Finish up work that's lined up */
if (use_multiport(portdev))
cancel_work_sync(&portdev->control_work);
@@ -2139,7 +2139,7 @@ static int virtcons_freeze(struct virtio_device *vdev)
 
portdev = vdev->priv;
 
-   vdev->config->reset(vdev);
+   virtio_reset_device(vdev);
 
if (use_multiport(portdev))
virtqueue_disable_cb(portdev->c_ivq);
diff --git a/drivers/crypto/virtio/virtio_crypto_core.c 
b/drivers/crypto/virtio/virtio_crypto_core.c
index e2375d992308..8e977b7627cb 100644
--- a/drivers/crypto/virti

Re: [PATCH RFC] virtio: wrap config->reset calls

2021-10-13 Thread Viresh Kumar

On 13-10-21, 06:55, Michael S. Tsirkin wrote:
> This will enable cleanups down the road.
> The idea is to disable cbs, then add "flush_queued_cbs" callback
> as a parameter, this way drivers can flush any work
> queued after callbacks have been disabled.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  drivers/gpio/gpio-virtio.c | 2 +-
>  drivers/i2c/busses/i2c-virtio.c| 2 +-

Reviewed-by: Viresh Kumar 

-- 
viresh

Re: [PATCH RFC] virtio: wrap config->reset calls

2021-10-13 Thread David Hildenbrand


On 13.10.21 12:55, Michael S. Tsirkin wrote:

This will enable cleanups down the road.
The idea is to disable cbs, then add "flush_queued_cbs" callback
as a parameter, this way drivers can flush any work
queued after callbacks have been disabled.

Signed-off-by: Michael S. Tsirkin 
---
  arch/um/drivers/virt-pci.c | 2 +-
  drivers/block/virtio_blk.c | 4 ++--
  drivers/bluetooth/virtio_bt.c  | 2 +-
  drivers/char/hw_random/virtio-rng.c| 2 +-
  drivers/char/virtio_console.c  | 4 ++--
  drivers/crypto/virtio/virtio_crypto_core.c | 8 
  drivers/firmware/arm_scmi/virtio.c | 2 +-
  drivers/gpio/gpio-virtio.c | 2 +-
  drivers/gpu/drm/virtio/virtgpu_kms.c   | 2 +-
  drivers/i2c/busses/i2c-virtio.c| 2 +-
  drivers/iommu/virtio-iommu.c   | 2 +-
  drivers/net/caif/caif_virtio.c | 2 +-
  drivers/net/virtio_net.c   | 4 ++--
  drivers/net/wireless/mac80211_hwsim.c  | 2 +-
  drivers/nvdimm/virtio_pmem.c   | 2 +-
  drivers/rpmsg/virtio_rpmsg_bus.c   | 2 +-
  drivers/scsi/virtio_scsi.c | 2 +-
  drivers/virtio/virtio.c| 5 +
  drivers/virtio/virtio_balloon.c| 2 +-
  drivers/virtio/virtio_input.c  | 2 +-
  drivers/virtio/virtio_mem.c| 2 +-
  fs/fuse/virtio_fs.c| 4 ++--
  include/linux/virtio.h | 1 +
  net/9p/trans_virtio.c  | 2 +-
  net/vmw_vsock/virtio_transport.c   | 4 ++--
  sound/virtio/virtio_card.c | 4 ++--
  26 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
index c08066633023..22c4d87c9c15 100644
--- a/arch/um/drivers/virt-pci.c
+++ b/arch/um/drivers/virt-pci.c
@@ -616,7 +616,7 @@ static void um_pci_virtio_remove(struct virtio_device *vdev)
int i;
  
  /* Stop all virtqueues */

-vdev->config->reset(vdev);
+virtio_reset_device(vdev);
  vdev->config->del_vqs(vdev);


Nit: virtio_device_reset()?

Because I see:

int virtio_device_freeze(struct virtio_device *dev);
int virtio_device_restore(struct virtio_device *dev);
void virtio_device_ready(struct virtio_device *dev)

But well, there is:
void virtio_break_device(struct virtio_device *dev);

--
Thanks,

David / dhildenb

[PATCH] drm/i915: Use dma_resv_iter for waiting in i915_gem_object_wait_reservation.

2021-10-13 Thread Maarten Lankhorst

No memory should be allocated when calling i915_gem_object_wait,
because it may be called to idle a BO when evicting memory.

Fix this by using dma_resv_iter helpers to call
i915_gem_object_wait_fence() on each fence, which cleans up the code a lot.
Also remove dma_resv_prune, it's questionably.

This will result in the following lockdep splat.

<4> [83.538517] ==
<4> [83.538520] WARNING: possible circular locking dependency detected
<4> [83.538522] 5.15.0-rc5-CI-Trybot_8062+ #1 Not tainted
<4> [83.538525] --
<4> [83.538527] gem_render_line/5242 is trying to acquire lock:
<4> [83.538530] 8275b1e0 (fs_reclaim){+.+.}-{0:0}, at: 
__kmalloc_track_caller+0x56/0x270
<4> [83.538538]
but task is already holding lock:
<4> [83.538540] 88813471d1e0 (&vm->mutex/1){+.+.}-{3:3}, at: 
i915_vma_pin_ww+0x1c7/0x970 [i915]
<4> [83.538638]
which lock already depends on the new lock.
<4> [83.538642]
the existing dependency chain (in reverse order) is:
<4> [83.538645]
-> #1 (&vm->mutex/1){+.+.}-{3:3}:
<4> [83.538649]lock_acquire+0xd3/0x310
<4> [83.538654]i915_gem_shrinker_taints_mutex+0x2d/0x50 [i915]
<4> [83.538730]i915_address_space_init+0xf5/0x1b0 [i915]
<4> [83.538794]ppgtt_init+0x55/0x70 [i915]
<4> [83.538856]gen8_ppgtt_create+0x44/0x5d0 [i915]
<4> [83.538912]i915_ppgtt_create+0x28/0xf0 [i915]
<4> [83.538971]intel_gt_init+0x130/0x3b0 [i915]
<4> [83.539029]i915_gem_init+0x14b/0x220 [i915]
<4> [83.539100]i915_driver_probe+0x97e/0xdd0 [i915]
<4> [83.539149]i915_pci_probe+0x43/0x1d0 [i915]
<4> [83.539197]pci_device_probe+0x9b/0x110
<4> [83.539201]really_probe+0x1b0/0x3b0
<4> [83.539205]__driver_probe_device+0xf6/0x170
<4> [83.539208]driver_probe_device+0x1a/0x90
<4> [83.539210]__driver_attach+0x93/0x160
<4> [83.539213]bus_for_each_dev+0x72/0xc0
<4> [83.539216]bus_add_driver+0x14b/0x1f0
<4> [83.539220]driver_register+0x66/0xb0
<4> [83.539222]hdmi_get_spk_alloc+0x1f/0x50 [snd_hda_codec_hdmi]
<4> [83.539227]do_one_initcall+0x53/0x2e0
<4> [83.539230]do_init_module+0x55/0x200
<4> [83.539234]load_module+0x2700/0x2980
<4> [83.539237]__do_sys_finit_module+0xaa/0x110
<4> [83.539241]do_syscall_64+0x37/0xb0
<4> [83.539244]entry_SYSCALL_64_after_hwframe+0x44/0xae
<4> [83.539247]
-> #0 (fs_reclaim){+.+.}-{0:0}:
<4> [83.539251]validate_chain+0xb37/0x1e70
<4> [83.539254]__lock_acquire+0x5a1/0xb70
<4> [83.539258]lock_acquire+0xd3/0x310
<4> [83.539260]fs_reclaim_acquire+0x9d/0xd0
<4> [83.539264]__kmalloc_track_caller+0x56/0x270
<4> [83.539267]krealloc+0x48/0xa0
<4> [83.539270]dma_resv_get_fences+0x1c3/0x280
<4> [83.539274]i915_gem_object_wait+0x1ff/0x410 [i915]
<4> [83.539342]i915_gem_evict_for_node+0x16b/0x440 [i915]
<4> [83.539412]i915_gem_gtt_reserve+0xff/0x130 [i915]
<4> [83.539482]i915_vma_pin_ww+0x765/0x970 [i915]
<4> [83.539556]eb_validate_vmas+0x6fe/0x8e0 [i915]
<4> [83.539626]i915_gem_do_execbuffer+0x9a6/0x20a0 [i915]
<4> [83.539693]i915_gem_execbuffer2_ioctl+0x11f/0x2c0 [i915]
<4> [83.539759]drm_ioctl_kernel+0xac/0x140
<4> [83.539763]drm_ioctl+0x201/0x3d0
<4> [83.539766]__x64_sys_ioctl+0x6a/0xa0
<4> [83.539769]do_syscall_64+0x37/0xb0
<4> [83.539772]entry_SYSCALL_64_after_hwframe+0x44/0xae
<4> [83.539775]
other info that might help us debug this:
<4> [83.539778]  Possible unsafe locking scenario:
<4> [83.539781]CPU0CPU1
<4> [83.539783]
<4> [83.539785]   lock(&vm->mutex/1);
<4> [83.539788]lock(fs_reclaim);
<4> [83.539791]lock(&vm->mutex/1);
<4> [83.539794]   lock(fs_reclaim);
<4> [83.539796]
 *** DEADLOCK ***
<4> [83.539799] 3 locks held by gem_render_line/5242:
<4> [83.539802]  #0: c9d4bbf0 
(reservation_ww_class_acquire){+.+.}-{0:0}, at: 
i915_gem_do_execbuffer+0x8e5/0x20a0 [i915]
<4> [83.539870]  #1: 88811e48bae8 (reservation_ww_class_mutex){+.+.}-{3:3}, 
at: eb_validate_vmas+0x81/0x8e0 [i915]
<4> [83.539936]  #2: 88813471d1e0 (&vm->mutex/1){+.+.}-{3:3}, at: 
i915_vma_pin_ww+0x1c7/0x970 [i915]
<4> [83.540011]
stack backtrace:
<4> [83.540014] CPU: 2 PID: 5242 Comm: gem_render_line Not tainted 
5.15.0-rc5-CI-Trybot_8062+ #1
<4> [83.540019] Hardware name: Intel(R) Client Systems NUC11TNHi3/NUC11TNBi3, 
BIOS TNTGL357.0038.2020.1124.1648 11/24/2020
<4> [83.540023] Call Trace:
<4> [83.540026]  dump_stack_lvl+0x56/0x7b
<4> [83.540030]  check_noncircular+0x12e/0x150
<4> [83.540034]  ? _raw_spin_unlock_irqrestore+0x50/0x60
<4> [83.540038]  validate_chain+0xb37/0x1e70
<4> [83.540042]  __lock_acquire+0x5a1/0xb70
<4> [83.540046]  lock_acquire+0

Re: [PATCH] drm/i915: Prefer struct_size over open coded arithmetic

2021-10-13 Thread Jani Nikula

On Mon, 11 Oct 2021, Len Baker  wrote:
> Hi,
>
> On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote:
>> As noted in the "Deprecated Interfaces, Language Features, Attributes,
>> and Conventions" documentation [1], size calculations (especially
>> multiplication) should not be performed in memory allocator (or similar)
>> function arguments due to the risk of them overflowing. This could lead
>> to values wrapping around and a smaller allocation being made than the
>> caller was expecting. Using those allocations could lead to linear
>> overflows of heap memory and other misbehaviors.
>>
>> In this case these are not actually dynamic sizes: all the operands
>> involved in the calculation are constant values. However it is better to
>> refactor them anyway, just to keep the open-coded math idiom out of
>> code.
>>
>> So, add at the end of the struct i915_syncmap a union with two flexible
>> array members (these arrays share the same memory layout). This is
>> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the
>> struct_size() helper to do the arithmetic instead of the argument
>> "size + count * size" in the kmalloc and kzalloc() functions.
>>
>> Also, take the opportunity to refactor the __sync_seqno and __sync_child
>> making them more readable.
>>
>> This code was detected with the help of Coccinelle and audited and fixed
>> manually.
>>
>> [1] 
>> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>>
>> Signed-off-by: Len Baker 
>> ---
>>  drivers/gpu/drm/i915/i915_syncmap.c | 12 
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> I received a mail telling that this patch doesn't build:
>
> == Series Details ==
>
> Series: drm/i915: Prefer struct_size over open coded arithmetic
> URL   : https://patchwork.freedesktop.org/series/95408/
> State : failure
>
> But it builds without error against linux-next (tag next-20211001). Against
> which tree and branch do I need to build?

drm-tip [1]. It's a sort of linux-next for graphics. I think there are
still some branches that don't feed to linux-next.

BR,
Jani.


[1] https://cgit.freedesktop.org/drm/drm-tip


>
> Regards,
> Len

-- 
Jani Nikula, Intel Open Source Graphics Center

1 2 3 >

1 - 100 of 217 matches

Mail list logo