* [PATCH v2] media: intel/ipu6: remove buttress ish structure
@ 2024-10-31 13:06 Stanislaw Gruszka
2024-11-01 4:22 ` Bingbu Cao
0 siblings, 1 reply; 9+ messages in thread
From: Stanislaw Gruszka @ 2024-10-31 13:06 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Bingbu Cao
The buttress ipc ish structure is not effectively used on IPU6 - data
is nullified on init. Remove it to cleanup the code a bit.
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
v2: fix formatting: use media: prefix in topic and white space alignment
to match open parenthesis
drivers/media/pci/intel/ipu6/ipu6-buttress.c | 27 ++++++--------------
drivers/media/pci/intel/ipu6/ipu6-buttress.h | 6 -----
2 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
index edaa285283a1..6644fd4c3d91 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
@@ -214,20 +214,17 @@ static void ipu6_buttress_ipc_recv(struct ipu6_device *isp,
}
static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
- enum ipu6_buttress_ipc_domain ipc_domain,
struct ipu6_ipc_buttress_bulk_msg *msgs,
u32 size)
{
unsigned long tx_timeout_jiffies, rx_timeout_jiffies;
unsigned int i, retry = BUTTRESS_IPC_CMD_SEND_RETRY;
struct ipu6_buttress *b = &isp->buttress;
- struct ipu6_buttress_ipc *ipc;
+ struct ipu6_buttress_ipc *ipc = &b->cse;
u32 val;
int ret;
int tout;
- ipc = ipc_domain == IPU6_BUTTRESS_IPC_CSE ? &b->cse : &b->ish;
-
mutex_lock(&b->ipc_mutex);
ret = ipu6_buttress_ipc_validity_open(isp, ipc);
@@ -305,7 +302,6 @@ static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
static int
ipu6_buttress_ipc_send(struct ipu6_device *isp,
- enum ipu6_buttress_ipc_domain ipc_domain,
u32 ipc_msg, u32 size, bool require_resp,
u32 expected_resp)
{
@@ -316,7 +312,7 @@ ipu6_buttress_ipc_send(struct ipu6_device *isp,
.expected_resp = expected_resp,
};
- return ipu6_buttress_ipc_send_bulk(isp, ipc_domain, &msg, 1);
+ return ipu6_buttress_ipc_send_bulk(isp, &msg, 1);
}
static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
@@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
}
if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
- dev_dbg(&isp->pdev->dev,
- "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
- ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
- complete(&b->ish.recv_complete);
+ dev_warn(&isp->pdev->dev,
+ "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
}
if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE) {
@@ -399,9 +393,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
}
if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH) {
- dev_dbg(&isp->pdev->dev,
- "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
- complete(&b->ish.send_complete);
+ dev_warn(&isp->pdev->dev,
+ "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
}
if (irq_status & BUTTRESS_ISR_SAI_VIOLATION &&
@@ -655,7 +648,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
*/
dev_info(&isp->pdev->dev, "Sending BOOT_LOAD to CSE\n");
- ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
+ ret = ipu6_buttress_ipc_send(isp,
BUTTRESS_IU2CSEDATA0_IPC_BOOT_LOAD,
1, true,
BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
@@ -697,7 +690,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
* IU2CSEDB.IU2CSECMD and set IU2CSEDB.IU2CSEBUSY as
*/
dev_info(&isp->pdev->dev, "Sending AUTHENTICATE_RUN to CSE\n");
- ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
+ ret = ipu6_buttress_ipc_send(isp,
BUTTRESS_IU2CSEDATA0_IPC_AUTH_RUN,
1, true,
BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
@@ -838,9 +831,7 @@ int ipu6_buttress_init(struct ipu6_device *isp)
mutex_init(&b->auth_mutex);
mutex_init(&b->cons_mutex);
mutex_init(&b->ipc_mutex);
- init_completion(&b->ish.send_complete);
init_completion(&b->cse.send_complete);
- init_completion(&b->ish.recv_complete);
init_completion(&b->cse.recv_complete);
b->cse.nack = BUTTRESS_CSE2IUDATA0_IPC_NACK;
@@ -852,8 +843,6 @@ int ipu6_buttress_init(struct ipu6_device *isp)
b->cse.data0_in = BUTTRESS_REG_CSE2IUDATA0;
b->cse.data0_out = BUTTRESS_REG_IU2CSEDATA0;
- /* no ISH on IPU6 */
- memset(&b->ish, 0, sizeof(b->ish));
INIT_LIST_HEAD(&b->constraints);
isp->secure_mode = ipu6_buttress_get_secure_mode(isp);
diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
index 9b6f56958be7..482978c2a09d 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h
+++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
@@ -46,18 +46,12 @@ struct ipu6_buttress_ipc {
struct ipu6_buttress {
struct mutex power_mutex, auth_mutex, cons_mutex, ipc_mutex;
struct ipu6_buttress_ipc cse;
- struct ipu6_buttress_ipc ish;
struct list_head constraints;
u32 wdt_cached_value;
bool force_suspend;
u32 ref_clk;
};
-enum ipu6_buttress_ipc_domain {
- IPU6_BUTTRESS_IPC_CSE,
- IPU6_BUTTRESS_IPC_ISH,
-};
-
struct ipu6_ipc_buttress_bulk_msg {
u32 cmd;
u32 expected_resp;
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-10-31 13:06 [PATCH v2] media: intel/ipu6: remove buttress ish structure Stanislaw Gruszka
@ 2024-11-01 4:22 ` Bingbu Cao
2024-11-01 4:26 ` Bingbu Cao
0 siblings, 1 reply; 9+ messages in thread
From: Bingbu Cao @ 2024-11-01 4:22 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-media; +Cc: Sakari Ailus, Bingbu Cao
Stanislaw,
Thanks for the patch, this is what I intended to do.
On 10/31/24 9:06 PM, Stanislaw Gruszka wrote:
> The buttress ipc ish structure is not effectively used on IPU6 - data
> is nullified on init. Remove it to cleanup the code a bit.
>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
> v2: fix formatting: use media: prefix in topic and white space alignment
> to match open parenthesis
>
> drivers/media/pci/intel/ipu6/ipu6-buttress.c | 27 ++++++--------------
> drivers/media/pci/intel/ipu6/ipu6-buttress.h | 6 -----
> 2 files changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> index edaa285283a1..6644fd4c3d91 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> @@ -214,20 +214,17 @@ static void ipu6_buttress_ipc_recv(struct ipu6_device *isp,
> }
>
> static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
> - enum ipu6_buttress_ipc_domain ipc_domain,
> struct ipu6_ipc_buttress_bulk_msg *msgs,
> u32 size)
> {
> unsigned long tx_timeout_jiffies, rx_timeout_jiffies;
> unsigned int i, retry = BUTTRESS_IPC_CMD_SEND_RETRY;
> struct ipu6_buttress *b = &isp->buttress;
> - struct ipu6_buttress_ipc *ipc;
> + struct ipu6_buttress_ipc *ipc = &b->cse;
> u32 val;
> int ret;
> int tout;
>
> - ipc = ipc_domain == IPU6_BUTTRESS_IPC_CSE ? &b->cse : &b->ish;
> -
> mutex_lock(&b->ipc_mutex);
>
> ret = ipu6_buttress_ipc_validity_open(isp, ipc);
> @@ -305,7 +302,6 @@ static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
>
> static int
> ipu6_buttress_ipc_send(struct ipu6_device *isp,
> - enum ipu6_buttress_ipc_domain ipc_domain,
> u32 ipc_msg, u32 size, bool require_resp,
> u32 expected_resp)
> {
> @@ -316,7 +312,7 @@ ipu6_buttress_ipc_send(struct ipu6_device *isp,
> .expected_resp = expected_resp,
> };
>
> - return ipu6_buttress_ipc_send_bulk(isp, ipc_domain, &msg, 1);
> + return ipu6_buttress_ipc_send_bulk(isp, &msg, 1);
> }
>
> static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
> }
>
> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
> - dev_dbg(&isp->pdev->dev,
> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
> - complete(&b->ish.recv_complete);
> + dev_warn(&isp->pdev->dev,
> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
I think this is a unrelated change, right?
> }
>
> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE) {
> @@ -399,9 +393,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
> }
>
> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH) {
> - dev_dbg(&isp->pdev->dev,
> - "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
> - complete(&b->ish.send_complete);
> + dev_warn(&isp->pdev->dev,
> + "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
Ditto.
> }
>
> if (irq_status & BUTTRESS_ISR_SAI_VIOLATION &&
> @@ -655,7 +648,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
> */
> dev_info(&isp->pdev->dev, "Sending BOOT_LOAD to CSE\n");
>
> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
> + ret = ipu6_buttress_ipc_send(isp,
> BUTTRESS_IU2CSEDATA0_IPC_BOOT_LOAD,
> 1, true,
> BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
> @@ -697,7 +690,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
> * IU2CSEDB.IU2CSECMD and set IU2CSEDB.IU2CSEBUSY as
> */
> dev_info(&isp->pdev->dev, "Sending AUTHENTICATE_RUN to CSE\n");
> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
> + ret = ipu6_buttress_ipc_send(isp,
> BUTTRESS_IU2CSEDATA0_IPC_AUTH_RUN,
> 1, true,
> BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
> @@ -838,9 +831,7 @@ int ipu6_buttress_init(struct ipu6_device *isp)
> mutex_init(&b->auth_mutex);
> mutex_init(&b->cons_mutex);
> mutex_init(&b->ipc_mutex);
> - init_completion(&b->ish.send_complete);
> init_completion(&b->cse.send_complete);
> - init_completion(&b->ish.recv_complete);
> init_completion(&b->cse.recv_complete);
>
> b->cse.nack = BUTTRESS_CSE2IUDATA0_IPC_NACK;
> @@ -852,8 +843,6 @@ int ipu6_buttress_init(struct ipu6_device *isp)
> b->cse.data0_in = BUTTRESS_REG_CSE2IUDATA0;
> b->cse.data0_out = BUTTRESS_REG_IU2CSEDATA0;
>
> - /* no ISH on IPU6 */
> - memset(&b->ish, 0, sizeof(b->ish));
> INIT_LIST_HEAD(&b->constraints);
>
> isp->secure_mode = ipu6_buttress_get_secure_mode(isp);
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
> index 9b6f56958be7..482978c2a09d 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h
> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
> @@ -46,18 +46,12 @@ struct ipu6_buttress_ipc {
> struct ipu6_buttress {
> struct mutex power_mutex, auth_mutex, cons_mutex, ipc_mutex;
> struct ipu6_buttress_ipc cse;
> - struct ipu6_buttress_ipc ish;
> struct list_head constraints;
> u32 wdt_cached_value;
> bool force_suspend;
> u32 ref_clk;
> };
>
> -enum ipu6_buttress_ipc_domain {
> - IPU6_BUTTRESS_IPC_CSE,
> - IPU6_BUTTRESS_IPC_ISH,
> -};
> -
> struct ipu6_ipc_buttress_bulk_msg {
> u32 cmd;
> u32 expected_resp;
>
Besides minor comment:
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-11-01 4:22 ` Bingbu Cao
@ 2024-11-01 4:26 ` Bingbu Cao
2024-11-01 7:46 ` Sakari Ailus
0 siblings, 1 reply; 9+ messages in thread
From: Bingbu Cao @ 2024-11-01 4:26 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-media; +Cc: Sakari Ailus, Bingbu Cao
On 11/1/24 12:22 PM, Bingbu Cao wrote:
> Stanislaw,
>
> Thanks for the patch, this is what I intended to do.
>
> On 10/31/24 9:06 PM, Stanislaw Gruszka wrote:
>> The buttress ipc ish structure is not effectively used on IPU6 - data
>> is nullified on init. Remove it to cleanup the code a bit.
>>
>> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>> ---
>> v2: fix formatting: use media: prefix in topic and white space alignment
>> to match open parenthesis
>>
>> drivers/media/pci/intel/ipu6/ipu6-buttress.c | 27 ++++++--------------
>> drivers/media/pci/intel/ipu6/ipu6-buttress.h | 6 -----
>> 2 files changed, 8 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>> index edaa285283a1..6644fd4c3d91 100644
>> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>> @@ -214,20 +214,17 @@ static void ipu6_buttress_ipc_recv(struct ipu6_device *isp,
>> }
>>
>> static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
>> - enum ipu6_buttress_ipc_domain ipc_domain,
>> struct ipu6_ipc_buttress_bulk_msg *msgs,
>> u32 size)
>> {
>> unsigned long tx_timeout_jiffies, rx_timeout_jiffies;
>> unsigned int i, retry = BUTTRESS_IPC_CMD_SEND_RETRY;
>> struct ipu6_buttress *b = &isp->buttress;
>> - struct ipu6_buttress_ipc *ipc;
>> + struct ipu6_buttress_ipc *ipc = &b->cse;
>> u32 val;
>> int ret;
>> int tout;
>>
>> - ipc = ipc_domain == IPU6_BUTTRESS_IPC_CSE ? &b->cse : &b->ish;
>> -
>> mutex_lock(&b->ipc_mutex);
>>
>> ret = ipu6_buttress_ipc_validity_open(isp, ipc);
>> @@ -305,7 +302,6 @@ static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
>>
>> static int
>> ipu6_buttress_ipc_send(struct ipu6_device *isp,
>> - enum ipu6_buttress_ipc_domain ipc_domain,
>> u32 ipc_msg, u32 size, bool require_resp,
>> u32 expected_resp)
>> {
>> @@ -316,7 +312,7 @@ ipu6_buttress_ipc_send(struct ipu6_device *isp,
>> .expected_resp = expected_resp,
>> };
>>
>> - return ipu6_buttress_ipc_send_bulk(isp, ipc_domain, &msg, 1);
>> + return ipu6_buttress_ipc_send_bulk(isp, &msg, 1);
>> }
>>
>> static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
>> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>> }
>>
>> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
>> - dev_dbg(&isp->pdev->dev,
>> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
>> - complete(&b->ish.recv_complete);
>> + dev_warn(&isp->pdev->dev,
>> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>
> I think this is a unrelated change, right?
I mean the change from dev_dbg() to dev_warn().
>
>> }
>>
>> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE) {
>> @@ -399,9 +393,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>> }
>>
>> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH) {
>> - dev_dbg(&isp->pdev->dev,
>> - "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
>> - complete(&b->ish.send_complete);
>> + dev_warn(&isp->pdev->dev,
>> + "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
>
> Ditto.
>
>> }
>>
>> if (irq_status & BUTTRESS_ISR_SAI_VIOLATION &&
>> @@ -655,7 +648,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
>> */
>> dev_info(&isp->pdev->dev, "Sending BOOT_LOAD to CSE\n");
>>
>> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
>> + ret = ipu6_buttress_ipc_send(isp,
>> BUTTRESS_IU2CSEDATA0_IPC_BOOT_LOAD,
>> 1, true,
>> BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
>> @@ -697,7 +690,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
>> * IU2CSEDB.IU2CSECMD and set IU2CSEDB.IU2CSEBUSY as
>> */
>> dev_info(&isp->pdev->dev, "Sending AUTHENTICATE_RUN to CSE\n");
>> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
>> + ret = ipu6_buttress_ipc_send(isp,
>> BUTTRESS_IU2CSEDATA0_IPC_AUTH_RUN,
>> 1, true,
>> BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
>> @@ -838,9 +831,7 @@ int ipu6_buttress_init(struct ipu6_device *isp)
>> mutex_init(&b->auth_mutex);
>> mutex_init(&b->cons_mutex);
>> mutex_init(&b->ipc_mutex);
>> - init_completion(&b->ish.send_complete);
>> init_completion(&b->cse.send_complete);
>> - init_completion(&b->ish.recv_complete);
>> init_completion(&b->cse.recv_complete);
>>
>> b->cse.nack = BUTTRESS_CSE2IUDATA0_IPC_NACK;
>> @@ -852,8 +843,6 @@ int ipu6_buttress_init(struct ipu6_device *isp)
>> b->cse.data0_in = BUTTRESS_REG_CSE2IUDATA0;
>> b->cse.data0_out = BUTTRESS_REG_IU2CSEDATA0;
>>
>> - /* no ISH on IPU6 */
>> - memset(&b->ish, 0, sizeof(b->ish));
>> INIT_LIST_HEAD(&b->constraints);
>>
>> isp->secure_mode = ipu6_buttress_get_secure_mode(isp);
>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>> index 9b6f56958be7..482978c2a09d 100644
>> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>> @@ -46,18 +46,12 @@ struct ipu6_buttress_ipc {
>> struct ipu6_buttress {
>> struct mutex power_mutex, auth_mutex, cons_mutex, ipc_mutex;
>> struct ipu6_buttress_ipc cse;
>> - struct ipu6_buttress_ipc ish;
>> struct list_head constraints;
>> u32 wdt_cached_value;
>> bool force_suspend;
>> u32 ref_clk;
>> };
>>
>> -enum ipu6_buttress_ipc_domain {
>> - IPU6_BUTTRESS_IPC_CSE,
>> - IPU6_BUTTRESS_IPC_ISH,
>> -};
>> -
>> struct ipu6_ipc_buttress_bulk_msg {
>> u32 cmd;
>> u32 expected_resp;
>>
>
> Besides minor comment:
>
> Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
>
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-11-01 4:26 ` Bingbu Cao
@ 2024-11-01 7:46 ` Sakari Ailus
2024-11-01 7:47 ` Bingbu Cao
0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2024-11-01 7:46 UTC (permalink / raw)
To: Bingbu Cao; +Cc: Stanislaw Gruszka, linux-media, Bingbu Cao
Hi Bingbu, Stanislaw,
On Fri, Nov 01, 2024 at 12:26:52PM +0800, Bingbu Cao wrote:
>
> On 11/1/24 12:22 PM, Bingbu Cao wrote:
> > Stanislaw,
> >
> > Thanks for the patch, this is what I intended to do.
> >
> > On 10/31/24 9:06 PM, Stanislaw Gruszka wrote:
> >> The buttress ipc ish structure is not effectively used on IPU6 - data
> >> is nullified on init. Remove it to cleanup the code a bit.
> >>
> >> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> >> ---
> >> v2: fix formatting: use media: prefix in topic and white space alignment
> >> to match open parenthesis
> >>
> >> drivers/media/pci/intel/ipu6/ipu6-buttress.c | 27 ++++++--------------
> >> drivers/media/pci/intel/ipu6/ipu6-buttress.h | 6 -----
> >> 2 files changed, 8 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> >> index edaa285283a1..6644fd4c3d91 100644
> >> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> >> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> >> @@ -214,20 +214,17 @@ static void ipu6_buttress_ipc_recv(struct ipu6_device *isp,
> >> }
> >>
> >> static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
> >> - enum ipu6_buttress_ipc_domain ipc_domain,
> >> struct ipu6_ipc_buttress_bulk_msg *msgs,
> >> u32 size)
> >> {
> >> unsigned long tx_timeout_jiffies, rx_timeout_jiffies;
> >> unsigned int i, retry = BUTTRESS_IPC_CMD_SEND_RETRY;
> >> struct ipu6_buttress *b = &isp->buttress;
> >> - struct ipu6_buttress_ipc *ipc;
> >> + struct ipu6_buttress_ipc *ipc = &b->cse;
> >> u32 val;
> >> int ret;
> >> int tout;
> >>
> >> - ipc = ipc_domain == IPU6_BUTTRESS_IPC_CSE ? &b->cse : &b->ish;
> >> -
> >> mutex_lock(&b->ipc_mutex);
> >>
> >> ret = ipu6_buttress_ipc_validity_open(isp, ipc);
> >> @@ -305,7 +302,6 @@ static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
> >>
> >> static int
> >> ipu6_buttress_ipc_send(struct ipu6_device *isp,
> >> - enum ipu6_buttress_ipc_domain ipc_domain,
> >> u32 ipc_msg, u32 size, bool require_resp,
> >> u32 expected_resp)
> >> {
> >> @@ -316,7 +312,7 @@ ipu6_buttress_ipc_send(struct ipu6_device *isp,
> >> .expected_resp = expected_resp,
> >> };
> >>
> >> - return ipu6_buttress_ipc_send_bulk(isp, ipc_domain, &msg, 1);
> >> + return ipu6_buttress_ipc_send_bulk(isp, &msg, 1);
> >> }
> >>
> >> static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
> >> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
> >> }
> >>
> >> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
> >> - dev_dbg(&isp->pdev->dev,
> >> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> >> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
> >> - complete(&b->ish.recv_complete);
> >> + dev_warn(&isp->pdev->dev,
> >> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> >
> > I think this is a unrelated change, right?
>
> I mean the change from dev_dbg() to dev_warn().
We're not handling these interrupts anymore in any way.
I wonder if the ipu6_buttress_ipc_recv() call should still remain in place,
even if we really do nothing with these. It looks like some kind of an
acknowledgement mechanism.
> >
> >> }
> >>
> >> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE) {
> >> @@ -399,9 +393,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
> >> }
> >>
> >> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH) {
> >> - dev_dbg(&isp->pdev->dev,
> >> - "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
> >> - complete(&b->ish.send_complete);
> >> + dev_warn(&isp->pdev->dev,
> >> + "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
> >
> > Ditto.
> >
> >> }
> >>
> >> if (irq_status & BUTTRESS_ISR_SAI_VIOLATION &&
> >> @@ -655,7 +648,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
> >> */
> >> dev_info(&isp->pdev->dev, "Sending BOOT_LOAD to CSE\n");
> >>
> >> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
> >> + ret = ipu6_buttress_ipc_send(isp,
> >> BUTTRESS_IU2CSEDATA0_IPC_BOOT_LOAD,
> >> 1, true,
> >> BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
> >> @@ -697,7 +690,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
> >> * IU2CSEDB.IU2CSECMD and set IU2CSEDB.IU2CSEBUSY as
> >> */
> >> dev_info(&isp->pdev->dev, "Sending AUTHENTICATE_RUN to CSE\n");
> >> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
> >> + ret = ipu6_buttress_ipc_send(isp,
> >> BUTTRESS_IU2CSEDATA0_IPC_AUTH_RUN,
> >> 1, true,
> >> BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
> >> @@ -838,9 +831,7 @@ int ipu6_buttress_init(struct ipu6_device *isp)
> >> mutex_init(&b->auth_mutex);
> >> mutex_init(&b->cons_mutex);
> >> mutex_init(&b->ipc_mutex);
> >> - init_completion(&b->ish.send_complete);
> >> init_completion(&b->cse.send_complete);
> >> - init_completion(&b->ish.recv_complete);
> >> init_completion(&b->cse.recv_complete);
> >>
> >> b->cse.nack = BUTTRESS_CSE2IUDATA0_IPC_NACK;
> >> @@ -852,8 +843,6 @@ int ipu6_buttress_init(struct ipu6_device *isp)
> >> b->cse.data0_in = BUTTRESS_REG_CSE2IUDATA0;
> >> b->cse.data0_out = BUTTRESS_REG_IU2CSEDATA0;
> >>
> >> - /* no ISH on IPU6 */
> >> - memset(&b->ish, 0, sizeof(b->ish));
> >> INIT_LIST_HEAD(&b->constraints);
> >>
> >> isp->secure_mode = ipu6_buttress_get_secure_mode(isp);
> >> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
> >> index 9b6f56958be7..482978c2a09d 100644
> >> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h
> >> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
> >> @@ -46,18 +46,12 @@ struct ipu6_buttress_ipc {
> >> struct ipu6_buttress {
> >> struct mutex power_mutex, auth_mutex, cons_mutex, ipc_mutex;
> >> struct ipu6_buttress_ipc cse;
> >> - struct ipu6_buttress_ipc ish;
> >> struct list_head constraints;
> >> u32 wdt_cached_value;
> >> bool force_suspend;
> >> u32 ref_clk;
> >> };
> >>
> >> -enum ipu6_buttress_ipc_domain {
> >> - IPU6_BUTTRESS_IPC_CSE,
> >> - IPU6_BUTTRESS_IPC_ISH,
> >> -};
> >> -
> >> struct ipu6_ipc_buttress_bulk_msg {
> >> u32 cmd;
> >> u32 expected_resp;
> >>
> >
> > Besides minor comment:
> >
> > Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
> >
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-11-01 7:46 ` Sakari Ailus
@ 2024-11-01 7:47 ` Bingbu Cao
2024-11-01 8:19 ` Sakari Ailus
0 siblings, 1 reply; 9+ messages in thread
From: Bingbu Cao @ 2024-11-01 7:47 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Stanislaw Gruszka, linux-media, Bingbu Cao
Sakari and Stanislaw,
On 11/1/24 3:46 PM, Sakari Ailus wrote:
> Hi Bingbu, Stanislaw,
>
> On Fri, Nov 01, 2024 at 12:26:52PM +0800, Bingbu Cao wrote:
>>
>> On 11/1/24 12:22 PM, Bingbu Cao wrote:
>>> Stanislaw,
>>>
>>> Thanks for the patch, this is what I intended to do.
>>>
>>> On 10/31/24 9:06 PM, Stanislaw Gruszka wrote:
>>>> The buttress ipc ish structure is not effectively used on IPU6 - data
>>>> is nullified on init. Remove it to cleanup the code a bit.
>>>>
>>>> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>>>> ---
>>>> v2: fix formatting: use media: prefix in topic and white space alignment
>>>> to match open parenthesis
>>>>
>>>> drivers/media/pci/intel/ipu6/ipu6-buttress.c | 27 ++++++--------------
>>>> drivers/media/pci/intel/ipu6/ipu6-buttress.h | 6 -----
>>>> 2 files changed, 8 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>>>> index edaa285283a1..6644fd4c3d91 100644
>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>>>> @@ -214,20 +214,17 @@ static void ipu6_buttress_ipc_recv(struct ipu6_device *isp,
>>>> }
>>>>
>>>> static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
>>>> - enum ipu6_buttress_ipc_domain ipc_domain,
>>>> struct ipu6_ipc_buttress_bulk_msg *msgs,
>>>> u32 size)
>>>> {
>>>> unsigned long tx_timeout_jiffies, rx_timeout_jiffies;
>>>> unsigned int i, retry = BUTTRESS_IPC_CMD_SEND_RETRY;
>>>> struct ipu6_buttress *b = &isp->buttress;
>>>> - struct ipu6_buttress_ipc *ipc;
>>>> + struct ipu6_buttress_ipc *ipc = &b->cse;
>>>> u32 val;
>>>> int ret;
>>>> int tout;
>>>>
>>>> - ipc = ipc_domain == IPU6_BUTTRESS_IPC_CSE ? &b->cse : &b->ish;
>>>> -
>>>> mutex_lock(&b->ipc_mutex);
>>>>
>>>> ret = ipu6_buttress_ipc_validity_open(isp, ipc);
>>>> @@ -305,7 +302,6 @@ static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
>>>>
>>>> static int
>>>> ipu6_buttress_ipc_send(struct ipu6_device *isp,
>>>> - enum ipu6_buttress_ipc_domain ipc_domain,
>>>> u32 ipc_msg, u32 size, bool require_resp,
>>>> u32 expected_resp)
>>>> {
>>>> @@ -316,7 +312,7 @@ ipu6_buttress_ipc_send(struct ipu6_device *isp,
>>>> .expected_resp = expected_resp,
>>>> };
>>>>
>>>> - return ipu6_buttress_ipc_send_bulk(isp, ipc_domain, &msg, 1);
>>>> + return ipu6_buttress_ipc_send_bulk(isp, &msg, 1);
>>>> }
>>>>
>>>> static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
>>>> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
>>>> - dev_dbg(&isp->pdev->dev,
>>>> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
>>>> - complete(&b->ish.recv_complete);
>>>> + dev_warn(&isp->pdev->dev,
>>>> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>
>>> I think this is a unrelated change, right?
>>
>> I mean the change from dev_dbg() to dev_warn().
>
> We're not handling these interrupts anymore in any way.
>
> I wonder if the ipu6_buttress_ipc_recv() call should still remain in place,
> even if we really do nothing with these. It looks like some kind of an
> acknowledgement mechanism.
I just confirm that IPC_FROM_ISH_IS_WAITING and IPC_EXEC_DONE_BY_ISH are
not valid anymore from IPU6, I think the handling here and below could be
removed.
>
>>>
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE) {
>>>> @@ -399,9 +393,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH) {
>>>> - dev_dbg(&isp->pdev->dev,
>>>> - "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
>>>> - complete(&b->ish.send_complete);
>>>> + dev_warn(&isp->pdev->dev,
>>>> + "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
>>>
>>> Ditto.
>>>
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_SAI_VIOLATION &&
>>>> @@ -655,7 +648,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
>>>> */
>>>> dev_info(&isp->pdev->dev, "Sending BOOT_LOAD to CSE\n");
>>>>
>>>> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
>>>> + ret = ipu6_buttress_ipc_send(isp,
>>>> BUTTRESS_IU2CSEDATA0_IPC_BOOT_LOAD,
>>>> 1, true,
>>>> BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
>>>> @@ -697,7 +690,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
>>>> * IU2CSEDB.IU2CSECMD and set IU2CSEDB.IU2CSEBUSY as
>>>> */
>>>> dev_info(&isp->pdev->dev, "Sending AUTHENTICATE_RUN to CSE\n");
>>>> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
>>>> + ret = ipu6_buttress_ipc_send(isp,
>>>> BUTTRESS_IU2CSEDATA0_IPC_AUTH_RUN,
>>>> 1, true,
>>>> BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
>>>> @@ -838,9 +831,7 @@ int ipu6_buttress_init(struct ipu6_device *isp)
>>>> mutex_init(&b->auth_mutex);
>>>> mutex_init(&b->cons_mutex);
>>>> mutex_init(&b->ipc_mutex);
>>>> - init_completion(&b->ish.send_complete);
>>>> init_completion(&b->cse.send_complete);
>>>> - init_completion(&b->ish.recv_complete);
>>>> init_completion(&b->cse.recv_complete);
>>>>
>>>> b->cse.nack = BUTTRESS_CSE2IUDATA0_IPC_NACK;
>>>> @@ -852,8 +843,6 @@ int ipu6_buttress_init(struct ipu6_device *isp)
>>>> b->cse.data0_in = BUTTRESS_REG_CSE2IUDATA0;
>>>> b->cse.data0_out = BUTTRESS_REG_IU2CSEDATA0;
>>>>
>>>> - /* no ISH on IPU6 */
>>>> - memset(&b->ish, 0, sizeof(b->ish));
>>>> INIT_LIST_HEAD(&b->constraints);
>>>>
>>>> isp->secure_mode = ipu6_buttress_get_secure_mode(isp);
>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>>>> index 9b6f56958be7..482978c2a09d 100644
>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>>>> @@ -46,18 +46,12 @@ struct ipu6_buttress_ipc {
>>>> struct ipu6_buttress {
>>>> struct mutex power_mutex, auth_mutex, cons_mutex, ipc_mutex;
>>>> struct ipu6_buttress_ipc cse;
>>>> - struct ipu6_buttress_ipc ish;
>>>> struct list_head constraints;
>>>> u32 wdt_cached_value;
>>>> bool force_suspend;
>>>> u32 ref_clk;
>>>> };
>>>>
>>>> -enum ipu6_buttress_ipc_domain {
>>>> - IPU6_BUTTRESS_IPC_CSE,
>>>> - IPU6_BUTTRESS_IPC_ISH,
>>>> -};
>>>> -
>>>> struct ipu6_ipc_buttress_bulk_msg {
>>>> u32 cmd;
>>>> u32 expected_resp;
>>>>
>>>
>>> Besides minor comment:
>>>
>>> Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
>>>
>>
>
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-11-01 7:47 ` Bingbu Cao
@ 2024-11-01 8:19 ` Sakari Ailus
2024-11-01 9:07 ` Bingbu Cao
0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2024-11-01 8:19 UTC (permalink / raw)
To: Bingbu Cao; +Cc: Stanislaw Gruszka, linux-media, Bingbu Cao
Hi Bingbu,
On Fri, Nov 01, 2024 at 03:47:54PM +0800, Bingbu Cao wrote:
> Sakari and Stanislaw,
>
> On 11/1/24 3:46 PM, Sakari Ailus wrote:
> >>>> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
> >>>> }
> >>>>
> >>>> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
> >>>> - dev_dbg(&isp->pdev->dev,
> >>>> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> >>>> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
> >>>> - complete(&b->ish.recv_complete);
> >>>> + dev_warn(&isp->pdev->dev,
> >>>> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> >>>
> >>> I think this is a unrelated change, right?
> >>
> >> I mean the change from dev_dbg() to dev_warn().
> >
> > We're not handling these interrupts anymore in any way.
> >
> > I wonder if the ipu6_buttress_ipc_recv() call should still remain in place,
> > even if we really do nothing with these. It looks like some kind of an
> > acknowledgement mechanism.
>
> I just confirm that IPC_FROM_ISH_IS_WAITING and IPC_EXEC_DONE_BY_ISH are
> not valid anymore from IPU6, I think the handling here and below could be
> removed.
Do you know which IPU version still needed it?
There are folks who'd like to add IPU4 support to the driver but they can
add it back if it's needed.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-11-01 8:19 ` Sakari Ailus
@ 2024-11-01 9:07 ` Bingbu Cao
2024-11-04 9:19 ` Stanislaw Gruszka
0 siblings, 1 reply; 9+ messages in thread
From: Bingbu Cao @ 2024-11-01 9:07 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Stanislaw Gruszka, linux-media, Bingbu Cao
On 11/1/24 4:19 PM, Sakari Ailus wrote:
> Hi Bingbu,
>
> On Fri, Nov 01, 2024 at 03:47:54PM +0800, Bingbu Cao wrote:
>> Sakari and Stanislaw,
>>
>> On 11/1/24 3:46 PM, Sakari Ailus wrote:
>>>>>> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>>>>>> }
>>>>>>
>>>>>> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
>>>>>> - dev_dbg(&isp->pdev->dev,
>>>>>> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>>>> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
>>>>>> - complete(&b->ish.recv_complete);
>>>>>> + dev_warn(&isp->pdev->dev,
>>>>>> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>>>
>>>>> I think this is a unrelated change, right?
>>>>
>>>> I mean the change from dev_dbg() to dev_warn().
>>>
>>> We're not handling these interrupts anymore in any way.
>>>
>>> I wonder if the ipu6_buttress_ipc_recv() call should still remain in place,
>>> even if we really do nothing with these. It looks like some kind of an
>>> acknowledgement mechanism.
>>
>> I just confirm that IPC_FROM_ISH_IS_WAITING and IPC_EXEC_DONE_BY_ISH are
>> not valid anymore from IPU6, I think the handling here and below could be
>> removed.
>
> Do you know which IPU version still needed it?
>
> There are folks who'd like to add IPU4 support to the driver but they can
> add it back if it's needed.
>
I know that ISH IPC was added from IPU4, but I am not sure IPU4 really
need that now.
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-11-01 9:07 ` Bingbu Cao
@ 2024-11-04 9:19 ` Stanislaw Gruszka
2024-11-05 2:47 ` Bingbu Cao
0 siblings, 1 reply; 9+ messages in thread
From: Stanislaw Gruszka @ 2024-11-04 9:19 UTC (permalink / raw)
To: Bingbu Cao; +Cc: Sakari Ailus, linux-media, Bingbu Cao
On Fri, Nov 01, 2024 at 05:07:25PM +0800, Bingbu Cao wrote:
>
> On 11/1/24 4:19 PM, Sakari Ailus wrote:
> > Hi Bingbu,
> >
> > On Fri, Nov 01, 2024 at 03:47:54PM +0800, Bingbu Cao wrote:
> >> Sakari and Stanislaw,
> >>
> >> On 11/1/24 3:46 PM, Sakari Ailus wrote:
> >>>>>> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
> >>>>>> }
> >>>>>>
> >>>>>> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
> >>>>>> - dev_dbg(&isp->pdev->dev,
> >>>>>> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> >>>>>> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
> >>>>>> - complete(&b->ish.recv_complete);
> >>>>>> + dev_warn(&isp->pdev->dev,
> >>>>>> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> >>>>>
> >>>>> I think this is a unrelated change, right?
> >>>>
> >>>> I mean the change from dev_dbg() to dev_warn().
> >>>
> >>> We're not handling these interrupts anymore in any way.
> >>>
> >>> I wonder if the ipu6_buttress_ipc_recv() call should still remain in place,
> >>> even if we really do nothing with these. It looks like some kind of an
> >>> acknowledgement mechanism.
> >>
> >> I just confirm that IPC_FROM_ISH_IS_WAITING and IPC_EXEC_DONE_BY_ISH are
> >> not valid anymore from IPU6, I think the handling here and below could be
> >> removed.
> >
> > Do you know which IPU version still needed it?
> >
> > There are folks who'd like to add IPU4 support to the driver but they can
> > add it back if it's needed.
> >
>
> I know that ISH IPC was added from IPU4, but I am not sure IPU4 really
> need that now.
Ok, I think on v3, I'll remove handling of BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING
and BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH from isr, but will keep the BIT's
definitions just in case.
Regards
Stanislaw
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] media: intel/ipu6: remove buttress ish structure
2024-11-04 9:19 ` Stanislaw Gruszka
@ 2024-11-05 2:47 ` Bingbu Cao
0 siblings, 0 replies; 9+ messages in thread
From: Bingbu Cao @ 2024-11-05 2:47 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Sakari Ailus, linux-media, Bingbu Cao
On 11/4/24 5:19 PM, Stanislaw Gruszka wrote:
> On Fri, Nov 01, 2024 at 05:07:25PM +0800, Bingbu Cao wrote:
>>
>> On 11/1/24 4:19 PM, Sakari Ailus wrote:
>>> Hi Bingbu,
>>>
>>> On Fri, Nov 01, 2024 at 03:47:54PM +0800, Bingbu Cao wrote:
>>>> Sakari and Stanislaw,
>>>>
>>>> On 11/1/24 3:46 PM, Sakari Ailus wrote:
>>>>>>>> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>>>>>>>> }
>>>>>>>>
>>>>>>>> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
>>>>>>>> - dev_dbg(&isp->pdev->dev,
>>>>>>>> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>>>>>> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
>>>>>>>> - complete(&b->ish.recv_complete);
>>>>>>>> + dev_warn(&isp->pdev->dev,
>>>>>>>> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>>>>>
>>>>>>> I think this is a unrelated change, right?
>>>>>>
>>>>>> I mean the change from dev_dbg() to dev_warn().
>>>>>
>>>>> We're not handling these interrupts anymore in any way.
>>>>>
>>>>> I wonder if the ipu6_buttress_ipc_recv() call should still remain in place,
>>>>> even if we really do nothing with these. It looks like some kind of an
>>>>> acknowledgement mechanism.
>>>>
>>>> I just confirm that IPC_FROM_ISH_IS_WAITING and IPC_EXEC_DONE_BY_ISH are
>>>> not valid anymore from IPU6, I think the handling here and below could be
>>>> removed.
>>>
>>> Do you know which IPU version still needed it?
>>>
>>> There are folks who'd like to add IPU4 support to the driver but they can
>>> add it back if it's needed.
>>>
>>
>> I know that ISH IPC was added from IPU4, but I am not sure IPU4 really
>> need that now.
>
> Ok, I think on v3, I'll remove handling of BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING
> and BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH from isr, but will keep the BIT's
> definitions just in case.
Thanks!
>
> Regards
> Stanislaw
>
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-11-05 2:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-31 13:06 [PATCH v2] media: intel/ipu6: remove buttress ish structure Stanislaw Gruszka
2024-11-01 4:22 ` Bingbu Cao
2024-11-01 4:26 ` Bingbu Cao
2024-11-01 7:46 ` Sakari Ailus
2024-11-01 7:47 ` Bingbu Cao
2024-11-01 8:19 ` Sakari Ailus
2024-11-01 9:07 ` Bingbu Cao
2024-11-04 9:19 ` Stanislaw Gruszka
2024-11-05 2:47 ` Bingbu Cao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.