RE: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks

2024-04-14 Thread Shankar, Uma 


> -Original Message-
> From: Luca Coelho 
> Sent: Friday, April 12, 2024 5:57 PM
> To: Shankar, Uma ; Coelho, Luciano
> ; intel-gfx@lists.freedesktop.org
> Cc: intel...@lists.freedesktop.org; ville.syrj...@linux.intel.com; Nikula, 
> Jani
> 
> Subject: Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
> 
> On Fri, 2024-04-12 at 10:30 +, Shankar, Uma wrote:
> >
> > > -Original Message-
> > > From: Coelho, Luciano 
> > > Sent: Friday, April 12, 2024 3:12 PM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: intel...@lists.freedesktop.org; Shankar, Uma
> > > ; ville.syrj...@linux.intel.com; Nikula, Jani
> > > 
> > > Subject: [PATCH v5 1/4] drm/i915/display: add support for DMC
> > > wakelocks
> > >
> > > In order to reduce the DC5->DC2 restore time, wakelocks have been
> > > introduced in DMC so the driver can tell it when registers and other
> > > memory areas are going to be accessed and keep their respective blocks
> awake.
> > >
> > > Implement this in the driver by adding the concept of DMC wakelocks.
> > > When the driver needs to access memory which lies inside pre-defined
> > > ranges, it will tell DMC to set the wakelock, access the memory,
> > > then wait for a while and clear the wakelock.
> > >
> > > The wakelock state is protected in the driver with spinlocks to
> > > prevent concurrency issues.
> >
> > Hi Luca,
> > Seems you missed to add the version history.
> 
> I've been sending the version history in the cover letter, because I don't 
> think it
> adds any information after it gets to the mainline kernel.  The history is 
> lost
> anyway, so the mailing list is a better place to store it (it's unique and 
> meaningful
> there).

Its matter of preference, but being part of the patch's commit message it stays 
with it
and can be checked with a git show. Cover letter details gets lost though as it 
doesn't
end up in the tree.

> Bur as I said to someone else before, I can add it to the commit message if 
> you
> think that it's needed.

Not needed Luca, it was a simple nitpick 😊. You can skip that.

> >
> > Anyways, changes look good to me.
> > Reviewed-by: Uma Shankar 
> 
> Thanks a lot!
> 
> Though you didn't review patch 3/4, the one about the module parameter.
> Was that intentional or did you just miss it?

I think I have reviewed and RB'ed it. The entire series is RB'ed now.

Regards,
Uma Shankar

> --
> Cheers,
> Luca.

Re: [PATCH] drm/mst: Fix NULL pointer dereference at drm_dp_add_payload_part2

2024-04-14 Thread Lin, Wayne 
[Public]

Ping for code review. Thanks!

Regards,
Wayne


From: Wayne Lin 
Sent: Thursday, March 7, 2024 14:29
To: dri-de...@lists.freedesktop.org; amd-...@lists.freedesktop.org; 
intel-gfx@lists.freedesktop.org
Cc: ly...@redhat.com; Wentland, Harry; imre.d...@intel.com; Lin, Wayne; Leon 
Weiß; sta...@vger.kernel.org; regressi...@lists.linux.dev
Subject: [PATCH] drm/mst: Fix NULL pointer dereference at 
drm_dp_add_payload_part2

[Why]
Commit:
- commit 5aa1dfcdf0a4 ("drm/mst: Refactor the flow for payload 
allocation/removement")
accidently overwrite the commit
- commit 54d217406afe ("drm: use mgr->dev in drm_dbg_kms in 
drm_dp_add_payload_part2")
which cause regression.

[How]
Recover the original NULL fix and remove the unnecessary input parameter 
'state' for
drm_dp_add_payload_part2().

Fixes: 5aa1dfcdf0a4 ("drm/mst: Refactor the flow for payload 
allocation/removement")
Reported-by: Leon Weiß 
Link: 
https://lore.kernel.org/r/38c253ea42072cc825dc969ac4e6b9b600371cc8.ca...@ruhr-uni-bochum.de/
Cc: ly...@redhat.com
Cc: imre.d...@intel.com
Cc: sta...@vger.kernel.org
Cc: regressi...@lists.linux.dev
Signed-off-by: Wayne Lin 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +---
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 2 +-
 drivers/gpu/drm/nouveau/dispnv50/disp.c   | 2 +-
 include/drm/display/drm_dp_mst_helper.h   | 1 -
 5 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index c27063305a13..2c36f3d00ca2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -363,7 +363,7 @@ void dm_helpers_dp_mst_send_payload_allocation(
mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
new_payload = drm_atomic_get_mst_payload_state(mst_state, 
aconnector->mst_output_port);

-   ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, 
new_payload);
+   ret = drm_dp_add_payload_part2(mst_mgr, new_payload);

if (ret) {
amdgpu_dm_set_mst_status(&aconnector->mst_status,
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c 
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 03d528209426..95fd18f24e94 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3421,7 +3421,6 @@ EXPORT_SYMBOL(drm_dp_remove_payload_part2);
 /**
  * drm_dp_add_payload_part2() - Execute payload update part 2
  * @mgr: Manager to use.
- * @state: The global atomic state
  * @payload: The payload to update
  *
  * If @payload was successfully assigned a starting time slot by 
drm_dp_add_payload_part1(), this
@@ -3430,14 +3429,13 @@ EXPORT_SYMBOL(drm_dp_remove_payload_part2);
  * Returns: 0 on success, negative error code on failure.
  */
 int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
-struct drm_atomic_state *state,
 struct drm_dp_mst_atomic_payload *payload)
 {
int ret = 0;

/* Skip failed payloads */
if (payload->payload_allocation_status != 
DRM_DP_MST_PAYLOAD_ALLOCATION_DFP) {
-   drm_dbg_kms(state->dev, "Part 1 of payload creation for %s 
failed, skipping part 2\n",
+   drm_dbg_kms(mgr->dev, "Part 1 of payload creation for %s 
failed, skipping part 2\n",
payload->port->connector->name);
return -EIO;
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c 
b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 53aec023ce92..2fba66aec038 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1160,7 +1160,7 @@ static void intel_mst_enable_dp(struct intel_atomic_state 
*state,
if (first_mst_stream)
intel_ddi_wait_for_fec_status(encoder, pipe_config, true);

-   drm_dp_add_payload_part2(&intel_dp->mst_mgr, &state->base,
+   drm_dp_add_payload_part2(&intel_dp->mst_mgr,
 drm_atomic_get_mst_payload_state(mst_state, 
connector->port));

if (DISPLAY_VER(dev_priv) >= 12)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c 
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 0c3d88ad0b0e..88728a0b2c25 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -915,7 +915,7 @@ nv50_msto_cleanup(struct drm_atomic_state *state,
msto->disabled = false;
drm_dp_remove_payload_part2(mgr, new_mst_state, old_payload, 
new_payload);
} else if (msto->enabled) {
-   drm_dp_add_payload_part2(mgr, state, new_payload);
+   drm_dp_add_payload_part2(mgr, new_payloa

Re: [PATCH] drm/i915/guc: Remove usage of the deprecated ida_simple_xx() API

2024-04-14 Thread Matthew Brost 
On Sun, Apr 14, 2024 at 10:26:35AM +0200, Christophe JAILLET wrote:
> Le 25/01/2024 à 01:04, Matthew Brost a écrit :
> > On Sun, Jan 14, 2024 at 04:15:34PM +0100, Christophe JAILLET wrote:
> > > ida_alloc() and ida_free() should be preferred to the deprecated
> > > ida_simple_get() and ida_simple_remove().
> > > 
> > > Note that the upper limit of ida_simple_get() is exclusive, but the one of
> > > ida_alloc_range() is inclusive. So a -1 has been added when needed.
> > > 
> > > Signed-off-by: Christophe JAILLET 
> > 
> > Reviewed-by: Matthew Brost 
> 
> Hi,
> 
> polite reminder ;-)
> 

Merged. Thanks for the patch.

Matt

> CJ
> 
> > 
> > > ---
> > >   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 13 ++---
> > >   1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
> > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > index a259f1118c5a..73ce21ddf682 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -2156,11 +2156,10 @@ static int new_guc_id(struct intel_guc *guc, 
> > > struct intel_context *ce)
> > > 
> > > order_base_2(ce->parallel.number_children
> > >  + 1));
> > >   else
> > > - ret = ida_simple_get(&guc->submission_state.guc_ids,
> > > -  NUMBER_MULTI_LRC_GUC_ID(guc),
> > > -  guc->submission_state.num_guc_ids,
> > > -  GFP_KERNEL | __GFP_RETRY_MAYFAIL |
> > > -  __GFP_NOWARN);
> > > + ret = ida_alloc_range(&guc->submission_state.guc_ids,
> > > +   NUMBER_MULTI_LRC_GUC_ID(guc),
> > > +   guc->submission_state.num_guc_ids - 1,
> > > +   GFP_KERNEL | __GFP_RETRY_MAYFAIL | 
> > > __GFP_NOWARN);
> > >   if (unlikely(ret < 0))
> > >   return ret;
> > > @@ -2183,8 +2182,8 @@ static void __release_guc_id(struct intel_guc *guc, 
> > > struct intel_context *ce)
> > >  + 1));
> > >   } else {
> > >   --guc->submission_state.guc_ids_in_use;
> > > - ida_simple_remove(&guc->submission_state.guc_ids,
> > > -   ce->guc_id.id);
> > > + ida_free(&guc->submission_state.guc_ids,
> > > +  ce->guc_id.id);
> > >   }
> > >   clr_ctx_id_mapping(guc, ce->guc_id.id);
> > >   set_context_guc_id_invalid(ce);
> > > -- 
> > > 2.43.0
> > > 
> > 
> > 
>

Re: [PATCH] drm/i915/hwmon: Get rid of devm

2024-04-14 Thread Dixit, Ashutosh 
On Fri, 12 Apr 2024 17:10:31 -0700, Ashutosh Dixit wrote:
>
> When both hwmon and hwmon drvdata (on which hwmon depends) are device
> managed resources, the expectation, on device unbind, is that hwmon will be
> released before the drvdata. However, it appears devres does not do this
> consistently, so that we occasionally see drvdata being released before
> hwmon itself. This results in a uaf if hwmon sysfs is accessed during
> device unbind.
>
> The only way out of this seems to be do get rid of devm_ and release/free
> everything explicitly during device unbind.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366
> Signed-off-by: Ashutosh Dixit 

Please don't review this patch yet, I will send a v2 tomorrow. Please
review the v2. Thanks.

Re: [PATCH] drm/i915/guc: Remove usage of the deprecated ida_simple_xx() API

2024-04-14 Thread Christophe JAILLET 

Le 25/01/2024 à 01:04, Matthew Brost a écrit :

On Sun, Jan 14, 2024 at 04:15:34PM +0100, Christophe JAILLET wrote:

ida_alloc() and ida_free() should be preferred to the deprecated
ida_simple_get() and ida_simple_remove().

Note that the upper limit of ida_simple_get() is exclusive, but the one of
ida_alloc_range() is inclusive. So a -1 has been added when needed.

Signed-off-by: Christophe JAILLET 


Reviewed-by: Matthew Brost 


Hi,

polite reminder ;-)

CJ




---
  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index a259f1118c5a..73ce21ddf682 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -2156,11 +2156,10 @@ static int new_guc_id(struct intel_guc *guc, struct 
intel_context *ce)
  
order_base_2(ce->parallel.number_children
   + 1));
else
-   ret = ida_simple_get(&guc->submission_state.guc_ids,
-NUMBER_MULTI_LRC_GUC_ID(guc),
-guc->submission_state.num_guc_ids,
-GFP_KERNEL | __GFP_RETRY_MAYFAIL |
-__GFP_NOWARN);
+   ret = ida_alloc_range(&guc->submission_state.guc_ids,
+ NUMBER_MULTI_LRC_GUC_ID(guc),
+ guc->submission_state.num_guc_ids - 1,
+ GFP_KERNEL | __GFP_RETRY_MAYFAIL | 
__GFP_NOWARN);
if (unlikely(ret < 0))
return ret;
  
@@ -2183,8 +2182,8 @@ static void __release_guc_id(struct intel_guc *guc, struct intel_context *ce)

   + 1));
} else {
--guc->submission_state.guc_ids_in_use;
-   ida_simple_remove(&guc->submission_state.guc_ids,
- ce->guc_id.id);
+   ida_free(&guc->submission_state.guc_ids,
+ce->guc_id.id);
}
clr_ctx_id_mapping(guc, ce->guc_id.id);
set_context_guc_id_invalid(ce);
--
2.43.0