| Message ID | 1507057753-31808-1-git-send-email-steve_longerbeam@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote: > imx_media_link_notify() should not return error if the source subdevice > is not recognized by imx-media, that isn't an error. If the subdev has > controls they will be inherited starting from a known subdev. What does "a known subdev" mean?
On 10/11/2017 02:49 PM, Russell King - ARM Linux wrote: > On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote: >> imx_media_link_notify() should not return error if the source subdevice >> is not recognized by imx-media, that isn't an error. If the subdev has >> controls they will be inherited starting from a known subdev. > What does "a known subdev" mean? It refers to the previous sentence, "not recognized by imx-media". A subdev that was not registered via async registration and so not in imx-media's async subdev list. I could elaborate in the commit message but it seems fairly obvious to me. Steve
On Wed, Oct 11, 2017 at 03:14:26PM -0700, Steve Longerbeam wrote: > > > On 10/11/2017 02:49 PM, Russell King - ARM Linux wrote: > >On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote: > >>imx_media_link_notify() should not return error if the source subdevice > >>is not recognized by imx-media, that isn't an error. If the subdev has > >>controls they will be inherited starting from a known subdev. > >What does "a known subdev" mean? > > It refers to the previous sentence, "not recognized by imx-media". A > subdev that was not registered via async registration and so not in > imx-media's async subdev list. I could elaborate in the commit message > but it seems fairly obvious to me. I don't think it's obvious, and I suspect you won't think it's obvious in years to come (I talk from experience of some commentry I've added in the past.) Now, isn't it true that for a subdev to be part of a media device, it has to be registered, and if it's part of a media device that is made up of lots of different drivers, it has to use the async registration code? So, is it not also true that any subdev that is part of a media device, it will be "known"? Under what circumstances could a subdev be part of a media device but not be "known" ? Now, if you mean "known" to be equivalent with "recognised by imx-media" then, as I've pointed out several times already, that statement is FALSE. I'm not sure how many times I'm going to have to state this fact. Let me re-iterate again. On my imx219 driver, I have two subdevs. Both subdevs have controls attached. The pixel subdev is not "recognised" by imx-media, and without a modification like my or your patch, it fails. However, with the modification, this "unrecognised" subdev _STILL_ has it's controls visible through imx-media. Hence, I believe your comment in the code and your commit message are misleading and wrong.
On 10/11/2017 04:06 PM, Russell King - ARM Linux wrote: > On Wed, Oct 11, 2017 at 03:14:26PM -0700, Steve Longerbeam wrote: >> >> On 10/11/2017 02:49 PM, Russell King - ARM Linux wrote: >>> On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote: >>>> imx_media_link_notify() should not return error if the source subdevice >>>> is not recognized by imx-media, that isn't an error. If the subdev has >>>> controls they will be inherited starting from a known subdev. >>> What does "a known subdev" mean? >> It refers to the previous sentence, "not recognized by imx-media". A >> subdev that was not registered via async registration and so not in >> imx-media's async subdev list. I could elaborate in the commit message >> but it seems fairly obvious to me. > I don't think it's obvious, and I suspect you won't think it's obvious > in years to come (I talk from experience of some commentry I've added > in the past.) > > Now, isn't it true that for a subdev to be part of a media device, it > has to be registered, and if it's part of a media device that is made > up of lots of different drivers, it has to use the async registration > code? So, is it not also true that any subdev that is part of a > media device, it will be "known"? > > Under what circumstances could a subdev be part of a media device but > not be "known" ? > > Now, if you mean "known" to be equivalent with "recognised by > imx-media" then, as I've pointed out several times already, that > statement is FALSE. I'm not sure how many times I'm going to have to > state this fact. Let me re-iterate again. On my imx219 driver, I > have two subdevs. Both subdevs have controls attached. The pixel > subdev is not "recognised" by imx-media, and without a modification > like my or your patch, it fails. However, with the modification, > this "unrecognised" subdev _STILL_ has it's controls visible through > imx-media. Well that's true, the controls for your pixel subdev (which was not registered via async) still are visible to imx-media, so in that sense the subdev is "known" to imx-media. > > Hence, I believe your comment in the code and your commit message > are misleading and wrong. Ok you convinced me, I'll fix the code comment and commit message. Steve
On Thu, Oct 12, 2017 at 12:06:33AM +0100, Russell King - ARM Linux wrote: > Now, if you mean "known" to be equivalent with "recognised by > imx-media" then, as I've pointed out several times already, that > statement is FALSE. I'm not sure how many times I'm going to have to > state this fact. Let me re-iterate again. On my imx219 driver, I > have two subdevs. Both subdevs have controls attached. The pixel > subdev is not "recognised" by imx-media, and without a modification > like my or your patch, it fails. However, with the modification, > this "unrecognised" subdev _STILL_ has it's controls visible through > imx-media. If you refuse to believe what I'm saying, then read through: http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=csi-v6&id=e3f847cd37b007d55b76282414bfcf13abb8fc9a and look at this: # for f in 2 3 4 5 6 7 8 9; do v4l2-ctl -L -d /dev/video$f; done # ./cam/cam-setup.sh # v4l2-ctl -L -d /dev/video6 User Controls gain (int) : min=256 max=4095 step=1 default=256 value=256 fim_enable (bool) : default=0 value=0 fim_num_average (int) : min=1 max=64 step=1 default=8 value=8 fim_tolerance_min (int) : min=2 max=200 step=1 default=50 value=50 fim_tolerance_max (int) : min=0 max=500 step=1 default=0 value=0 fim_num_skip (int) : min=0 max=256 step=1 default=2 value=2 fim_input_capture_edge (int) : min=0 max=3 step=1 default=0 value=0 fim_input_capture_channel (int) : min=0 max=1 step=1 default=0 value=0 Camera Controls exposure_time_absolute (int) : min=1 max=399 step=1 default=399 value=166 Image Source Controls vertical_blanking (int) : min=32 max=64814 step=1 default=2509 value=2509 flags=read-only horizontal_blanking (int) : min=2168 max=31472 step=1 default=2168 value=2168 flags=read-only analogue_gain (int) : min=1000 max=8000 step=1 default=1000 value=1000 red_pixel_value (int) : min=0 max=1023 step=1 default=0 value=0 flags=inactive green_red_pixel_value (int) : min=0 max=1023 step=1 default=0 value=0 flags=inactive blue_pixel_value (int) : min=0 max=1023 step=1 default=0 value=0 flags=inactive green_blue_pixel_value (int) : min=0 max=1023 step=1 default=0 value=0 flags=inactive Image Processing Controls pixel_rate (int64) : min=160000000 max=278400000 step=1 default=278400000 value=278400000 flags=read-only test_pattern (menu) : min=0 max=9 default=0 value=0 0: Disabled 1: Solid Color 2: 100% Color Bars 3: Fade to Grey Color Bar 4: PN9 5: 16 Split Color Bar 6: 16 Split Inverted Color Bar 7: Column Counter 8: Inverted Column Counter 9: PN31 Now, the pixel array subdev has these controls: gain vertical_blanking horizontal_blanking analogue_gain pixel_rate exposure_time_absolute The root subdev (which is the one which connects to your code) has these controls: red_pixel_value green_red_pixel_value blue_pixel_value green_blue_pixel_value test_pattern As you can see from the above output, _all_ controls from _both_ subdevs are listed. Now, before you spot your old patch adding v4l2_pipeline_inherit_controls() and try to blame that for this, nothing in my kernel calls that function, so that patch can be dropped, and so it's not responsible for this.
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c index b55e5eb..dd47861 100644 --- a/drivers/staging/media/imx/imx-media-dev.c +++ b/drivers/staging/media/imx/imx-media-dev.c @@ -508,8 +508,15 @@ static int imx_media_link_notify(struct media_link *link, u32 flags, imxmd = dev_get_drvdata(sd->v4l2_dev->dev); imxsd = imx_media_find_subdev_by_sd(imxmd, sd); - if (IS_ERR(imxsd)) - return PTR_ERR(imxsd); + if (IS_ERR(imxsd)) { + /* + * don't bother if the source subdev isn't known to + * imx-media. If the subdev has controls they will be + * inherited starting from a known subdev. + */ + return 0; + } + imxpad = &imxsd->pad[pad_idx]; /*
imx_media_link_notify() should not return error if the source subdevice is not recognized by imx-media, that isn't an error. If the subdev has controls they will be inherited starting from a known subdev. Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> --- drivers/staging/media/imx/imx-media-dev.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)