Re: [PATCH v2 6/8] v4l: vsp1: Adapt entities to configure into a body
Hi Laurent, On 12/09/17 20:18, Laurent Pinchart wrote: > Hi Kieran, > > On Tuesday, 12 September 2017 00:42:09 EEST Kieran Bingham wrote: >> On 17/08/17 18:58, Laurent Pinchart wrote: >>> On Monday 14 Aug 2017 16:13:29 Kieran Bingham wrote: Currently the entities store their configurations into a display list. Adapt this such that the code can be configured into a body fragment directly, allowing greater flexibility and control of the content. All users of vsp1_dl_list_write() are removed in this process, thus it too is removed. A helper, vsp1_dl_list_body() is provided to access the internal body0 from the display list. Signed-off-by: Kieran Bingham --- drivers/media/platform/vsp1/vsp1_bru.c| 22 ++-- drivers/media/platform/vsp1/vsp1_clu.c| 22 ++-- drivers/media/platform/vsp1/vsp1_dl.c | 12 ++- drivers/media/platform/vsp1/vsp1_dl.h | 2 +- drivers/media/platform/vsp1/vsp1_drm.c| 14 +--- drivers/media/platform/vsp1/vsp1_entity.c | 16 - drivers/media/platform/vsp1/vsp1_entity.h | 12 --- drivers/media/platform/vsp1/vsp1_hgo.c| 16 - drivers/media/platform/vsp1/vsp1_hgt.c| 18 +- drivers/media/platform/vsp1/vsp1_hsit.c | 10 +++--- drivers/media/platform/vsp1/vsp1_lif.c| 13 +++ drivers/media/platform/vsp1/vsp1_lut.c| 21 ++-- drivers/media/platform/vsp1/vsp1_pipe.c | 4 +- drivers/media/platform/vsp1/vsp1_pipe.h | 3 +- drivers/media/platform/vsp1/vsp1_rpf.c| 43 +++- drivers/media/platform/vsp1/vsp1_sru.c| 14 drivers/media/platform/vsp1/vsp1_uds.c| 24 +++-- drivers/media/platform/vsp1/vsp1_uds.h| 2 +- drivers/media/platform/vsp1/vsp1_video.c | 11 -- drivers/media/platform/vsp1/vsp1_wpf.c| 42 --- 20 files changed, 168 insertions(+), 153 deletions(-) >>> >>> This is quite intrusive, and it bothers me slightly that we need to pass >>> both the DL and the DLB to the configure function in order to add >>> fragments to the DL in the CLU and LUT modules. Wouldn't it be simpler to >>> add a pointer to the current body in the DL structure, and modify >>> vsp1_dl_list_write() to write to the current fragment ? >> >> No doubt about it, 168+, 153- is certainly intrusive. >> >> Yes, now I'm looking back at this, I think this does look like this part is >> not quite the right approach. >> >> Which otherwise stalls the series until I have time to reconsider. I will >> likely repost the work I have done on the earlier patches, including the >> 's/fragment/body/g' changes and ready myself for a v4 which will contain the >> heavier reworks. > > Fine with me. Could you make sure to mention the open issues in the cover > letter ? I want to avoid commenting on them if you know already that you will > rework them later. I've been trying to tackle this today, but I think I've come up a bit stuck on a key part. The reason for this patch, is to allow the functions to configure directly into a display list body, even when that body *is not part* of a display list. So - converting vsp1_dl_list_write() to configure into the 'current' body (was fragment) of a display list would not work for writing to the cached objects - which do not have a display list. They are simply body objects. It seems a bit extraneous to create holding display lists to contain a single body, when the display list itself will never be used, but I can't think of an alternative. Would you prefer this 'container display list' approach? or do you have another idea? -- Regards Kieran
Re: [PATCH v2 6/8] v4l: vsp1: Adapt entities to configure into a body
Hi Kieran, On Tuesday, 12 September 2017 00:42:09 EEST Kieran Bingham wrote: > On 17/08/17 18:58, Laurent Pinchart wrote: > > On Monday 14 Aug 2017 16:13:29 Kieran Bingham wrote: > >> Currently the entities store their configurations into a display list. > >> Adapt this such that the code can be configured into a body fragment > >> directly, allowing greater flexibility and control of the content. > >> > >> All users of vsp1_dl_list_write() are removed in this process, thus it > >> too is removed. > >> > >> A helper, vsp1_dl_list_body() is provided to access the internal body0 > >> from the display list. > >> > >> Signed-off-by: Kieran Bingham > >> --- > >> > >> drivers/media/platform/vsp1/vsp1_bru.c| 22 ++-- > >> drivers/media/platform/vsp1/vsp1_clu.c| 22 ++-- > >> drivers/media/platform/vsp1/vsp1_dl.c | 12 ++- > >> drivers/media/platform/vsp1/vsp1_dl.h | 2 +- > >> drivers/media/platform/vsp1/vsp1_drm.c| 14 +--- > >> drivers/media/platform/vsp1/vsp1_entity.c | 16 - > >> drivers/media/platform/vsp1/vsp1_entity.h | 12 --- > >> drivers/media/platform/vsp1/vsp1_hgo.c| 16 - > >> drivers/media/platform/vsp1/vsp1_hgt.c| 18 +- > >> drivers/media/platform/vsp1/vsp1_hsit.c | 10 +++--- > >> drivers/media/platform/vsp1/vsp1_lif.c| 13 +++ > >> drivers/media/platform/vsp1/vsp1_lut.c| 21 ++-- > >> drivers/media/platform/vsp1/vsp1_pipe.c | 4 +- > >> drivers/media/platform/vsp1/vsp1_pipe.h | 3 +- > >> drivers/media/platform/vsp1/vsp1_rpf.c| 43 +++- > >> drivers/media/platform/vsp1/vsp1_sru.c| 14 > >> drivers/media/platform/vsp1/vsp1_uds.c| 24 +++-- > >> drivers/media/platform/vsp1/vsp1_uds.h| 2 +- > >> drivers/media/platform/vsp1/vsp1_video.c | 11 -- > >> drivers/media/platform/vsp1/vsp1_wpf.c| 42 --- > >> 20 files changed, 168 insertions(+), 153 deletions(-) > > > > This is quite intrusive, and it bothers me slightly that we need to pass > > both the DL and the DLB to the configure function in order to add > > fragments to the DL in the CLU and LUT modules. Wouldn't it be simpler to > > add a pointer to the current body in the DL structure, and modify > > vsp1_dl_list_write() to write to the current fragment ? > > No doubt about it, 168+, 153- is certainly intrusive. > > Yes, now I'm looking back at this, I think this does look like this part is > not quite the right approach. > > Which otherwise stalls the series until I have time to reconsider. I will > likely repost the work I have done on the earlier patches, including the > 's/fragment/body/g' changes and ready myself for a v4 which will contain the > heavier reworks. Fine with me. Could you make sure to mention the open issues in the cover letter ? I want to avoid commenting on them if you know already that you will rework them later. -- Regards, Laurent Pinchart
Re: [PATCH v2 6/8] v4l: vsp1: Adapt entities to configure into a body
Hi Laurent, On 17/08/17 18:58, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Monday 14 Aug 2017 16:13:29 Kieran Bingham wrote: >> Currently the entities store their configurations into a display list. >> Adapt this such that the code can be configured into a body fragment >> directly, allowing greater flexibility and control of the content. >> >> All users of vsp1_dl_list_write() are removed in this process, thus it >> too is removed. >> >> A helper, vsp1_dl_list_body() is provided to access the internal body0 >> from the display list. >> >> Signed-off-by: Kieran Bingham >> --- >> drivers/media/platform/vsp1/vsp1_bru.c| 22 ++-- >> drivers/media/platform/vsp1/vsp1_clu.c| 22 ++-- >> drivers/media/platform/vsp1/vsp1_dl.c | 12 ++- >> drivers/media/platform/vsp1/vsp1_dl.h | 2 +- >> drivers/media/platform/vsp1/vsp1_drm.c| 14 +--- >> drivers/media/platform/vsp1/vsp1_entity.c | 16 - >> drivers/media/platform/vsp1/vsp1_entity.h | 12 --- >> drivers/media/platform/vsp1/vsp1_hgo.c| 16 - >> drivers/media/platform/vsp1/vsp1_hgt.c| 18 +- >> drivers/media/platform/vsp1/vsp1_hsit.c | 10 +++--- >> drivers/media/platform/vsp1/vsp1_lif.c| 13 +++ >> drivers/media/platform/vsp1/vsp1_lut.c| 21 ++-- >> drivers/media/platform/vsp1/vsp1_pipe.c | 4 +- >> drivers/media/platform/vsp1/vsp1_pipe.h | 3 +- >> drivers/media/platform/vsp1/vsp1_rpf.c| 43 +++- >> drivers/media/platform/vsp1/vsp1_sru.c| 14 >> drivers/media/platform/vsp1/vsp1_uds.c| 24 +++-- >> drivers/media/platform/vsp1/vsp1_uds.h| 2 +- >> drivers/media/platform/vsp1/vsp1_video.c | 11 -- >> drivers/media/platform/vsp1/vsp1_wpf.c| 42 --- >> 20 files changed, 168 insertions(+), 153 deletions(-) > > This is quite intrusive, and it bothers me slightly that we need to pass both > the DL and the DLB to the configure function in order to add fragments to the > DL in the CLU and LUT modules. Wouldn't it be simpler to add a pointer to the > current body in the DL structure, and modify vsp1_dl_list_write() to write to > the current fragment ? > No doubt about it, 168+, 153- is certainly intrusive. Yes, now I'm looking back at this, I think this does look like this part is not quite the right approach. Which otherwise stalls the series until I have time to reconsider. I will likely repost the work I have done on the earlier patches, including the 's/fragment/body/g' changes and ready myself for a v4 which will contain the heavier reworks. -- Kieran
Re: [PATCH v2 6/8] v4l: vsp1: Adapt entities to configure into a body
Hi Kieran, Thank you for the patch. On Monday 14 Aug 2017 16:13:29 Kieran Bingham wrote: > Currently the entities store their configurations into a display list. > Adapt this such that the code can be configured into a body fragment > directly, allowing greater flexibility and control of the content. > > All users of vsp1_dl_list_write() are removed in this process, thus it > too is removed. > > A helper, vsp1_dl_list_body() is provided to access the internal body0 > from the display list. > > Signed-off-by: Kieran Bingham > --- > drivers/media/platform/vsp1/vsp1_bru.c| 22 ++-- > drivers/media/platform/vsp1/vsp1_clu.c| 22 ++-- > drivers/media/platform/vsp1/vsp1_dl.c | 12 ++- > drivers/media/platform/vsp1/vsp1_dl.h | 2 +- > drivers/media/platform/vsp1/vsp1_drm.c| 14 +--- > drivers/media/platform/vsp1/vsp1_entity.c | 16 - > drivers/media/platform/vsp1/vsp1_entity.h | 12 --- > drivers/media/platform/vsp1/vsp1_hgo.c| 16 - > drivers/media/platform/vsp1/vsp1_hgt.c| 18 +- > drivers/media/platform/vsp1/vsp1_hsit.c | 10 +++--- > drivers/media/platform/vsp1/vsp1_lif.c| 13 +++ > drivers/media/platform/vsp1/vsp1_lut.c| 21 ++-- > drivers/media/platform/vsp1/vsp1_pipe.c | 4 +- > drivers/media/platform/vsp1/vsp1_pipe.h | 3 +- > drivers/media/platform/vsp1/vsp1_rpf.c| 43 +++- > drivers/media/platform/vsp1/vsp1_sru.c| 14 > drivers/media/platform/vsp1/vsp1_uds.c| 24 +++-- > drivers/media/platform/vsp1/vsp1_uds.h| 2 +- > drivers/media/platform/vsp1/vsp1_video.c | 11 -- > drivers/media/platform/vsp1/vsp1_wpf.c| 42 --- > 20 files changed, 168 insertions(+), 153 deletions(-) This is quite intrusive, and it bothers me slightly that we need to pass both the DL and the DLB to the configure function in order to add fragments to the DL in the CLU and LUT modules. Wouldn't it be simpler to add a pointer to the current body in the DL structure, and modify vsp1_dl_list_write() to write to the current fragment ? -- Regards, Laurent Pinchart
[PATCH v2 6/8] v4l: vsp1: Adapt entities to configure into a body
Currently the entities store their configurations into a display list.
Adapt this such that the code can be configured into a body fragment
directly, allowing greater flexibility and control of the content.
All users of vsp1_dl_list_write() are removed in this process, thus it
too is removed.
A helper, vsp1_dl_list_body() is provided to access the internal body0
from the display list.
Signed-off-by: Kieran Bingham
---
drivers/media/platform/vsp1/vsp1_bru.c| 22 ++--
drivers/media/platform/vsp1/vsp1_clu.c| 22 ++--
drivers/media/platform/vsp1/vsp1_dl.c | 12 ++-
drivers/media/platform/vsp1/vsp1_dl.h | 2 +-
drivers/media/platform/vsp1/vsp1_drm.c| 14 +---
drivers/media/platform/vsp1/vsp1_entity.c | 16 -
drivers/media/platform/vsp1/vsp1_entity.h | 12 ---
drivers/media/platform/vsp1/vsp1_hgo.c| 16 -
drivers/media/platform/vsp1/vsp1_hgt.c| 18 +-
drivers/media/platform/vsp1/vsp1_hsit.c | 10 +++---
drivers/media/platform/vsp1/vsp1_lif.c| 13 +++
drivers/media/platform/vsp1/vsp1_lut.c| 21 ++--
drivers/media/platform/vsp1/vsp1_pipe.c | 4 +-
drivers/media/platform/vsp1/vsp1_pipe.h | 3 +-
drivers/media/platform/vsp1/vsp1_rpf.c| 43 +++-
drivers/media/platform/vsp1/vsp1_sru.c| 14
drivers/media/platform/vsp1/vsp1_uds.c| 24 +++--
drivers/media/platform/vsp1/vsp1_uds.h| 2 +-
drivers/media/platform/vsp1/vsp1_video.c | 11 --
drivers/media/platform/vsp1/vsp1_wpf.c| 42 ---
20 files changed, 168 insertions(+), 153 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_bru.c
b/drivers/media/platform/vsp1/vsp1_bru.c
index b9ff96f76b3e..652b42e3ec2d 100644
--- a/drivers/media/platform/vsp1/vsp1_bru.c
+++ b/drivers/media/platform/vsp1/vsp1_bru.c
@@ -30,10 +30,10 @@
* Device Access
*/
-static inline void vsp1_bru_write(struct vsp1_bru *bru, struct vsp1_dl_list
*dl,
- u32 reg, u32 data)
+static inline void vsp1_bru_write(struct vsp1_bru *bru,
+ struct vsp1_dl_body *dlb, u32 reg, u32 data)
{
- vsp1_dl_list_write(dl, bru->base + reg, data);
+ vsp1_dl_fragment_write(dlb, bru->base + reg, data);
}
/*
-
@@ -287,7 +287,7 @@ static const struct v4l2_subdev_ops bru_ops = {
static void bru_prepare(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
- struct vsp1_dl_list *dl)
+ struct vsp1_dl_body *dlb)
{
struct vsp1_bru *bru = to_bru(&entity->subdev);
struct v4l2_mbus_framefmt *format;
@@ -309,7 +309,7 @@ static void bru_prepare(struct vsp1_entity *entity,
* format at the pipeline output is premultiplied.
*/
flags = pipe->output ? pipe->output->format.flags : 0;
- vsp1_bru_write(bru, dl, VI6_BRU_INCTRL,
+ vsp1_bru_write(bru, dlb, VI6_BRU_INCTRL,
flags & V4L2_PIX_FMT_FLAG_PREMUL_ALPHA ?
0 : VI6_BRU_INCTRL_NRM);
@@ -317,12 +317,12 @@ static void bru_prepare(struct vsp1_entity *entity,
* Set the background position to cover the whole output image and
* configure its color.
*/
- vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_SIZE,
+ vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_SIZE,
(format->width << VI6_BRU_VIRRPF_SIZE_HSIZE_SHIFT) |
(format->height << VI6_BRU_VIRRPF_SIZE_VSIZE_SHIFT));
- vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_LOC, 0);
+ vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_LOC, 0);
- vsp1_bru_write(bru, dl, VI6_BRU_VIRRPF_COL, bru->bgcolor |
+ vsp1_bru_write(bru, dlb, VI6_BRU_VIRRPF_COL, bru->bgcolor |
(0xff << VI6_BRU_VIRRPF_COL_A_SHIFT));
/*
@@ -332,7 +332,7 @@ static void bru_prepare(struct vsp1_entity *entity,
* unit.
*/
if (entity->type == VSP1_ENTITY_BRU)
- vsp1_bru_write(bru, dl, VI6_BRU_ROP,
+ vsp1_bru_write(bru, dlb, VI6_BRU_ROP,
VI6_BRU_ROP_DSTSEL_BRUIN(1) |
VI6_BRU_ROP_CROP(VI6_ROP_NOP) |
VI6_BRU_ROP_AROP(VI6_ROP_NOP));
@@ -374,7 +374,7 @@ static void bru_prepare(struct vsp1_entity *entity,
if (!(entity->type == VSP1_ENTITY_BRU && i == 1))
ctrl |= VI6_BRU_CTRL_SRCSEL_BRUIN(i);
- vsp1_bru_write(bru, dl, VI6_BRU_CTRL(i), ctrl);
+ vsp1_bru_write(bru, dlb, VI6_BRU_CTRL(i), ctrl);
/*
* Harcode the blending formula to
@@ -389,7 +389,7 @@ static void bru_prepare(struct vsp1_entity *entity,
*
* otherwise.
*/
- vsp1_bru_write(bru, dl, VI6_BRU_BLD(i)
