Re: [PATCH v3 08/11] media: qcom: camss: Switch on frame_sync_en logic for csid-gen3.

[Gjorgji Rosikopulos (Consultant)]

Hi Loic, Bryan,

On 11/9/2026 11:56 AM, Loic Poulain wrote:
> On Sat, Nov 7, 2026 at 2:02 AM <bryan.odonoghue@xxxxxxxxxx> wrote:
>>
>> From: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>>
>> Switch frame_sync_en to true to track and consume SOF/EOF data.
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx> # sm8650-hdk
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>> Signed-off-by: Bryan O'Donoghue <bod@xxxxxxxxxx>
> 
> I would have preferred this to be defined directly in the
> corresponding CSID implementations rather than in the resource
> descriptors, so that the component implementing frame synchronization
> is also the one advertising support for it. In other words, something
> along the lines of a csid_hw_ops.features flag or a dedicated
> csid_hw_ops.has_frame_sync callback/field. That said, this is more of
> a design preference than a blocking concern for me, so:

I double that. It make more sense to introduce capability field, only drawback is
that as of today once the feature is validated for the SoC (which may contain
same csid or vfe version) is marked as supported which may not be the case
with caps, however the risk of breaking something is low.
Note that in multistream support we are introducing new field per SoC stream api
which also can be taken based on the caps. 

~Gjorgji

> 
> Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> 
>> ---
>>  drivers/media/platform/qcom/camss/camss.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 52bb19f5fbf3b..b7821019241d7 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -3120,6 +3120,7 @@ static const struct camss_subdev_resources csid_res_8550[] = {
>>                 .reg = { "csid0" },
>>                 .interrupt = { "csid0" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = false,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>> @@ -3135,6 +3136,7 @@ static const struct camss_subdev_resources csid_res_8550[] = {
>>                 .reg = { "csid1" },
>>                 .interrupt = { "csid1" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = false,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>> @@ -3150,6 +3152,7 @@ static const struct camss_subdev_resources csid_res_8550[] = {
>>                 .reg = { "csid2" },
>>                 .interrupt = { "csid2" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = false,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>> @@ -3165,6 +3168,7 @@ static const struct camss_subdev_resources csid_res_8550[] = {
>>                 .reg = { "csid_lite0" },
>>                 .interrupt = { "csid_lite0" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>> @@ -3180,6 +3184,7 @@ static const struct camss_subdev_resources csid_res_8550[] = {
>>                 .reg = { "csid_lite1" },
>>                 .interrupt = { "csid_lite1" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>> @@ -3433,6 +3438,7 @@ static const struct camss_subdev_resources csid_res_sm8650[] = {
>>                 .reg = { "csid0" },
>>                 .interrupt = { "csid0" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .formats = &csid_formats_gen2,
>> @@ -3447,6 +3453,7 @@ static const struct camss_subdev_resources csid_res_sm8650[] = {
>>                 .reg = { "csid1" },
>>                 .interrupt = { "csid1" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .formats = &csid_formats_gen2,
>> @@ -3461,6 +3468,7 @@ static const struct camss_subdev_resources csid_res_sm8650[] = {
>>                 .reg = { "csid2" },
>>                 .interrupt = { "csid2" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .formats = &csid_formats_gen2,
>> @@ -3476,6 +3484,7 @@ static const struct camss_subdev_resources csid_res_sm8650[] = {
>>                 .reg = { "csid_lite0" },
>>                 .interrupt = { "csid_lite0" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>> @@ -3492,6 +3501,7 @@ static const struct camss_subdev_resources csid_res_sm8650[] = {
>>                 .reg = { "csid_lite1" },
>>                 .interrupt = { "csid_lite1" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>                         .hw_ops = &csid_ops_gen3,
>> @@ -3790,6 +3800,7 @@ static const struct camss_subdev_resources csid_res_8775p[] = {
>>                 .reg = { "csid0" },
>>                 .interrupt = { "csid0" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = false,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>> @@ -3807,6 +3818,7 @@ static const struct camss_subdev_resources csid_res_8775p[] = {
>>                 .reg = { "csid1" },
>>                 .interrupt = { "csid1" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = false,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>> @@ -3825,6 +3837,7 @@ static const struct camss_subdev_resources csid_res_8775p[] = {
>>                 .reg = { "csid_lite0" },
>>                 .interrupt = { "csid_lite0" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>> @@ -3842,6 +3855,7 @@ static const struct camss_subdev_resources csid_res_8775p[] = {
>>                 .reg = { "csid_lite1" },
>>                 .interrupt = { "csid_lite1" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>> @@ -3859,6 +3873,7 @@ static const struct camss_subdev_resources csid_res_8775p[] = {
>>                 .reg = { "csid_lite2" },
>>                 .interrupt = { "csid_lite2" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>> @@ -3876,6 +3891,7 @@ static const struct camss_subdev_resources csid_res_8775p[] = {
>>                 .reg = { "csid_lite3" },
>>                 .interrupt = { "csid_lite3" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>> @@ -3893,6 +3909,7 @@ static const struct camss_subdev_resources csid_res_8775p[] = {
>>                 .reg = { "csid_lite4" },
>>                 .interrupt = { "csid_lite4" },
>>                 .csid = {
>> +                       .frame_sync_en = true,
>>                         .is_lite = true,
>>                         .hw_ops = &csid_ops_gen3,
>>                         .parent_dev_ops = &vfe_parent_dev_ops,
>>
>> --
>> 2.56.0
>>
>