| Message ID | 20230823104444.1954663-6-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand |
Hi Bryan, Thank you for the patch. On Wed, Aug 23, 2023 at 11:44:34AM +0100, Bryan O'Donoghue wrote: > line_num indicates the number of RDI - raw data interface channels which > are associated with a given IFE/VFE - image/video front end. Should the variable then be renamed to num_rdi or similar ? > On several SoCs the RDI number is not static for each VFE - for example > on sm8250 VFE Lite has four RDIs where regular VFE has three. > > Assigning line_num statically in the subdev_init() phase initialises > each VFE to the lower number, meaning in practical terms that we are > lobbing off one RDI on some VFEs. > > Interrupt handling uses static for (i = RDI0; i < RDI2; i++) {} in some > of our VFE blocks but this can't work for situations where we have a > mixture of VFE @ 3 RDI and VFE-lite @ 4 RDI blocks. > > First step to remediate is to pass line_num from a compat string > controlled data-structure and do so on a per-VFE basis. > > Later patches will assign the correct number of RDI blocks per VFE. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../media/platform/qcom/camss/camss-vfe-170.c | 2 -- > .../media/platform/qcom/camss/camss-vfe-4-1.c | 2 -- > .../media/platform/qcom/camss/camss-vfe-4-7.c | 2 -- > .../media/platform/qcom/camss/camss-vfe-4-8.c | 2 -- > .../media/platform/qcom/camss/camss-vfe-480.c | 1 - > drivers/media/platform/qcom/camss/camss-vfe.c | 5 +++ > drivers/media/platform/qcom/camss/camss.c | 36 ++++++++++++------- > drivers/media/platform/qcom/camss/camss.h | 1 + > 8 files changed, 30 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c > index 9905bb06b3823..8aa921400ded0 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c > @@ -756,8 +756,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) > { > vfe->isr_ops = vfe_isr_ops_170; > vfe->video_ops = vfe_video_ops_170; > - > - vfe->line_num = VFE_LINE_NUM_GEN2; > } > > const struct vfe_hw_ops vfe_ops_170 = { > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c > index bc309f326f519..2911e4126e7ad 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c > @@ -992,8 +992,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) > vfe->isr_ops = vfe_isr_ops_gen1; > vfe->ops_gen1 = &vfe_ops_gen1_4_1; > vfe->video_ops = vfe_video_ops_gen1; > - > - vfe->line_num = VFE_LINE_NUM_GEN1; > } > > const struct vfe_hw_ops vfe_ops_4_1 = { > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c > index 8acd76c9746ba..b65ed0fef595e 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c > @@ -1188,8 +1188,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) > vfe->isr_ops = vfe_isr_ops_gen1; > vfe->ops_gen1 = &vfe_ops_gen1_4_7; > vfe->video_ops = vfe_video_ops_gen1; > - > - vfe->line_num = VFE_LINE_NUM_GEN1; > } > > const struct vfe_hw_ops vfe_ops_4_7 = { > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c > index 3a0167ecf873a..7b3805177f037 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c > @@ -1173,8 +1173,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) > vfe->isr_ops = vfe_isr_ops_gen1; > vfe->ops_gen1 = &vfe_ops_gen1_4_8; > vfe->video_ops = vfe_video_ops_gen1; > - > - vfe->line_num = VFE_LINE_NUM_GEN1; > } > > const struct vfe_hw_ops vfe_ops_4_8 = { > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c > index 80338efceb9e1..b1a07e846e25b 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe-480.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c > @@ -572,7 +572,6 @@ static const struct camss_video_ops vfe_video_ops_480 = { > static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) > { > vfe->video_ops = vfe_video_ops_480; > - vfe->line_num = MAX_VFE_OUTPUT_LINES; > } > > const struct vfe_hw_ops vfe_ops_480 = { > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c > index 526dd4ab343fe..b789b3b2e4cfd 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe.c > @@ -1305,6 +1305,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, > default: > return -EINVAL; > } > + > + if (!res->line_num) > + return -EINVAL; > + > + vfe->line_num = res->line_num; > vfe->ops->subdev_init(dev, vfe); > > /* Memory */ > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index c8a2571e664fe..ce0d86e45fe48 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -123,7 +123,8 @@ static const struct resources vfe_res_8x16[] = { > { 0 }, > { 0 } }, > .reg = { "vfe0" }, > - .interrupt = { "vfe0" } > + .interrupt = { "vfe0" }, > + .line_num = VFE_LINE_NUM_GEN1, > } > }; > > @@ -263,7 +264,8 @@ static const struct resources vfe_res_8x96[] = { > { 0 }, > { 0 } }, > .reg = { "vfe0" }, > - .interrupt = { "vfe0" } > + .interrupt = { "vfe0" }, > + .line_num = VFE_LINE_NUM_GEN1, > }, > > /* VFE1 */ > @@ -281,7 +283,8 @@ static const struct resources vfe_res_8x96[] = { > { 0 }, > { 0 } }, > .reg = { "vfe1" }, > - .interrupt = { "vfe1" } > + .interrupt = { "vfe1" }, > + .line_num = VFE_LINE_NUM_GEN1, > } > }; > > @@ -442,7 +445,8 @@ static const struct resources vfe_res_660[] = { > { 0 }, > { 0 } }, > .reg = { "vfe0" }, > - .interrupt = { "vfe0" } > + .interrupt = { "vfe0" }, > + .line_num = VFE_LINE_NUM_GEN1, > }, > > /* VFE1 */ > @@ -463,7 +467,8 @@ static const struct resources vfe_res_660[] = { > { 0 }, > { 0 } }, > .reg = { "vfe1" }, > - .interrupt = { "vfe1" } > + .interrupt = { "vfe1" }, > + .line_num = VFE_LINE_NUM_GEN1, > } > }; > > @@ -621,7 +626,8 @@ static const struct resources vfe_res_845[] = { > { 19200000, 75000000, 384000000, 538666667 }, > { 384000000 } }, > .reg = { "vfe0" }, > - .interrupt = { "vfe0" } > + .interrupt = { "vfe0" }, > + .line_num = VFE_LINE_NUM_GEN2, > }, > > /* VFE1 */ > @@ -641,7 +647,8 @@ static const struct resources vfe_res_845[] = { > { 19200000, 75000000, 384000000, 538666667 }, > { 384000000 } }, > .reg = { "vfe1" }, > - .interrupt = { "vfe1" } > + .interrupt = { "vfe1" }, > + .line_num = VFE_LINE_NUM_GEN2, > }, > > /* VFE-lite */ > @@ -660,7 +667,8 @@ static const struct resources vfe_res_845[] = { > { 19200000, 75000000, 384000000, 538666667 }, > { 384000000 } }, > .reg = { "vfe_lite" }, > - .interrupt = { "vfe_lite" } > + .interrupt = { "vfe_lite" }, > + .line_num = VFE_LINE_NUM_GEN2, > } > }; > > @@ -787,7 +795,8 @@ static const struct resources vfe_res_8250[] = { > { 0 }, > { 0 } }, > .reg = { "vfe0" }, > - .interrupt = { "vfe0" } > + .interrupt = { "vfe0" }, > + .line_num = 4, > }, > /* VFE1 */ > { > @@ -805,7 +814,8 @@ static const struct resources vfe_res_8250[] = { > { 0 }, > { 0 } }, > .reg = { "vfe1" }, > - .interrupt = { "vfe1" } > + .interrupt = { "vfe1" }, > + .line_num = 4, > }, > /* VFE2 (lite) */ > { > @@ -822,7 +832,8 @@ static const struct resources vfe_res_8250[] = { > { 400000000, 480000000 }, > { 0 } }, > .reg = { "vfe_lite0" }, > - .interrupt = { "vfe_lite0" } > + .interrupt = { "vfe_lite0" }, > + .line_num = 4, > }, > /* VFE3 (lite) */ > { > @@ -839,7 +850,8 @@ static const struct resources vfe_res_8250[] = { > { 400000000, 480000000 }, > { 0 } }, > .reg = { "vfe_lite1" }, > - .interrupt = { "vfe_lite1" } > + .interrupt = { "vfe_lite1" }, > + .line_num = 4, > }, > }; > > diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h > index dd8c58d349685..101ce6e527931 100644 > --- a/drivers/media/platform/qcom/camss/camss.h > +++ b/drivers/media/platform/qcom/camss/camss.h > @@ -48,6 +48,7 @@ struct resources { > u32 clock_rate[CAMSS_RES_MAX][CAMSS_RES_MAX]; > char *reg[CAMSS_RES_MAX]; > char *interrupt[CAMSS_RES_MAX]; > + u8 line_num; > }; > > struct icc_bw_tbl {
On 28/08/2023 19:36, Laurent Pinchart wrote:
> Should the variable then be renamed to num_rdi or similar ?
Yes but, the use of "line num" is pretty extensive. I plan a series to
normalise the namespace a bit later on. I'm just trying to scope limit
the extent of the changes in one blob.
- rdi_num
- size of struct resource strings
being two that are "high up" on my TODOs.
---
bod
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c index 9905bb06b3823..8aa921400ded0 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c @@ -756,8 +756,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) { vfe->isr_ops = vfe_isr_ops_170; vfe->video_ops = vfe_video_ops_170; - - vfe->line_num = VFE_LINE_NUM_GEN2; } const struct vfe_hw_ops vfe_ops_170 = { diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c index bc309f326f519..2911e4126e7ad 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c @@ -992,8 +992,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) vfe->isr_ops = vfe_isr_ops_gen1; vfe->ops_gen1 = &vfe_ops_gen1_4_1; vfe->video_ops = vfe_video_ops_gen1; - - vfe->line_num = VFE_LINE_NUM_GEN1; } const struct vfe_hw_ops vfe_ops_4_1 = { diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c index 8acd76c9746ba..b65ed0fef595e 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c @@ -1188,8 +1188,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) vfe->isr_ops = vfe_isr_ops_gen1; vfe->ops_gen1 = &vfe_ops_gen1_4_7; vfe->video_ops = vfe_video_ops_gen1; - - vfe->line_num = VFE_LINE_NUM_GEN1; } const struct vfe_hw_ops vfe_ops_4_7 = { diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c index 3a0167ecf873a..7b3805177f037 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c @@ -1173,8 +1173,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) vfe->isr_ops = vfe_isr_ops_gen1; vfe->ops_gen1 = &vfe_ops_gen1_4_8; vfe->video_ops = vfe_video_ops_gen1; - - vfe->line_num = VFE_LINE_NUM_GEN1; } const struct vfe_hw_ops vfe_ops_4_8 = { diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c index 80338efceb9e1..b1a07e846e25b 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe-480.c +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c @@ -572,7 +572,6 @@ static const struct camss_video_ops vfe_video_ops_480 = { static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe) { vfe->video_ops = vfe_video_ops_480; - vfe->line_num = MAX_VFE_OUTPUT_LINES; } const struct vfe_hw_ops vfe_ops_480 = { diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c index 526dd4ab343fe..b789b3b2e4cfd 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe.c +++ b/drivers/media/platform/qcom/camss/camss-vfe.c @@ -1305,6 +1305,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, default: return -EINVAL; } + + if (!res->line_num) + return -EINVAL; + + vfe->line_num = res->line_num; vfe->ops->subdev_init(dev, vfe); /* Memory */ diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c index c8a2571e664fe..ce0d86e45fe48 100644 --- a/drivers/media/platform/qcom/camss/camss.c +++ b/drivers/media/platform/qcom/camss/camss.c @@ -123,7 +123,8 @@ static const struct resources vfe_res_8x16[] = { { 0 }, { 0 } }, .reg = { "vfe0" }, - .interrupt = { "vfe0" } + .interrupt = { "vfe0" }, + .line_num = VFE_LINE_NUM_GEN1, } }; @@ -263,7 +264,8 @@ static const struct resources vfe_res_8x96[] = { { 0 }, { 0 } }, .reg = { "vfe0" }, - .interrupt = { "vfe0" } + .interrupt = { "vfe0" }, + .line_num = VFE_LINE_NUM_GEN1, }, /* VFE1 */ @@ -281,7 +283,8 @@ static const struct resources vfe_res_8x96[] = { { 0 }, { 0 } }, .reg = { "vfe1" }, - .interrupt = { "vfe1" } + .interrupt = { "vfe1" }, + .line_num = VFE_LINE_NUM_GEN1, } }; @@ -442,7 +445,8 @@ static const struct resources vfe_res_660[] = { { 0 }, { 0 } }, .reg = { "vfe0" }, - .interrupt = { "vfe0" } + .interrupt = { "vfe0" }, + .line_num = VFE_LINE_NUM_GEN1, }, /* VFE1 */ @@ -463,7 +467,8 @@ static const struct resources vfe_res_660[] = { { 0 }, { 0 } }, .reg = { "vfe1" }, - .interrupt = { "vfe1" } + .interrupt = { "vfe1" }, + .line_num = VFE_LINE_NUM_GEN1, } }; @@ -621,7 +626,8 @@ static const struct resources vfe_res_845[] = { { 19200000, 75000000, 384000000, 538666667 }, { 384000000 } }, .reg = { "vfe0" }, - .interrupt = { "vfe0" } + .interrupt = { "vfe0" }, + .line_num = VFE_LINE_NUM_GEN2, }, /* VFE1 */ @@ -641,7 +647,8 @@ static const struct resources vfe_res_845[] = { { 19200000, 75000000, 384000000, 538666667 }, { 384000000 } }, .reg = { "vfe1" }, - .interrupt = { "vfe1" } + .interrupt = { "vfe1" }, + .line_num = VFE_LINE_NUM_GEN2, }, /* VFE-lite */ @@ -660,7 +667,8 @@ static const struct resources vfe_res_845[] = { { 19200000, 75000000, 384000000, 538666667 }, { 384000000 } }, .reg = { "vfe_lite" }, - .interrupt = { "vfe_lite" } + .interrupt = { "vfe_lite" }, + .line_num = VFE_LINE_NUM_GEN2, } }; @@ -787,7 +795,8 @@ static const struct resources vfe_res_8250[] = { { 0 }, { 0 } }, .reg = { "vfe0" }, - .interrupt = { "vfe0" } + .interrupt = { "vfe0" }, + .line_num = 4, }, /* VFE1 */ { @@ -805,7 +814,8 @@ static const struct resources vfe_res_8250[] = { { 0 }, { 0 } }, .reg = { "vfe1" }, - .interrupt = { "vfe1" } + .interrupt = { "vfe1" }, + .line_num = 4, }, /* VFE2 (lite) */ { @@ -822,7 +832,8 @@ static const struct resources vfe_res_8250[] = { { 400000000, 480000000 }, { 0 } }, .reg = { "vfe_lite0" }, - .interrupt = { "vfe_lite0" } + .interrupt = { "vfe_lite0" }, + .line_num = 4, }, /* VFE3 (lite) */ { @@ -839,7 +850,8 @@ static const struct resources vfe_res_8250[] = { { 400000000, 480000000 }, { 0 } }, .reg = { "vfe_lite1" }, - .interrupt = { "vfe_lite1" } + .interrupt = { "vfe_lite1" }, + .line_num = 4, }, }; diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h index dd8c58d349685..101ce6e527931 100644 --- a/drivers/media/platform/qcom/camss/camss.h +++ b/drivers/media/platform/qcom/camss/camss.h @@ -48,6 +48,7 @@ struct resources { u32 clock_rate[CAMSS_RES_MAX][CAMSS_RES_MAX]; char *reg[CAMSS_RES_MAX]; char *interrupt[CAMSS_RES_MAX]; + u8 line_num; }; struct icc_bw_tbl {
line_num indicates the number of RDI - raw data interface channels which are associated with a given IFE/VFE - image/video front end. On several SoCs the RDI number is not static for each VFE - for example on sm8250 VFE Lite has four RDIs where regular VFE has three. Assigning line_num statically in the subdev_init() phase initialises each VFE to the lower number, meaning in practical terms that we are lobbing off one RDI on some VFEs. Interrupt handling uses static for (i = RDI0; i < RDI2; i++) {} in some of our VFE blocks but this can't work for situations where we have a mixture of VFE @ 3 RDI and VFE-lite @ 4 RDI blocks. First step to remediate is to pass line_num from a compat string controlled data-structure and do so on a per-VFE basis. Later patches will assign the correct number of RDI blocks per VFE. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- .../media/platform/qcom/camss/camss-vfe-170.c | 2 -- .../media/platform/qcom/camss/camss-vfe-4-1.c | 2 -- .../media/platform/qcom/camss/camss-vfe-4-7.c | 2 -- .../media/platform/qcom/camss/camss-vfe-4-8.c | 2 -- .../media/platform/qcom/camss/camss-vfe-480.c | 1 - drivers/media/platform/qcom/camss/camss-vfe.c | 5 +++ drivers/media/platform/qcom/camss/camss.c | 36 ++++++++++++------- drivers/media/platform/qcom/camss/camss.h | 1 + 8 files changed, 30 insertions(+), 21 deletions(-)