| Message ID | 20250123-v4h-iif-v1-3-7b4e5299939f@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: renesas: vsp1: Add support for IIF | expand |
Hi Jacopo, Thank you for the patch. On Thu, Jan 23, 2025 at 06:04:04PM +0100, Jacopo Mondi wrote: > The vsp1_dl library allows to program a display list and feed it > continuously to the VSP2. As an alternative operation mode, the library > allows to program the VSP2 in 'single shot' mode, where a display list > is submitted to the VSP on request only. > > Currently the 'single shot' mode is only available when the VSP2 is > controlled by userspace, while it works in continuous mode when driven > by DRM, as frames have to be submitted to the display continuously. > > For the VSPX use case, where there is no uapi support, we should however > work in single-shot mode as the ISP driver programs the VSPX on > request. > > Initialize the display lists in single shot mode in case the VSP1 is > controlled by userspace or in case the pipeline features an IIF entity, > as in the VSPX case. > > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com> > --- > drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c > index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c > @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, > return NULL; > > dlm->index = index; > - dlm->singleshot = vsp1->info->uapi; > + /* > + * uapi = single shot mode; > + * DRM = continuous mode; > + * VSPX = single shot mode; > + */ > + dlm->singleshot = (vsp1->info->uapi || vsp1->iif); I'm wondering if we should make this a bit more generic, and allow the caller to select the mode of operation. It could be passed as a flag to vsp1_dl_list_commit() and stored in the vsp1_dl_list. There is however no use case at the moment to switch between singleshot and continuous modes on a per display list basis. Implementing support for that may be overkill, but on the other hand, the implementation seems very simple, so it's not a big effort. The main and possibly only reason why we may not want to do that now is because the display list helpers haven't been tested to successfully switch between the modes, so we may falsely advertise something as supported. Despite that, as no caller would attempt switching, it wouldn't cause an issue. What do you think ? What do you feel would be cleaner ? > dlm->vsp1 = vsp1; > > spin_lock_init(&dlm->lock);
Hi Laurent On Thu, Jan 23, 2025 at 11:44:45PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Jan 23, 2025 at 06:04:04PM +0100, Jacopo Mondi wrote: > > The vsp1_dl library allows to program a display list and feed it > > continuously to the VSP2. As an alternative operation mode, the library > > allows to program the VSP2 in 'single shot' mode, where a display list > > is submitted to the VSP on request only. > > > > Currently the 'single shot' mode is only available when the VSP2 is > > controlled by userspace, while it works in continuous mode when driven > > by DRM, as frames have to be submitted to the display continuously. > > > > For the VSPX use case, where there is no uapi support, we should however > > work in single-shot mode as the ISP driver programs the VSPX on > > request. > > > > Initialize the display lists in single shot mode in case the VSP1 is > > controlled by userspace or in case the pipeline features an IIF entity, > > as in the VSPX case. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com> > > --- > > drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c > > index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644 > > --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c > > @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, > > return NULL; > > > > dlm->index = index; > > - dlm->singleshot = vsp1->info->uapi; > > + /* > > + * uapi = single shot mode; > > + * DRM = continuous mode; > > + * VSPX = single shot mode; > > + */ > > + dlm->singleshot = (vsp1->info->uapi || vsp1->iif); > > I'm wondering if we should make this a bit more generic, and allow the > caller to select the mode of operation. It could be passed as a flag to > vsp1_dl_list_commit() and stored in the vsp1_dl_list. > > There is however no use case at the moment to switch between singleshot > and continuous modes on a per display list basis. Implementing support > for that may be overkill, but on the other hand, the implementation > seems very simple, so it's not a big effort. The main and possibly only > reason why we may not want to do that now is because the display list > helpers haven't been tested to successfully switch between the modes, so > we may falsely advertise something as supported. Despite that, as no > caller would attempt switching, it wouldn't cause an issue. I would simply avoid upstreaming code I can't test, and changing the singleshot mode between commit might create contentions with vsp1_dlm_irq_frame_end() which inspects dlm->singleshot. > > What do you think ? What do you feel would be cleaner ? > > > dlm->vsp1 = vsp1; > > > > spin_lock_init(&dlm->lock); > > -- > Regards, > > Laurent Pinchart
On Fri, Jan 24, 2025 at 09:44:06AM +0100, Jacopo Mondi wrote: > Hi Laurent > > On Thu, Jan 23, 2025 at 11:44:45PM +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Thu, Jan 23, 2025 at 06:04:04PM +0100, Jacopo Mondi wrote: > > > The vsp1_dl library allows to program a display list and feed it > > > continuously to the VSP2. As an alternative operation mode, the library > > > allows to program the VSP2 in 'single shot' mode, where a display list > > > is submitted to the VSP on request only. > > > > > > Currently the 'single shot' mode is only available when the VSP2 is > > > controlled by userspace, while it works in continuous mode when driven > > > by DRM, as frames have to be submitted to the display continuously. > > > > > > For the VSPX use case, where there is no uapi support, we should however > > > work in single-shot mode as the ISP driver programs the VSPX on > > > request. > > > > > > Initialize the display lists in single shot mode in case the VSP1 is > > > controlled by userspace or in case the pipeline features an IIF entity, > > > as in the VSPX case. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com> > > > --- > > > drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c > > > index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644 > > > --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c > > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c > > > @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, > > > return NULL; > > > > > > dlm->index = index; > > > - dlm->singleshot = vsp1->info->uapi; > > > + /* > > > + * uapi = single shot mode; > > > + * DRM = continuous mode; > > > + * VSPX = single shot mode; > > > + */ > > > + dlm->singleshot = (vsp1->info->uapi || vsp1->iif); > > > > I'm wondering if we should make this a bit more generic, and allow the > > caller to select the mode of operation. It could be passed as a flag to > > vsp1_dl_list_commit() and stored in the vsp1_dl_list. > > > > There is however no use case at the moment to switch between singleshot > > and continuous modes on a per display list basis. Implementing support > > for that may be overkill, but on the other hand, the implementation > > seems very simple, so it's not a big effort. The main and possibly only > > reason why we may not want to do that now is because the display list > > helpers haven't been tested to successfully switch between the modes, so > > we may falsely advertise something as supported. Despite that, as no > > caller would attempt switching, it wouldn't cause an issue. > > I would simply avoid upstreaming code I can't test, and changing the > singleshot mode between commit might create contentions with > vsp1_dlm_irq_frame_end() which inspects dlm->singleshot. I had considered that when looking at the code. Moving the single shot flag to the vsp1_dl_list, vsp1_dlm_irq_frame_end() would check the flag from that structure instead of getting it from the dlm, so I don't think it would be an issue. That's the reason why I'm in two minds about this, I think it would be easy to do, with very low risk for our use cases, but indeed the switch itself wouldn't be fully tested. > > What do you think ? What do you feel would be cleaner ? > > > > > dlm->vsp1 = vsp1; > > > > > > spin_lock_init(&dlm->lock);
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, return NULL; dlm->index = index; - dlm->singleshot = vsp1->info->uapi; + /* + * uapi = single shot mode; + * DRM = continuous mode; + * VSPX = single shot mode; + */ + dlm->singleshot = (vsp1->info->uapi || vsp1->iif); dlm->vsp1 = vsp1; spin_lock_init(&dlm->lock);
The vsp1_dl library allows to program a display list and feed it continuously to the VSP2. As an alternative operation mode, the library allows to program the VSP2 in 'single shot' mode, where a display list is submitted to the VSP on request only. Currently the 'single shot' mode is only available when the VSP2 is controlled by userspace, while it works in continuous mode when driven by DRM, as frames have to be submitted to the display continuously. For the VSPX use case, where there is no uapi support, we should however work in single-shot mode as the ISP driver programs the VSPX on request. Initialize the display lists in single shot mode in case the VSP1 is controlled by userspace or in case the pipeline features an IIF entity, as in the VSPX case. Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com> --- drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)