| Message ID | 20230823104444.1954663-1-bryan.odonoghue@linaro.org |
|---|---|
| Headers | show |
| Series | media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand |
On 23.08.2023 12:44, Bryan O'Donoghue wrote: > From sdm845 onwards we need to ensure the VFE is powered on prior to > switching on the CSID. And what's the symptom if we fail to ensure this? How can someone adding support for another platform tell whether the match-list should be expanded? > > Alternatively we could model up the GDSCs and clocks the CSID needs > without the VFE but, there's a real question of the legitimacy of such a > use-case. > > For now drawing a line at sdm845 and switching on the associated VFEs is > a perfectly valid thing to do. > > Rather than continually extend out this clause for at least two new SoCs > with this same model - making the vfe_get/vfe_put path start to look > like spaghetti we can encoded the dependency in a helper function. > > Use csid_depends_vfe() for this purpose. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../media/platform/qcom/camss/camss-csid.c | 20 +++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c > index 08991b070bd61..fd04ed112b564 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.c > +++ b/drivers/media/platform/qcom/camss/camss-csid.c > @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid) > return 0; > } > > +static bool csid_depends_vfe(u32 version) toggle_vfe_before_csid? > +{ > + bool ret = false; > + > + switch (version) { > + case CAMSS_845: > + case CAMSS_8250: > + ret = true; > + break; > + default: > + break; > + } > + > + return ret; I'm not sure if it would be okay with like C conventions and stuff, but this can be made shorter by returning from within the switch statement Konrad
On 26/08/2023 11:02, Konrad Dybcio wrote: > On 23.08.2023 12:44, Bryan O'Donoghue wrote: >> From sdm845 onwards we need to ensure the VFE is powered on prior to >> switching on the CSID. > And what's the symptom if we fail to ensure this? How can someone > adding support for another platform tell whether the match-list > should be expanded? That person has to understand the dependency. The first version of this patch >= SDM845 would mitigate needing to know to expand the list. Rather than revisit that discussion, I will amend the commit log. > >> >> Alternatively we could model up the GDSCs and clocks the CSID needs >> without the VFE but, there's a real question of the legitimacy of such a >> use-case. >> >> For now drawing a line at sdm845 and switching on the associated VFEs is >> a perfectly valid thing to do. >> >> Rather than continually extend out this clause for at least two new SoCs >> with this same model - making the vfe_get/vfe_put path start to look >> like spaghetti we can encoded the dependency in a helper function. >> >> Use csid_depends_vfe() for this purpose. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> .../media/platform/qcom/camss/camss-csid.c | 20 +++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c >> index 08991b070bd61..fd04ed112b564 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csid.c >> +++ b/drivers/media/platform/qcom/camss/camss-csid.c >> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid) >> return 0; >> } >> >> +static bool csid_depends_vfe(u32 version) > toggle_vfe_before_csid? If that's clearer np. >> +{ >> + bool ret = false; >> + >> + switch (version) { >> + case CAMSS_845: >> + case CAMSS_8250: >> + ret = true; >> + break; >> + default: >> + break; >> + } >> + >> + return ret; > I'm not sure if it would be okay with like C conventions and > stuff, but this can be made shorter by returning from within > the switch statement Yes but you still need the explicit return at the end of the function or from memory at least some of the compiler/static analysis or checkpatch stuff - I forget which - will complain. --- bod
On 26.08.2023 14:01, Bryan O'Donoghue wrote: > On 26/08/2023 11:02, Konrad Dybcio wrote: >> On 23.08.2023 12:44, Bryan O'Donoghue wrote: >>> From sdm845 onwards we need to ensure the VFE is powered on prior to >>> switching on the CSID. >> And what's the symptom if we fail to ensure this? How can someone >> adding support for another platform tell whether the match-list >> should be expanded? > > That person has to understand the dependency. If we need this workaround, there surely must be something that doesn't work without it, a measurable symptom. What is it? Konrad
On 26/08/2023 13:04, Konrad Dybcio wrote: >>>> From sdm845 onwards we need to ensure the VFE is powered on prior to >>>> switching on the CSID. >>> And what's the symptom if we fail to ensure this? How can someone >>> adding support for another platform tell whether the match-list >>> should be expanded? >> That person has to understand the dependency. > If we need this workaround, there surely must be something that doesn't > work without it, a measurable symptom. What is it? The CSID lives inside of the VFE therefore the VFE must be power prior to the CSID. The symptom will be .. the CSID doesn't come out of reset. --- bod
On 26.08.2023 14:12, Bryan O'Donoghue wrote: > On 26/08/2023 13:04, Konrad Dybcio wrote: >>>>> From sdm845 onwards we need to ensure the VFE is powered on prior to >>>>> switching on the CSID. >>>> And what's the symptom if we fail to ensure this? How can someone >>>> adding support for another platform tell whether the match-list >>>> should be expanded? >>> That person has to understand the dependency. >> If we need this workaround, there surely must be something that doesn't >> work without it, a measurable symptom. What is it? > > The CSID lives inside of the VFE therefore the VFE must be power prior to the CSID. > > The symptom will be .. the CSID doesn't come out of reset. Good, that's what I needed to know. Now we can rename that function to something like camss_csid_inside_vfe() to make it more meaningful Konrad
On 26/08/2023 13:13, Konrad Dybcio wrote: > On 26.08.2023 14:12, Bryan O'Donoghue wrote: >> On 26/08/2023 13:04, Konrad Dybcio wrote: >>>>>> From sdm845 onwards we need to ensure the VFE is powered on prior to >>>>>> switching on the CSID. >>>>> And what's the symptom if we fail to ensure this? How can someone >>>>> adding support for another platform tell whether the match-list >>>>> should be expanded? >>>> That person has to understand the dependency. >>> If we need this workaround, there surely must be something that doesn't >>> work without it, a measurable symptom. What is it? >> >> The CSID lives inside of the VFE therefore the VFE must be power prior to the CSID. >> >> The symptom will be .. the CSID doesn't come out of reset. > Good, that's what I needed to know. > > Now we can rename that function to something like camss_csid_inside_vfe() > to make it more meaningful If you feel there is a meaningful distinction between "csid_depends_vfe()" and "camss_csid_inside_vfe()" I'm happy to humour you. --- bod
On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote: > On 28/08/2023 19:47, Laurent Pinchart wrote: > > Hi Bryan, > > > > Thank you for the patch. > > > > On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote: > >> From sdm845 onwards we need to ensure the VFE is powered on prior to > >> switching on the CSID. > > > > Based on the discussions with Konrad in the mail thread, I would record > > here the reason for this requirement. > > > > What happens if you unconditionally power up the VFE prior to switching > > the CSID on ? Will this break something on platforms older than SDM845 ? > > vfe->power_count will bump and you will be fine. Then maybe that would be a simpler solution than having a device-specific power sequence ?
On 28/08/2023 20:40, Laurent Pinchart wrote: > On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote: >> On 28/08/2023 19:47, Laurent Pinchart wrote: >>> Hi Bryan, >>> >>> Thank you for the patch. >>> >>> On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote: >>>> From sdm845 onwards we need to ensure the VFE is powered on prior to >>>> switching on the CSID. >>> >>> Based on the discussions with Konrad in the mail thread, I would record >>> here the reason for this requirement. >>> >>> What happens if you unconditionally power up the VFE prior to switching >>> the CSID on ? Will this break something on platforms older than SDM845 ? >> >> vfe->power_count will bump and you will be fine. > > Then maybe that would be a simpler solution than having a > device-specific power sequence ? > So, this works. I'll send out a patch based on this suggestion. --- bod
V3: - Adds RB/AB as indicated - Konrad - Replaces >= SDM845 with helper function per discussion - bod/Konrad - Leaves out constraining VFE clock names sizes. A full pass for resource strings will happen later. - bod - Clarifies commit log resulting in updated patch title also "Add support for setting CSIPHY clock name csiphyX" -> "Fix support for setting CSIPHY clock name csiphyX" - Adds patch to remove dead integer return type in vfe_disable() - Adds patch to comment CSID dt_id meanining which I personally find non-obvious right now - bod Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commits/09e7805a733b488c5dc19b301eb3b77cb0fad3d6 V2: - Replaces &camss->res with pointer to res - Konrad - Squashes patch for NULL removal - Konrad - Left suggestion on ICC initialisation points alone, doesn't seem to fit Konrad/bod Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-08-07-db410c-rb3-camss-dts-v3+maintenance-bugfixes-v2 V1: - I forgot to include patch # 14 in V0 of this series. This patch leverages previous changes to unwind the fixed polling of RDI[0..2] allowing driver data to articulate on a per-VFE basis how many RDIs to poll. Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-08-07-db410c-rb3-camss-dts-v3+maintenance-bugfixes-v1 V0: This second series of bugfixes stacks ontop of the Fixes series sent earlier. Link: https://lore.kernel.org/linux-arm-msm/20230814141007.3721197-1-bryan.odonoghue@linaro.org/T/#t Rather than send both series as one giant series, I opted to send a pure Fixes series above, with this second series a non-backport series i.e. no Fixes tags in this series. The existing CAMSS code relies on some hard-coded parameters buried inside of the driver, instead of passed via compat .data as arguably ought to be the case. This brittle model is an extending morass of spaghetti code. More than that in CAMSS Video Front Ends (VFEs) and the number of Raw Data Interfaces (RDIs) per VFE can vary from SoC to SoC. Indeed sm8250 has VFE and VFE Lite blocks which have a different number of RDIs per block. The use of defines as opposed to per-compat parameters inside of ISRs leads to either under-polling or over-polling the number of RDIs. On top of all of that we have some hard-coded statements for clock names which breaks easily. We can solve the under/over polling loop problem by transitioning loop controls from macros to parameters passed via probe(). Similarly and unsurprisingly we can also solve the hard-coded clock problem by adding some string processing routines that take passed arguments. There is still some additional maintenance work to be done in this driver but before adding one more SoC the code needs to be made more extensible and less brittle. Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commits/dc346c7f46c0680bcfb84fded6db97497fffe49a Bryan O'Donoghue (15): media: qcom: camss: Amalgamate struct resource with struct resource_ispif media: qcom: camss: Start to move to module compat matched resources media: qcom: camss: Pass icc bandwidth table as a platform parameter media: qcom: camss: Pass remainder of variables as resources media: qcom: camss: Pass line_num from compat resources media: qcom: camss: Assign the correct number of RDIs per VFE media: qcom: camss: Capture VFE CSID dependency in a helper function media: qcom: camss: Untangle if/else spaghetti in camss media: qcom: camss: Improve error printout on icc_get fail media: qcom: camss: Allow clocks vfeN vfe_liteN or vfe_lite media: qcom: camss: Functionally decompose CSIPHY clock lookups media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX media: qcom: camss: Support RDI3 for VFE 17x media: qcom: camss: Convert vfe_disable() from int to void media: qcom: camss: Comment CSID dt_id field .../platform/qcom/camss/camss-csid-gen2.c | 5 + .../media/platform/qcom/camss/camss-csid.c | 40 ++- .../qcom/camss/camss-csiphy-3ph-1-0.c | 8 +- .../media/platform/qcom/camss/camss-csiphy.c | 67 ++-- .../media/platform/qcom/camss/camss-ispif.c | 32 +- .../media/platform/qcom/camss/camss-ispif.h | 4 +- .../media/platform/qcom/camss/camss-vfe-170.c | 22 +- .../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 | 10 +- .../platform/qcom/camss/camss-vfe-gen1.c | 5 +- .../platform/qcom/camss/camss-vfe-gen1.h | 3 +- drivers/media/platform/qcom/camss/camss-vfe.c | 83 +++-- drivers/media/platform/qcom/camss/camss-vfe.h | 2 +- .../media/platform/qcom/camss/camss-video.c | 16 +- drivers/media/platform/qcom/camss/camss.c | 293 +++++++++--------- drivers/media/platform/qcom/camss/camss.h | 31 +- 18 files changed, 352 insertions(+), 275 deletions(-)