ISP lane shifter support

Hi all,

In the OMAP ISP driver, I'm interested in being able to choose between 8-bit and 12-bit formats when I have a 12-bit sensor attached. At the moment it looks like it's only possible to define this statically with data_lane_shift in the board definition. But with the ISP's lane shifter, it should be possible for the application to ask for 8-bits although it has a 12-bit sensor attached.

Has anybody already begun implementing this functionality?

One approach that comes to mind is to create a subdev for the bridge/lane shifter in front of the CCDC, but this also seems a bit overkill. Otherwise, perhaps consider the lane shifter a part of the CCDC and put the code in there?

Then ccdc_try_format() would have to check whether the sink pad has a pixel format which is shiftable to the requested pixel format on the source pad. A problem with this might be if there are architectures which have a CCDC but no shifter.

Are there other approaches I'm not considering? Or problems I'm overlooking?

Also- it looks like the CCDC now supports writing 12-bit bayer data to memory. Is that true?

Hi Michael,

Quoted 8 lines · On Friday 21 January 2011 09:40:21 Michael Jones wrote:

Hi all,

In the OMAP ISP driver, I'm interested in being able to choose between 8-bit and 12-bit formats when I have a 12-bit sensor attached. At the moment it looks like it's only possible to define this statically with data_lane_shift in the board definition. But with the ISP's lane shifter, it should be possible for the application to ask for 8-bits although it has a 12-bit sensor attached.

That's right. This would be an interesting feature for the driver. It's also possible to implement this at the input of the video port (but only when forwarding data to the preview engine).

Quoted 2 lines

Has anybody already begun implementing this functionality?

Not that I know of.

Quoted 4 lines

One approach that comes to mind is to create a subdev for the bridge/lane shifter in front of the CCDC, but this also seems a bit overkill. Otherwise, perhaps consider the lane shifter a part of the CCDC and put the code in there?

I would keep the code in isp.c, and call it from ccdc_configure(). It should just be a matter of adding an argument to the function.

Quoted 5 lines

Then ccdc_try_format() would have to check whether the sink pad has a pixel format which is shiftable to the requested pixel format on the source pad. A problem with this might be if there are architectures which have a CCDC but no shifter.

The CCDC module already calls isp_configure_bridge(), so I don't think it's an issue for now. Let's fix the code when (and if) we start using the driver on a platform without a lane shifter.

Quoted 3 lines

Are there other approaches I'm not considering? Or problems I'm overlooking?

As the lane shifter is located at the CCDC input, it might be easier to implement support for this using the CCDC input format. ispvideo.c would need to validate the pipeline when the output of the entity connected to the CCDC input (parallel sensor, CCP2 or CSI2) is configured with a format that can be shifted to the format at the CCDC input.

Quoted 3 lines

Also- it looks like the CCDC now supports writing 12-bit bayer data to memory. Is that true?

That's correct. It will support 8-bit grey data soon (patches have been submitted internally already).

Quoted 10 lines · Laurent Pinchart

Quoted 8 lines · On Friday 21 January 2011 09:40:21 Michael Jones wrote:

Hi all,

In the OMAP ISP driver, I'm interested in being able to choose between 8-bit and 12-bit formats when I have a 12-bit sensor attached. At the moment it looks like it's only possible to define this statically with data_lane_shift in the board definition. But with the ISP's lane shifter, it should be possible for the application to ask for 8-bits although it has a 12-bit sensor attached.

Quoted 3 lines

That's right. This would be an interesting feature for the driver. It's also possible to implement this at the input of the video port (but only when forwarding data to the preview engine).

True, but I do also want the feature available for data written directly to memory.

Quoted 11 lines

Quoted 1 line

Has anybody already begun implementing this functionality?

Not that I know of.

Quoted 4 lines

One approach that comes to mind is to create a subdev for the bridge/lane shifter in front of the CCDC, but this also seems a bit overkill. Otherwise, perhaps consider the lane shifter a part of the CCDC and put the code in there?

I would keep the code in isp.c, and call it from ccdc_configure(). It should just be a matter of adding an argument to the function.

It seems unnecessary to add an arg to ccdc_configure (that's what I understood you to mean). It gets isp_ccdc_device, which has all the necessary info (pixel format in, which output is active, pixel format out). Seems like I could implement it entirely within isp_configure_bridge(). And of course some changes in ccdc_[try/set]_format(). This is what I will try to do.

Quoted 8 lines

Quoted 4 lines

Then ccdc_try_format() would have to check whether the sink pad has a pixel format which is shiftable to the requested pixel format on the source pad. A problem with this might be if there are architectures which have a CCDC but no shifter.

The CCDC module already calls isp_configure_bridge(), so I don't think it's an issue for now. Let's fix the code when (and if) we start using the driver on a platform without a lane shifter.

Agreed.

Quoted 3 lines

Quoted 2 lines

Are there other approaches I'm not considering? Or problems I'm overlooking?

Quoted 5 lines

As the lane shifter is located at the CCDC input, it might be easier to implement support for this using the CCDC input format. ispvideo.c would need to validate the pipeline when the output of the entity connected to the CCDC input (parallel sensor, CCP2 or CSI2) is configured with a format that can be shifted to the format at the CCDC input.

This crossed my mind, but it seems illogical to have a link with a different format at each of its ends.

For instance, I think it is a sensible assumption that media-ctl automatically sets the format at the remote end of a link if you're setting an output pad's format. This is when I thought a subdev of its own would be more logically consistent with the media controller framework (although overkill).

Quoted 7 lines

Quoted 6 lines

As the lane shifter is located at the CCDC input, it might be easier to implement support for this using the CCDC input format. ispvideo.c would need to validate the pipeline when the output of the entity connected to the CCDC input (parallel sensor, CCP2 or CSI2) is configured with a format that can be shifted to the format at the CCDC input.

Quoted 2 lines

This crossed my mind, but it seems illogical to have a link with a different format at each of its ends.

I agree in theory, but it might be problematic for the CCDC. Right now the CCDC can write to memory or send the data to the preview engine, but not both at the same time. That's something that I'd like to change in the future. What happens if the user then sets different widths on the output pads ?

On 01/24/2011 02:57 PM, Laurent Pinchart wrote:
<snip>

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

As the lane shifter is located at the CCDC input, it might be easier to implement support for this using the CCDC input format. ispvideo.c would need to validate the pipeline when the output of the entity connected to the CCDC input (parallel sensor, CCP2 or CSI2) is configured with a format that can be shifted to the format at the CCDC input.

Quoted 8 lines

Quoted 2 lines

This crossed my mind, but it seems illogical to have a link with a different format at each of its ends.

I agree in theory, but it might be problematic for the CCDC. Right now the CCDC can write to memory or send the data to the preview engine, but not both at the same time. That's something that I'd like to change in the future. What happens if the user then sets different widths on the output pads ?

Shouldn't we prohibit the user from doing this in ccdc_[try/set]_format in the first place? By "prohibit", I mean shouldn't we be sure that the pixel format on pad 1 is always the same as on pad 2?

Downside: this suggests that set_fmt on pad 2 could change the fmt on pad 1, which may be unexpected. But that does at least reflect the reality of the hardware, right?

Hi Michael,

Quoted 19 lines · On Monday 24 January 2011 15:16:28 Michael Jones wrote:

On 01/24/2011 02:57 PM, Laurent Pinchart wrote:
<snip>

Quoted 15 lines

Quoted 9 lines

Quoted 6 lines

As the lane shifter is located at the CCDC input, it might be easier to implement support for this using the CCDC input format. ispvideo.c would need to validate the pipeline when the output of the entity connected to the CCDC input (parallel sensor, CCP2 or CSI2) is configured with a format that can be shifted to the format at the CCDC input.

This crossed my mind, but it seems illogical to have a link with a different format at each of its ends.

I agree in theory, but it might be problematic for the CCDC. Right now the CCDC can write to memory or send the data to the preview engine, but not both at the same time. That's something that I'd like to change in the future. What happens if the user then sets different widths on the output pads ?

Quoted 3 lines

Shouldn't we prohibit the user from doing this in ccdc_[try/set]_format in the first place? By "prohibit", I mean shouldn't we be sure that the pixel format on pad 1 is always the same as on pad 2?

Yes we should (although we could have a larger width on the memory write port, as the video port can further shift the data).

Quoted 4 lines

Downside: this suggests that set_fmt on pad 2 could change the fmt on pad 1, which may be unexpected. But that does at least reflect the reality of the hardware, right?

I don't think it would be a good idea to silently change formats on pad 1 when setting the format on pad 2. Applications don't expect that. That's why I've proposed changing the format on pad 0 instead. I agree that it would be better to have the same format on the sensor output and on CCDC pad 0 though.

Quoted 24 lines · Laurent Pinchart

Quoted 22 lines · On Monday 24 January 2011 15:16:28 Michael Jones wrote:

On 01/24/2011 02:57 PM, Laurent Pinchart wrote:
<snip>

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

As the lane shifter is located at the CCDC input, it might be easier to implement support for this using the CCDC input format. ispvideo.c would need to validate the pipeline when the output of the entity connected to the CCDC input (parallel sensor, CCP2 or CSI2) is configured with a format that can be shifted to the format at the CCDC input.

Quoted 8 lines

Quoted 2 lines

This crossed my mind, but it seems illogical to have a link with a different format at each of its ends.

I agree in theory, but it might be problematic for the CCDC. Right now the CCDC can write to memory or send the data to the preview engine, but not both at the same time. That's something that I'd like to change in the future. What happens if the user then sets different widths on the output pads ?

Shouldn't we prohibit the user from doing this in ccdc_[try/set]_format in the first place? By "prohibit", I mean shouldn't we be sure that the pixel format on pad 1 is always the same as on pad 2?

Quoted 2 lines

Yes we should (although we could have a larger width on the memory write port, as the video port can further shift the data).

Doesn't this conflict with your comment below that we shouldn't silently change pad 1 when setting pad 2? How can we ensure that they're always the same if a change in one doesn't result in a change in the other? See my example below.

I didn't realize the video port can further shift the data. Where can I find this in the TRM?

Quoted 9 lines

Quoted 3 lines

Downside: this suggests that set_fmt on pad 2 could change the fmt on pad 1, which may be unexpected. But that does at least reflect the reality of the hardware, right?

I don't think it would be a good idea to silently change formats on pad 1 when setting the format on pad 2. Applications don't expect that. That's why I've proposed changing the format on pad 0 instead. I agree that it would be better to have the same format on the sensor output and on CCDC pad 0 though.

I don't understand how we can change the pixel format on pad 1 without also changing it on pad 2. Let me take a simple example:
0. Default state: all 3 CCDC pads have SGRBG10.
1. Sensor delivers Y10, so I set CCDC pad 0 to Y10. CCDC then changes format of pad 1&2 to Y10 also.
2. I want 8-bit data written to memory, so I set Y8 on pad 1 to use the shifter. Pad 0 stays Y10, but pad 2 can no longer get Y10, so (?) it must be changed to Y8. And I have to allow the change on pad 1 to be able to use the shifter at all.

I agree applications may not expect this behavior. They may _expect_ that they can get Y10 to the video port and Y8 to memory, but they can't. Isn't this just what we pay for the simplicity of building the lane shifter into the CCDC subdev rather than creating its own subdev?

Hi Michael,

Quoted 25 lines · On Tuesday 25 January 2011 10:10:50 Michael Jones wrote:

Quoted 24 lines · Laurent Pinchart

Quoted 22 lines · On Monday 24 January 2011 15:16:28 Michael Jones wrote:

On 01/24/2011 02:57 PM, Laurent Pinchart wrote:
<snip>

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

As the lane shifter is located at the CCDC input, it might be easier to implement support for this using the CCDC input format. ispvideo.c would need to validate the pipeline when the output of the entity connected to the CCDC input (parallel sensor, CCP2 or CSI2) is configured with a format that can be shifted to the format at the CCDC input.

Quoted 8 lines

Quoted 2 lines

This crossed my mind, but it seems illogical to have a link with a different format at each of its ends.

I agree in theory, but it might be problematic for the CCDC. Right now the CCDC can write to memory or send the data to the preview engine, but not both at the same time. That's something that I'd like to change in the future. What happens if the user then sets different widths on the output pads ?

Shouldn't we prohibit the user from doing this in ccdc_[try/set]_format in the first place? By "prohibit", I mean shouldn't we be sure that the pixel format on pad 1 is always the same as on pad 2?

Quoted 7 lines

Quoted 2 lines

Yes we should (although we could have a larger width on the memory write port, as the video port can further shift the data).

Doesn't this conflict with your comment below that we shouldn't silently change pad 1 when setting pad 2? How can we ensure that they're always the same if a change in one doesn't result in a change in the other? See my example below.

Yes it does, and that's why I'm not too sure yet how this should be implemented.

Quoted 2 lines

I didn't realize the video port can further shift the data. Where can I find this in the TRM?

VPIN field of the CCDC_FMTCFG register.

Quoted 20 lines

Quoted 9 lines

Quoted 3 lines

Downside: this suggests that set_fmt on pad 2 could change the fmt on pad 1, which may be unexpected. But that does at least reflect the reality of the hardware, right?

I don't think it would be a good idea to silently change formats on pad 1 when setting the format on pad 2. Applications don't expect that. That's why I've proposed changing the format on pad 0 instead. I agree that it would be better to have the same format on the sensor output and on CCDC pad 0 though.

I don't understand how we can change the pixel format on pad 1 without also changing it on pad 2. Let me take a simple example:
0. Default state: all 3 CCDC pads have SGRBG10.
1. Sensor delivers Y10, so I set CCDC pad 0 to Y10. CCDC then changes format of pad 1&2 to Y10 also.
2. I want 8-bit data written to memory, so I set Y8 on pad 1 to use the shifter. Pad 0 stays Y10, but pad 2 can no longer get Y10, so (?) it must be changed to Y8. And I have to allow the change on pad 1 to be able to use the shifter at all.

Quoted 4 lines

I agree applications may not expect this behavior. They may _expect_ that they can get Y10 to the video port and Y8 to memory, but they can't. Isn't this just what we pay for the simplicity of building the lane shifter into the CCDC subdev rather than creating its own subdev?

It could be, yes. The other option is to modify the format at the CCDC input. I agree that both options have drawbacks.

Hans, Guennadi, any opinion on this ?

Quoted 5 lines

Quoted 3 lines

I didn't realize the video port can further shift the data. Where can I find this in the TRM?

VPIN field of the CCDC_FMTCFG register.

This only plays a role, if cam_d is set to 10 bits raw in CCDC_SYN_MODE.DATSIZ, right?

Quoted 4 lines

It could be, yes. The other option is to modify the format at the CCDC input. I agree that both options have drawbacks.

Hans, Guennadi, any opinion on this ?

Looking at the "Data-Lane Shifter" table (12.27 in my datasheet, in the "Bridge-Lane Shifter" chapter), I think, the first two columns are fixed by the board design, right? So, our freedom lies only in one line there and is a single parameter - the shift value. The output shifter (VPIN) is independent from this one, but not unrelated. It seems logical to me to relate the former one to CCDC's input pad, and the latter one to CCDC's output pad. AFAIU, Laurent, your implementation in what concerns pad configuration is: let the user configure all interfaces independently, and first when we have to actually activate the pipeline (start streaming or configure video buffers) we can verify, whether all parts fit together. So, why don't we stay consistent and do the same here? Give the user both parameters and see how clever they were in the end;) I also think, if we later decide to add some consistency checks, we can always do it.

Thanks
Guennadi

Looking at the "Data-Lane Shifter" table (12.27 in my datasheet, in the "Bridge-Lane Shifter" chapter), I think, the first two columns are fixed by the board design, right? So, our freedom lies only in one line there and is a single parameter - the shift value. The output shifter (VPIN) is independent from this one, but not unrelated. It seems logical to me to relate the former one to CCDC's input pad, and the latter one to CCDC's output pad. AFAIU, Laurent, your implementation in what concerns pad configuration is: let the user configure all interfaces independently, and first when we have to actually activate the pipeline (start streaming or configure video buffers) we can verify, whether all parts fit together.

So, why don't we stay consistent and do the same here? Give the user both parameters and see how clever they were in the end;) I also think, if we later decide to add some consistency checks, we can always do it.

Thanks
Guennadi

Quoted 11 lines · Guennadi Liakhovetski

Looking at the "Data-Lane Shifter" table (12.27 in my datasheet, in the "Bridge-Lane Shifter" chapter), I think, the first two columns are fixed by the board design, right? So, our freedom lies only in one line there and is a single parameter - the shift value. The output shifter (VPIN) is independent from this one, but not unrelated. It seems logical to me to relate the former one to CCDC's input pad, and the latter one to CCDC's output pad. AFAIU, Laurent, your implementation in what concerns pad configuration is: let the user configure all interfaces independently, and first when we have to actually activate the pipeline (start streaming or configure video buffers) we can verify, whether all parts fit together.

I would like to add this lane shifter support. Would you like me to implement it as Guennadi suggested- letting the user set all 3 CCDC pad formats arbitrarily and postpone the consistency checks to streamon time?

Hi Michael,

Quoted 13 lines · On Friday 11 February 2011 13:07:33 Michael Jones wrote:

Quoted 11 lines · Guennadi Liakhovetski

Looking at the "Data-Lane Shifter" table (12.27 in my datasheet, in the "Bridge-Lane Shifter" chapter), I think, the first two columns are fixed by the board design, right? So, our freedom lies only in one line there and is a single parameter - the shift value. The output shifter (VPIN) is independent from this one, but not unrelated. It seems logical to me to relate the former one to CCDC's input pad, and the latter one to CCDC's output pad. AFAIU, Laurent, your implementation in what concerns pad configuration is: let the user configure all interfaces independently, and first when we have to actually activate the pipeline (start streaming or configure video buffers) we can verify, whether all parts fit together.

Quoted 3 lines

I would like to add this lane shifter support. Would you like me to implement it as Guennadi suggested- letting the user set all 3 CCDC pad formats arbitrarily and postpone the consistency checks to streamon time?

I've discussed this with Sakari Ailus, and we would implement it with different formats on the sensor output and the CCDC input. I'd like to get Hans Verkuil's opinion.

Hans, can you weigh in on this? I'm waiting to submit a patch to implement lane shifter support until I get a consensus what the best approach is. Laurent and Sakari favor having a different format on the sensor output than on the CCDC input to indicate a shift.

If you agree that this is a sensible approach, I will go ahead and submit my patch soon.

thanks,
Michael

Quoted 16 lines · Laurent Pinchart

Hi Michael,

Quoted 13 lines · On Friday 11 February 2011 13:07:33 Michael Jones wrote:

Quoted 11 lines · Guennadi Liakhovetski

Looking at the "Data-Lane Shifter" table (12.27 in my datasheet, in the "Bridge-Lane Shifter" chapter), I think, the first two columns are fixed by the board design, right? So, our freedom lies only in one line there and is a single parameter - the shift value. The output shifter (VPIN) is independent from this one, but not unrelated. It seems logical to me to relate the former one to CCDC's input pad, and the latter one to CCDC's output pad. AFAIU, Laurent, your implementation in what concerns pad configuration is: let the user configure all interfaces independently, and first when we have to actually activate the pipeline (start streaming or configure video buffers) we can verify, whether all parts fit together.

Quoted 8 lines

Quoted 3 lines

I would like to add this lane shifter support. Would you like me to implement it as Guennadi suggested- letting the user set all 3 CCDC pad formats arbitrarily and postpone the consistency checks to streamon time?

I've discussed this with Sakari Ailus, and we would implement it with different formats on the sensor output and the CCDC input. I'd like to get Hans Verkuil's opinion.

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner --
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html