Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Laurent Pinchart]

Hi Hans,

On Friday 11 Aug 2017 08:05:03 Hans Verkuil wrote:
> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> > In the past, only string controls were pointers. That changed when
> > compounded types got added, but the compat32 code was not updated.
> > 
> > We could just add those controls there, but maintaining it is flaw, as we
> > often forget about the compat code. So, instead, rely on the control type,
> > as this is always updated when new controls are added.
> > 
> > As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
> > move the ctrl_is_pointer() helper function to v4l2-ctrl.c.
> 
> This series doesn't really solve anything:
> 
> - it introduces a circular dependency between two modules
> - it doesn't handle driver-custom controls (the old code didn't either). For
> example vivid has custom pointer controls.
> - it replaces a list of control IDs with a list of type IDs, which also has
> to be kept up to date.
> 
> I thought this over and I have a better and much simpler idea. I'll post a
> patch for that.

Wouldn't it be time to replace the large switch/case with an array of control 
information ? Maybe that was your better idea already :-)

> > ---
> > 
> > Re-sending this patch series, as it was c/c to the linux-doc ML by
> > mistake.
> > 
> > Mauro Carvalho Chehab (3):
> >   media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
> >   media: v4l2-ctrls: prepare the function to be used by compat32 code
> >   media: compat32: reimplement ctrl_is_pointer()
> >  
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +-
> >  drivers/media/v4l2-core/v4l2-ctrls.c  | 49 --
> >  include/media/v4l2-ctrls.h| 28 ++-
> >  3 files changed, 67 insertions(+), 28 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Hans Verkuil]

On 11/08/17 11:00, Mauro Carvalho Chehab wrote:
> Em Fri, 11 Aug 2017 08:05:03 +0200
> Hans Verkuil  escreveu:
> 
>> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
>>> In the past, only string controls were pointers. That changed when 
>>> compounded
>>> types got added, but the compat32 code was not updated.
>>>
>>> We could just add those controls there, but maintaining it is flaw, as we
>>> often forget about the compat code. So, instead, rely on the control type,
>>> as this is always updated when new controls are added.
>>>
>>> As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
>>> move the ctrl_is_pointer() helper function to v4l2-ctrl.c.  
>>
>> This series doesn't really solve anything:
>>
>> - it introduces a circular dependency between two modules
> 
> What two modules? both v4l2-ctrl and compat32 belong to the *same* module.
> See the Makefile:
> 
> videodev-objs :=  v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
>   v4l2-async.o
> ifeq ($(CONFIG_COMPAT),y)
>   videodev-objs += v4l2-compat-ioctl32.o
> endif

My fault, I should have checked. The 'circular dependency' comment in the code
referred to the bad old days when v4l2_ctrl_fill was in v4l2-common.c and that
was a separate module. All that has long since changed.

> Both belong to videodev module. IMHO, the best is to move whatever
> control check logic it might need to v4l2-ctrls.
> 
>> - it doesn't handle driver-custom controls (the old code didn't either). For
>>   example vivid has custom pointer controls.
> 
> True.
> 
>> - it replaces a list of control IDs with a list of type IDs, which also has 
>> to
>>   be kept up to date.
> 
> True, but at least after the patch, the ancillary function is together
> with the code that handles the controls. Also, we don't introduce new
> types too often.
> 
>>
>> I thought this over and I have a better and much simpler idea. I'll post a
>> patch for that.
> 
> OK.

I have the patch ready, but I need to test it first with vivid.

Regards,

Hans

Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Mauro Carvalho Chehab]

Em Fri, 11 Aug 2017 08:05:03 +0200
Hans Verkuil  escreveu:

> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> > In the past, only string controls were pointers. That changed when 
> > compounded
> > types got added, but the compat32 code was not updated.
> > 
> > We could just add those controls there, but maintaining it is flaw, as we
> > often forget about the compat code. So, instead, rely on the control type,
> > as this is always updated when new controls are added.
> > 
> > As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
> > move the ctrl_is_pointer() helper function to v4l2-ctrl.c.  
> 
> This series doesn't really solve anything:
> 
> - it introduces a circular dependency between two modules

What two modules? both v4l2-ctrl and compat32 belong to the *same* module.
See the Makefile:

videodev-objs   :=  v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
v4l2-async.o
ifeq ($(CONFIG_COMPAT),y)
  videodev-objs += v4l2-compat-ioctl32.o
endif

Both belong to videodev module. IMHO, the best is to move whatever
control check logic it might need to v4l2-ctrls.

> - it doesn't handle driver-custom controls (the old code didn't either). For
>   example vivid has custom pointer controls.

True.

> - it replaces a list of control IDs with a list of type IDs, which also has to
>   be kept up to date.

True, but at least after the patch, the ancillary function is together
with the code that handles the controls. Also, we don't introduce new
types too often.

> 
> I thought this over and I have a better and much simpler idea. I'll post a
> patch for that.

OK.

> 
> Regards,
> 
>   Hans
> 
> > 
> > ---
> > 
> > Re-sending this patch series, as it was c/c to the linux-doc ML by mistake.
> > 
> > Mauro Carvalho Chehab (3):
> >   media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
> >   media: v4l2-ctrls: prepare the function to be used by compat32 code
> >   media: compat32: reimplement ctrl_is_pointer()
> > 
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +-
> >  drivers/media/v4l2-core/v4l2-ctrls.c  | 49 
> > +--
> >  include/media/v4l2-ctrls.h| 28 ++-
> >  3 files changed, 67 insertions(+), 28 deletions(-)
> >   
> 



Thanks,
Mauro

Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Hans Verkuil]

On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> In the past, only string controls were pointers. That changed when compounded
> types got added, but the compat32 code was not updated.
> 
> We could just add those controls there, but maintaining it is flaw, as we
> often forget about the compat code. So, instead, rely on the control type,
> as this is always updated when new controls are added.
> 
> As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
> move the ctrl_is_pointer() helper function to v4l2-ctrl.c.

This series doesn't really solve anything:

- it introduces a circular dependency between two modules
- it doesn't handle driver-custom controls (the old code didn't either). For
  example vivid has custom pointer controls.
- it replaces a list of control IDs with a list of type IDs, which also has to
  be kept up to date.

I thought this over and I have a better and much simpler idea. I'll post a
patch for that.

Regards,

Hans

> 
> ---
> 
> Re-sending this patch series, as it was c/c to the linux-doc ML by mistake.
> 
> Mauro Carvalho Chehab (3):
>   media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
>   media: v4l2-ctrls: prepare the function to be used by compat32 code
>   media: compat32: reimplement ctrl_is_pointer()
> 
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +-
>  drivers/media/v4l2-core/v4l2-ctrls.c  | 49 
> +--
>  include/media/v4l2-ctrls.h| 28 ++-
>  3 files changed, 67 insertions(+), 28 deletions(-)
>