RFC: VIDIOC_DBG_G_CHIP_NAME improvements

RFC: VIDIOC_DBG_G_CHIP_NAME improvements

[Hans Verkuil]

Now that the VIDIOC_DBG_G_CHIP_NAME ioctl has been added to the v4l2 API I
started work on removing the VIDIOC_DBG_G_CHIP_IDENT support in existing
drivers. Based on that effort I realized that there are a few things that
could be improved.

One thing that Laurent pointed out is that this ioctl should be available
only if CONFIG_VIDEO_ADV_DEBUG is set to prevent abuse by either userspace
or kernelspace. I agree with that, especially since g_chip_ident is being
abused today by some bridge drivers. That should be avoided in the future.

I am also unhappy with the name. G_CHIP_INFO would certainly be more descriptive,
but perhaps we should move a bit more into the direction of the Media Controller
and call it G_ENTITY_INFO. Opinions are welcome.

What surprised me when digging into the existing uses of G_CHIP_IDENT was that
there are more devices than expected that have multiple register blocks. I.e.
rather than a single set of registers they have multiple blocks of registers,
say one block at address 0x1000, another at 0x2000, etc.

Usually such register blocks represent IP blocks inside the chip, each doing
a specific task. In other cases (adv7604) each block corresponds to an i2c
address, each again representing an IP block inside the chip.

In the case of adv7604 it has been implementing by mapping register offsets
to specific i2c addresses, in the case of the cx231xx it has been implemented
by exposing different bridge chips, unfortunately that's done in such a way
that it can't be enumerated.

The existing debug API has no support for discovering such ranges, but having
worked with such a chip I think that having support for this is very desirable.

Since we added a new ioctl anyway, I thought that this is a good time to
extend it a bit and allow range discovery to be implemented:

/**     
 * struct v4l2_dbg_chip_name - VIDIOC_DBG_G_CHIP_NAME argument
 * @match:      which chip to match
 * @flags:      flags that tell whether this range is readable/writable
 * @name:       unique name of the chip
 * @range_name: name of the register range
 * @range_min:  minimum register of the register range
 * @range_max:  maximum register of the register range
 * @reserved:   future extensions
 */     
struct v4l2_dbg_chip_name {
        struct v4l2_dbg_match match;
	__u32 range;
        __u32 flags;
        char name[32];
        char range_name[32];
        __u64 range_start;
        __u64 range_size;
        __u32 reserved[8];
} __attribute__ ((packed));

range is the range index, range_name describes the purpose of the register
range, range_start and size are the start register address and the size of
this register range.

This extension allows you to enumerate the available register ranges for each
device. If there is only one range, then range_size may be 0. This is mostly
for backwards compatibility as otherwise I would have to modify all existing
drivers for this, and also because this is not really necessary for simple
devices with just one range. These are mostly i2c devices with start address
0 and a size of 256 bytes at most.

Comments?

Regards,

	Hans

Re: RFC: VIDIOC_DBG_G_CHIP_NAME improvements

[Laurent Pinchart]

Hi Hans,

On Wednesday 27 March 2013 11:54:34 Hans Verkuil wrote:
> Now that the VIDIOC_DBG_G_CHIP_NAME ioctl has been added to the v4l2 API I
> started work on removing the VIDIOC_DBG_G_CHIP_IDENT support in existing
> drivers. Based on that effort I realized that there are a few things that
> could be improved.
> 
> One thing that Laurent pointed out is that this ioctl should be available
> only if CONFIG_VIDEO_ADV_DEBUG is set to prevent abuse by either userspace
> or kernelspace. I agree with that, especially since g_chip_ident is being
> abused today by some bridge drivers. That should be avoided in the future.
> 
> I am also unhappy with the name. G_CHIP_INFO would certainly be more
> descriptive, but perhaps we should move a bit more into the direction of
> the Media Controller and call it G_ENTITY_INFO. Opinions are welcome.

We need such an ioctl to retrieve extended informations about entities (it's 
been on my to-do list for too long), but I'd like to see it on the media 
device node.

> What surprised me when digging into the existing uses of G_CHIP_IDENT was
> that there are more devices than expected that have multiple register
> blocks. I.e. rather than a single set of registers they have multiple
> blocks of registers, say one block at address 0x1000, another at 0x2000,
> etc.
> 
> Usually such register blocks represent IP blocks inside the chip, each doing
> a specific task. In other cases (adv7604) each block corresponds to an i2c
> address, each again representing an IP block inside the chip.
> 
> In the case of adv7604 it has been implementing by mapping register offsets
> to specific i2c addresses, in the case of the cx231xx it has been
> implemented by exposing different bridge chips, unfortunately that's done
> in such a way that it can't be enumerated.
> 
> The existing debug API has no support for discovering such ranges, but
> having worked with such a chip I think that having support for this is very
> desirable.

As this is really a debug API, and applications (and users) need to know what 
they're doing, do we really need to make the ranges discoverable ? If you 
don't know what ranges a device supports you probably won't know enough to 
poke its registers directly anyway.

> Since we added a new ioctl anyway, I thought that this is a good time to
> extend it a bit and allow range discovery to be implemented:
> 
> /**
>  * struct v4l2_dbg_chip_name - VIDIOC_DBG_G_CHIP_NAME argument
>  * @match:      which chip to match
>  * @flags:      flags that tell whether this range is readable/writable
>  * @name:       unique name of the chip
>  * @range_name: name of the register range
>  * @range_min:  minimum register of the register range
>  * @range_max:  maximum register of the register range
>  * @reserved:   future extensions
>  */
> struct v4l2_dbg_chip_name {
>         struct v4l2_dbg_match match;
> 	__u32 range;
>         __u32 flags;
>         char name[32];
>         char range_name[32];
>         __u64 range_start;
>         __u64 range_size;
>         __u32 reserved[8];
> } __attribute__ ((packed));
> 
> range is the range index, range_name describes the purpose of the register
> range, range_start and size are the start register address and the size of
> this register range.
> 
> This extension allows you to enumerate the available register ranges for
> each device. If there is only one range, then range_size may be 0. This is
> mostly for backwards compatibility as otherwise I would have to modify all
> existing drivers for this, and also because this is not really necessary
> for simple devices with just one range. These are mostly i2c devices with
> start address 0 and a size of 256 bytes at most.

-- 
Regards,

Laurent Pinchart