| Message ID | 20241031130622.430308-1-stanislaw.gruszka@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] media: intel/ipu6: remove buttress ish structure | expand |
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> >
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.
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.
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
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;
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(-)