[PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning_configure

[PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning_configure

[Allen Ballway]

Prevents ov8865_mode_binning_configure from overwriting the hflip
register values. Allows programs to configure the hflip.

Signed-off-by: Allen Ballway <ballway@chromium.org>
---
 drivers/media/i2c/ov8865.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..40a852d31f13aff960acfd09b378d71525e19332 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -1746,7 +1746,13 @@ static int ov8865_mode_binning_configure(struct ov8865_sensor *sensor,
 	if (ret)
 		return ret;
 
-	value = OV8865_FORMAT2_HSYNC_EN;
+	ret = ov8865_read(sensor, OV8865_FORMAT2_REG, &value);
+	if (ret)
+		return ret;
+
+	value &= OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
+		  OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
+	value |= OV8865_FORMAT2_HSYNC_EN;
 
 	if (mode->binning_x)
 		value |= OV8865_FORMAT2_FST_HBIN_EN;

---
base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf
change-id: 20250717-su-94b187fa3d1e

Best regards,
-- 
Allen Ballway <ballway@chromium.org>

Re: [PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning_configure

[Hans de Goede]

Hi,

On 17-Jul-25 11:07 PM, Allen Ballway wrote:
> Prevents ov8865_mode_binning_configure from overwriting the hflip
> register values. Allows programs to configure the hflip.
> 
> Signed-off-by: Allen Ballway <ballway@chromium.org>

Thank you for your patch.

> ---
>  drivers/media/i2c/ov8865.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..40a852d31f13aff960acfd09b378d71525e19332 100644
> --- a/drivers/media/i2c/ov8865.c
> +++ b/drivers/media/i2c/ov8865.c
> @@ -1746,7 +1746,13 @@ static int ov8865_mode_binning_configure(struct ov8865_sensor *sensor,
>  	if (ret)
>  		return ret;
>  
> -	value = OV8865_FORMAT2_HSYNC_EN;
> +	ret = ov8865_read(sensor, OV8865_FORMAT2_REG, &value);
> +	if (ret)
> +		return ret;
> +
> +	value &= OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
> +		  OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
> +	value |= OV8865_FORMAT2_HSYNC_EN;
>  
>  	if (mode->binning_x)
>  		value |= OV8865_FORMAT2_FST_HBIN_EN;

this change should not be necessary. Lets assume we start
with the sensor runtime-suspended, then ov8865_resume()
will call:

ov8865_sensor_power(true)
ov8865_sensor_init()
  ov8865_state_configure()
    ov8865_mode_configure()
      ov8865_mode_binning_configure()
__v4l2_ctrl_handler_setup()

Where the __v4l2_ctrl_handler_setup() call will apply
all control settings including hflip.

So unless you manage to hit a code-path where somehow
ov8865_state_configure() gets called without calling
__v4l2_ctrl_handler_setup() afterwards then this should
not be necessary.

Note the whole runtime-pm / calling of ov8865_sensor_init() /
ov8865_state_configure() code in this driver is somewhat
unusual. So it could be there is a bug there.

But I don't believe that this patch is the correct fix.

Regards,

Hans





> 
> ---
> base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf
> change-id: 20250717-su-94b187fa3d1e
> 
> Best regards,

Re: [PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning_configure

[Allen Ballway]

Hello,

On Mon, Jul 21, 2025 at 4:51 AM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
>
> On 17-Jul-25 11:07 PM, Allen Ballway wrote:
> > Prevents ov8865_mode_binning_configure from overwriting the hflip
> > register values. Allows programs to configure the hflip.
> >
> > Signed-off-by: Allen Ballway <ballway@chromium.org>
>
> Thank you for your patch.
>
> > ---
> >  drivers/media/i2c/ov8865.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> > index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..40a852d31f13aff960acfd09b378d71525e19332 100644
> > --- a/drivers/media/i2c/ov8865.c
> > +++ b/drivers/media/i2c/ov8865.c
> > @@ -1746,7 +1746,13 @@ static int ov8865_mode_binning_configure(struct ov8865_sensor *sensor,
> >       if (ret)
> >               return ret;
> >
> > -     value = OV8865_FORMAT2_HSYNC_EN;
> > +     ret = ov8865_read(sensor, OV8865_FORMAT2_REG, &value);
> > +     if (ret)
> > +             return ret;
> > +
> > +     value &= OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
> > +               OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
> > +     value |= OV8865_FORMAT2_HSYNC_EN;
> >
> >       if (mode->binning_x)
> >               value |= OV8865_FORMAT2_FST_HBIN_EN;
>
> this change should not be necessary. Lets assume we start
> with the sensor runtime-suspended, then ov8865_resume()
> will call:
>
> ov8865_sensor_power(true)
> ov8865_sensor_init()
>   ov8865_state_configure()
>     ov8865_mode_configure()
>       ov8865_mode_binning_configure()
> __v4l2_ctrl_handler_setup()
>
> Where the __v4l2_ctrl_handler_setup() call will apply
> all control settings including hflip.
>
> So unless you manage to hit a code-path where somehow
> ov8865_state_configure() gets called without calling
> __v4l2_ctrl_handler_setup() afterwards then this should
> not be necessary.

ov8865_state_configure() is also being called from ov8865_set_fmt(),
and makes no calls to __v4l2_ctrl_handler_setup(). I'm not sure if
calling __v4l2_ctrl_handler_setup() here as well is the right fix, but
the driver ov8865 seems to be based upon, ov5648, seems to avoid
this issue by preserving the flip values when setting the binning
register values in ov5648_mode_configure by using
ov5648_update_bits() rather than ov5648_write(). I believe that we
just need to preserve the register values unrelated to binning inside
ov8865_mode_binning_configure, possibly by just using
ov8865_update_bits() instead of ov8865_write().

>
> Note the whole runtime-pm / calling of ov8865_sensor_init() /
> ov8865_state_configure() code in this driver is somewhat
> unusual. So it could be there is a bug there.
>
> But I don't believe that this patch is the correct fix.
>
> Regards,
>
> Hans

Thanks,
Allen
>
>
>
>
>
> >
> > ---
> > base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf
> > change-id: 20250717-su-94b187fa3d1e
> >
> > Best regards,
>

Re: [PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning_configure

[Hans de Goede]

Hi,

On 21-Jul-25 7:46 PM, Allen Ballway wrote:
> Hello,
> 
> On Mon, Jul 21, 2025 at 4:51 AM Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi,
>>
>> On 17-Jul-25 11:07 PM, Allen Ballway wrote:
>>> Prevents ov8865_mode_binning_configure from overwriting the hflip
>>> register values. Allows programs to configure the hflip.
>>>
>>> Signed-off-by: Allen Ballway <ballway@chromium.org>
>>
>> Thank you for your patch.
>>
>>> ---
>>>  drivers/media/i2c/ov8865.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>>> index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..40a852d31f13aff960acfd09b378d71525e19332 100644
>>> --- a/drivers/media/i2c/ov8865.c
>>> +++ b/drivers/media/i2c/ov8865.c
>>> @@ -1746,7 +1746,13 @@ static int ov8865_mode_binning_configure(struct ov8865_sensor *sensor,
>>>       if (ret)
>>>               return ret;
>>>
>>> -     value = OV8865_FORMAT2_HSYNC_EN;
>>> +     ret = ov8865_read(sensor, OV8865_FORMAT2_REG, &value);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     value &= OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
>>> +               OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
>>> +     value |= OV8865_FORMAT2_HSYNC_EN;
>>>
>>>       if (mode->binning_x)
>>>               value |= OV8865_FORMAT2_FST_HBIN_EN;
>>
>> this change should not be necessary. Lets assume we start
>> with the sensor runtime-suspended, then ov8865_resume()
>> will call:
>>
>> ov8865_sensor_power(true)
>> ov8865_sensor_init()
>>   ov8865_state_configure()
>>     ov8865_mode_configure()
>>       ov8865_mode_binning_configure()
>> __v4l2_ctrl_handler_setup()
>>
>> Where the __v4l2_ctrl_handler_setup() call will apply
>> all control settings including hflip.
>>
>> So unless you manage to hit a code-path where somehow
>> ov8865_state_configure() gets called without calling
>> __v4l2_ctrl_handler_setup() afterwards then this should
>> not be necessary.
> 
> ov8865_state_configure() is also being called from ov8865_set_fmt(),
> and makes no calls to __v4l2_ctrl_handler_setup(). I'm not sure if
> calling __v4l2_ctrl_handler_setup() here as well is the right fix, but
> the driver ov8865 seems to be based upon, ov5648, seems to avoid
> this issue by preserving the flip values when setting the binning
> register values in ov5648_mode_configure by using
> ov5648_update_bits() rather than ov5648_write(). I believe that we
> just need to preserve the register values unrelated to binning inside
> ov8865_mode_binning_configure, possibly by just using
> ov8865_update_bits() instead of ov8865_write().

But you cannot call ov8865_set_fmt() while streaming, since
it has :

        if (sensor->state.streaming) {
                ret = -EBUSY;
                goto complete;
        }

in there.

And when not streaming the sensor is off. So inside ov8865_state_configure()
the ov8865_mode_configure() and thus ov8865_mode_binning_configure()
will be skipped since that is protected by if (!pm_runtime_suspended())
as mentioned before this is all a bit messy in this driver and it would
be good to untangle this a bit, I think the ov8865_mode_configure()
should be moved out of ov8865_state_configure() and instead done
separately on power-up.

Regards,

Hans

Re: [PATCH] media: ov8865: Preserve hflip in ov8865_mode_binning_configure

[Allen Ballway]

Hello,

On Tue, Jul 22, 2025 at 2:19 AM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
>
> On 21-Jul-25 7:46 PM, Allen Ballway wrote:
> > Hello,
> >
> > On Mon, Jul 21, 2025 at 4:51 AM Hans de Goede <hansg@kernel.org> wrote:
> >>
> >> Hi,
> >>
> >> On 17-Jul-25 11:07 PM, Allen Ballway wrote:
> >>> Prevents ov8865_mode_binning_configure from overwriting the hflip
> >>> register values. Allows programs to configure the hflip.
> >>>
> >>> Signed-off-by: Allen Ballway <ballway@chromium.org>
> >>
> >> Thank you for your patch.
> >>
> >>> ---
> >>>  drivers/media/i2c/ov8865.c | 8 +++++++-
> >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> >>> index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..40a852d31f13aff960acfd09b378d71525e19332 100644
> >>> --- a/drivers/media/i2c/ov8865.c
> >>> +++ b/drivers/media/i2c/ov8865.c
> >>> @@ -1746,7 +1746,13 @@ static int ov8865_mode_binning_configure(struct ov8865_sensor *sensor,
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>> -     value = OV8865_FORMAT2_HSYNC_EN;
> >>> +     ret = ov8865_read(sensor, OV8865_FORMAT2_REG, &value);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     value &= OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
> >>> +               OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
> >>> +     value |= OV8865_FORMAT2_HSYNC_EN;
> >>>
> >>>       if (mode->binning_x)
> >>>               value |= OV8865_FORMAT2_FST_HBIN_EN;
> >>
> >> this change should not be necessary. Lets assume we start
> >> with the sensor runtime-suspended, then ov8865_resume()
> >> will call:
> >>
> >> ov8865_sensor_power(true)
> >> ov8865_sensor_init()
> >>   ov8865_state_configure()
> >>     ov8865_mode_configure()
> >>       ov8865_mode_binning_configure()
> >> __v4l2_ctrl_handler_setup()
> >>
> >> Where the __v4l2_ctrl_handler_setup() call will apply
> >> all control settings including hflip.
> >>
> >> So unless you manage to hit a code-path where somehow
> >> ov8865_state_configure() gets called without calling
> >> __v4l2_ctrl_handler_setup() afterwards then this should
> >> not be necessary.
> >
> > ov8865_state_configure() is also being called from ov8865_set_fmt(),
> > and makes no calls to __v4l2_ctrl_handler_setup(). I'm not sure if
> > calling __v4l2_ctrl_handler_setup() here as well is the right fix, but
> > the driver ov8865 seems to be based upon, ov5648, seems to avoid
> > this issue by preserving the flip values when setting the binning
> > register values in ov5648_mode_configure by using
> > ov5648_update_bits() rather than ov5648_write(). I believe that we
> > just need to preserve the register values unrelated to binning inside
> > ov8865_mode_binning_configure, possibly by just using
> > ov8865_update_bits() instead of ov8865_write().
>
> But you cannot call ov8865_set_fmt() while streaming, since
> it has :
>
>         if (sensor->state.streaming) {
>                 ret = -EBUSY;
>                 goto complete;
>         }
>
> in there.
>
> And when not streaming the sensor is off. So inside ov8865_state_configure()
> the ov8865_mode_configure() and thus ov8865_mode_binning_configure()
> will be skipped since that is protected by if (!pm_runtime_suspended())
> as mentioned before this is all a bit messy in this driver and it would
> be good to untangle this a bit, I think the ov8865_mode_configure()
> should be moved out of ov8865_state_configure() and instead done
> separately on power-up.

I see now. ov8865_set_fmt() will be called multiple times after the sensor is
resumed but before it is streaming, calling through to ov8865_mode_configure()
and then stomping the register values. Moving the ov8865_mode_configure()
call out of ov8865_state_configure() and into ov8865_sensor_init() prevents
this and allows the flip to be configured correctly.

Thanks for the feedback, I'll send out a new patch for the above change soon.

Allen
>
> Regards,
>
> Hans
>