Re: RFC: VIDIOC_DBG_G_CHIP_NAME improvements

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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