* [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.