diff mbox series

[06/11] accel/ivpu: Change test_mode module param to bitmask

Message ID 20231025094323.989987-7-stanislaw.gruszka@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series accel/ivpu: Update to -next 2023-10-25 | expand

Commit Message

Stanislaw Gruszka Oct. 25, 2023, 9:43 a.m. UTC 
From: Karol Wachowski <karol.wachowski@linux.intel.com>

Change meaning of test_mode module parameter from integer value
to bitmask allowing setting different test features with corresponding
bits.

Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_drv.c | 4 ++--
 drivers/accel/ivpu/ivpu_drv.h | 7 +++----
 drivers/accel/ivpu/ivpu_job.c | 4 ++--
 3 files changed, 7 insertions(+), 8 deletions(-)

Comments

Stanislaw Gruszka Oct. 28, 2023, 8:18 a.m. UTC | #1 
On Fri, Oct 27, 2023 at 08:47:11AM -0600, Jeffrey Hugo wrote:
> On 10/25/2023 3:43 AM, Stanislaw Gruszka wrote:
> > From: Karol Wachowski <karol.wachowski@linux.intel.com>
> > 
> > Change meaning of test_mode module parameter from integer value
> > to bitmask allowing setting different test features with corresponding
> > bits.
> > 
> > Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> > Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> 
> Seems like this changes the uAPI.  You still haven't made a release of the
> userspace, correct? 

Yes the user space is not yet released. However I think module parameter
is not considered part of the linux kernel uAPI and there are no guaranties
regarding not changing or removing or change the semantics.

Obviously we don't want to brake anyone config file or script, but that's more
like courtesy than hardcoded rule. Currently nobody except Intel and it's
partners are using intel_vpu and all user should be aware of the change.

Regards
Stanislaw
Jeffrey Hugo Oct. 30, 2023, 2:05 p.m. UTC | #2 
On 10/28/2023 2:18 AM, Stanislaw Gruszka wrote:
> On Fri, Oct 27, 2023 at 08:47:11AM -0600, Jeffrey Hugo wrote:
>> On 10/25/2023 3:43 AM, Stanislaw Gruszka wrote:
>>> From: Karol Wachowski <karol.wachowski@linux.intel.com>
>>>
>>> Change meaning of test_mode module parameter from integer value
>>> to bitmask allowing setting different test features with corresponding
>>> bits.
>>>
>>> Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
>>> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>>> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>>
>> Seems like this changes the uAPI.  You still haven't made a release of the
>> userspace, correct?
> 
> Yes the user space is not yet released. However I think module parameter
> is not considered part of the linux kernel uAPI and there are no guaranties
> regarding not changing or removing or change the semantics.

Patch 3 of [1] seems to suggest otherwise (module parameters are part of 
the uAPI)

[1]: 
https://lore.kernel.org/all/20231027193016.27516-1-quic_johmoo@quicinc.com/
Stanislaw Gruszka Oct. 30, 2023, 3:07 p.m. UTC | #3 
On Mon, Oct 30, 2023 at 08:05:28AM -0600, Jeffrey Hugo wrote:
> On 10/28/2023 2:18 AM, Stanislaw Gruszka wrote:
> > On Fri, Oct 27, 2023 at 08:47:11AM -0600, Jeffrey Hugo wrote:
> > > On 10/25/2023 3:43 AM, Stanislaw Gruszka wrote:
> > > > From: Karol Wachowski <karol.wachowski@linux.intel.com>
> > > > 
> > > > Change meaning of test_mode module parameter from integer value
> > > > to bitmask allowing setting different test features with corresponding
> > > > bits.
> > > > 
> > > > Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> > > > Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > 
> > > Seems like this changes the uAPI.  You still haven't made a release of the
> > > userspace, correct?
> > 
> > Yes the user space is not yet released. However I think module parameter
> > is not considered part of the linux kernel uAPI and there are no guaranties
> > regarding not changing or removing or change the semantics.
> 
> Patch 3 of [1] seems to suggest otherwise (module parameters are part of the
> uAPI)

I hope it will not be applied :-) Will be quite burden to maintain module
parameters compatibility.

Regards
Stanislaw
diff mbox series

Patch

diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index 2a58fb1e2fcc..4dbfd05680ce 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -39,7 +39,7 @@  MODULE_PARM_DESC(dbg_mask, "Driver debug mask. See IVPU_DBG_* macros.");
 
 int ivpu_test_mode;
 module_param_named_unsafe(test_mode, ivpu_test_mode, int, 0644);
-MODULE_PARM_DESC(test_mode, "Test mode: 0 - disabled , 1 - fw unit test, 2 - null hw, 3 - null submission");
+MODULE_PARM_DESC(test_mode, "Test mode mask. See IVPU_TEST_MODE_* macros.");
 
 u8 ivpu_pll_min_ratio;
 module_param_named(pll_min_ratio, ivpu_pll_min_ratio, byte, 0644);
@@ -315,7 +315,7 @@  static int ivpu_wait_for_ready(struct ivpu_device *vdev)
 	unsigned long timeout;
 	int ret;
 
-	if (ivpu_test_mode == IVPU_TEST_MODE_FW_TEST)
+	if (ivpu_test_mode & IVPU_TEST_MODE_FW_TEST)
 		return 0;
 
 	ivpu_ipc_consumer_add(vdev, &cons, IVPU_IPC_CHAN_BOOT_MSG);
diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
index 267e03a5edf4..5432b5ee90df 100644
--- a/drivers/accel/ivpu/ivpu_drv.h
+++ b/drivers/accel/ivpu/ivpu_drv.h
@@ -148,10 +148,9 @@  extern u8 ivpu_pll_min_ratio;
 extern u8 ivpu_pll_max_ratio;
 extern bool ivpu_disable_mmu_cont_pages;
 
-#define IVPU_TEST_MODE_DISABLED        0
-#define IVPU_TEST_MODE_FW_TEST         1
-#define IVPU_TEST_MODE_NULL_HW         2
-#define IVPU_TEST_MODE_NULL_SUBMISSION 3
+#define IVPU_TEST_MODE_FW_TEST         BIT(0)
+#define IVPU_TEST_MODE_NULL_HW         BIT(1)
+#define IVPU_TEST_MODE_NULL_SUBMISSION BIT(2)
 extern int ivpu_test_mode;
 
 struct ivpu_file_priv *ivpu_file_priv_get(struct ivpu_file_priv *file_priv);
diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
index 646b8f812901..6e96c921547d 100644
--- a/drivers/accel/ivpu/ivpu_job.c
+++ b/drivers/accel/ivpu/ivpu_job.c
@@ -196,7 +196,7 @@  static int ivpu_cmdq_push_job(struct ivpu_cmdq *cmdq, struct ivpu_job *job)
 	entry->batch_buf_addr = job->cmd_buf_vpu_addr;
 	entry->job_id = job->job_id;
 	entry->flags = 0;
-	if (unlikely(ivpu_test_mode == IVPU_TEST_MODE_NULL_SUBMISSION))
+	if (unlikely(ivpu_test_mode & IVPU_TEST_MODE_NULL_SUBMISSION))
 		entry->flags = VPU_JOB_FLAGS_NULL_SUBMISSION_MASK;
 	wmb(); /* Ensure that tail is updated after filling entry */
 	header->tail = next_entry;
@@ -404,7 +404,7 @@  static int ivpu_direct_job_submission(struct ivpu_job *job)
 		 job->job_id, job->cmd_buf_vpu_addr, file_priv->ctx.id,
 		 job->engine_idx, cmdq->jobq->header.tail);
 
-	if (ivpu_test_mode == IVPU_TEST_MODE_NULL_HW) {
+	if (ivpu_test_mode & IVPU_TEST_MODE_NULL_HW) {
 		ivpu_job_done(vdev, job->job_id, VPU_JSM_STATUS_SUCCESS);
 		cmdq->jobq->header.head = cmdq->jobq->header.tail;
 		wmb(); /* Flush WC buffer for jobq header */