Message ID | 1587700513-28449-1-git-send-email-skomatineni@nvidia.com (mailing list archive) |
---|---|
Headers | show |
Series | Add Tegra driver for video capture | expand |
24.04.2020 06:55, Sowjanya Komatineni пишет: Is this driver compiled as a single kernel module file? > +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); > +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver"); > +MODULE_LICENSE("GPL v2"); ... > +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); > +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver"); > +MODULE_LICENSE("GPL v2"); I don't think that these macros are needed in that case. The video.c should be enough, isn't it?
24.04.2020 06:55, Sowjanya Komatineni пишет: > Tegra210 contains a powerful Video Input (VI) hardware controller > which can support up to 6 MIPI CSI camera sensors. > > Each Tegra CSI port can be one-to-one mapped to VI channel and can > capture from an external camera sensor connected to CSI or from > built-in test pattern generator. > > Tegra210 supports built-in test pattern generator from CSI to VI. > > This patch adds a v4l2 capture driver with media interface for > Tegra210 built-in CSI to VI test pattern generator. > > This patch includes TPG support only and all the video pipeline > configuration happens through the video device node. > > Acked-by: Thierry Reding <treding@nvidia.com> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > drivers/staging/media/Kconfig | 2 + > drivers/staging/media/Makefile | 1 + > drivers/staging/media/tegra/Kconfig | 12 + > drivers/staging/media/tegra/Makefile | 8 + > drivers/staging/media/tegra/TODO | 10 + > drivers/staging/media/tegra/common.h | 259 ++++++++ > drivers/staging/media/tegra/csi.c | 604 +++++++++++++++++ > drivers/staging/media/tegra/csi.h | 144 ++++ > drivers/staging/media/tegra/tegra210.c | 708 ++++++++++++++++++++ > drivers/staging/media/tegra/tegra210.h | 190 ++++++ > drivers/staging/media/tegra/vi.c | 1127 ++++++++++++++++++++++++++++++++ > drivers/staging/media/tegra/vi.h | 72 ++ > drivers/staging/media/tegra/video.c | 153 +++++ > drivers/staging/media/tegra/video.h | 29 + The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path should better reflect the driver, IMO. It also should be better to name the compiled kernel module as tegra-vi, IMO.
On 4/24/20 8:07 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 24.04.2020 06:55, Sowjanya Komatineni пишет: > > Is this driver compiled as a single kernel module file? > >> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver"); >> +MODULE_LICENSE("GPL v2"); > ... >> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver"); >> +MODULE_LICENSE("GPL v2"); > I don't think that these macros are needed in that case. > The video.c should be enough, isn't it? yes these can be removed
On 24/04/2020 17:11, Dmitry Osipenko wrote: > 24.04.2020 06:55, Sowjanya Komatineni пишет: >> Tegra210 contains a powerful Video Input (VI) hardware controller >> which can support up to 6 MIPI CSI camera sensors. >> >> Each Tegra CSI port can be one-to-one mapped to VI channel and can >> capture from an external camera sensor connected to CSI or from >> built-in test pattern generator. >> >> Tegra210 supports built-in test pattern generator from CSI to VI. >> >> This patch adds a v4l2 capture driver with media interface for >> Tegra210 built-in CSI to VI test pattern generator. >> >> This patch includes TPG support only and all the video pipeline >> configuration happens through the video device node. >> >> Acked-by: Thierry Reding <treding@nvidia.com> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> drivers/staging/media/Kconfig | 2 + >> drivers/staging/media/Makefile | 1 + >> drivers/staging/media/tegra/Kconfig | 12 + >> drivers/staging/media/tegra/Makefile | 8 + >> drivers/staging/media/tegra/TODO | 10 + >> drivers/staging/media/tegra/common.h | 259 ++++++++ >> drivers/staging/media/tegra/csi.c | 604 +++++++++++++++++ >> drivers/staging/media/tegra/csi.h | 144 ++++ >> drivers/staging/media/tegra/tegra210.c | 708 ++++++++++++++++++++ >> drivers/staging/media/tegra/tegra210.h | 190 ++++++ >> drivers/staging/media/tegra/vi.c | 1127 ++++++++++++++++++++++++++++++++ >> drivers/staging/media/tegra/vi.h | 72 ++ >> drivers/staging/media/tegra/video.c | 153 +++++ >> drivers/staging/media/tegra/video.h | 29 + > > The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path > should better reflect the driver, IMO. > > It also should be better to name the compiled kernel module as tegra-vi, > IMO. > The driver name and the directory should be in sync, so either tegra-video or tegra-vi for both. I have no preference myself, as long as they are the same. This can be done as a follow-up patch. Regards, Hans
25.04.2020 01:00, Sowjanya Komatineni пишет: > > On 4/24/20 8:07 AM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 24.04.2020 06:55, Sowjanya Komatineni пишет: >> >> Is this driver compiled as a single kernel module file? >> >>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver"); >>> +MODULE_LICENSE("GPL v2"); >> ... >>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver"); >>> +MODULE_LICENSE("GPL v2"); >> I don't think that these macros are needed in that case. >> The video.c should be enough, isn't it? > yes these can be removed It will be nice to factor out the Tegra210-specific VI/CSI OPS into a separate driver module (say tegra210-vi) to ease supporting of other Tegra versions. Of course this could be done later on, although I suppose the amount of hassle could be reduced if it's done from the start.
On 4/25/20 3:08 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 25.04.2020 01:00, Sowjanya Komatineni пишет: >> On 4/24/20 8:07 AM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 24.04.2020 06:55, Sowjanya Komatineni пишет: >>> >>> Is this driver compiled as a single kernel module file? >>> >>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver"); >>>> +MODULE_LICENSE("GPL v2"); >>> ... >>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver"); >>>> +MODULE_LICENSE("GPL v2"); >>> I don't think that these macros are needed in that case. >>> The video.c should be enough, isn't it? >> yes these can be removed > It will be nice to factor out the Tegra210-specific VI/CSI OPS into a > separate driver module (say tegra210-vi) to ease supporting of other > Tegra versions. Of course this could be done later on, although I > suppose the amount of hassle could be reduced if it's done from the start. vi/csi.c are common drivers for all Tegras. All Tegra chip specific related programming for both vi and csi were already moved to Tegra210.c based on prior feedbacks.
24.04.2020 06:55, Sowjanya Komatineni пишет: > +static int __maybe_unused vi_runtime_resume(struct device *dev) > +{ > + struct tegra_vi *vi = dev_get_drvdata(dev); > + int ret; > + > + ret = regulator_enable(vi->vdd); > + if (ret) { > + dev_err(dev, "failed to enable VDD supply: %d\n", ret); > + return ret; > + } > + > + ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz); > + if (ret) { > + dev_err(dev, "failed to set vi clock rate: %d\n", ret); > + goto disable_vdd; > + } Isn't setting clock rate using assigned-clocks in a device-tree enough? Could you please clarify why this vi_max_clk_hz is needed?
On 4/25/20 4:13 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 24.04.2020 06:55, Sowjanya Komatineni пишет: >> +static int __maybe_unused vi_runtime_resume(struct device *dev) >> +{ >> + struct tegra_vi *vi = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = regulator_enable(vi->vdd); >> + if (ret) { >> + dev_err(dev, "failed to enable VDD supply: %d\n", ret); >> + return ret; >> + } >> + >> + ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz); >> + if (ret) { >> + dev_err(dev, "failed to set vi clock rate: %d\n", ret); >> + goto disable_vdd; >> + } > Isn't setting clock rate using assigned-clocks in a device-tree enough? > Could you please clarify why this vi_max_clk_hz is needed? Max clock rate with sensor support will be 998Mhz. Later when sensor support is added, based on TPG or Sensor mode clock rate will be set here
26.04.2020 02:13, Dmitry Osipenko пишет: > 24.04.2020 06:55, Sowjanya Komatineni пишет: >> +static int __maybe_unused vi_runtime_resume(struct device *dev) >> +{ >> + struct tegra_vi *vi = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = regulator_enable(vi->vdd); >> + if (ret) { >> + dev_err(dev, "failed to enable VDD supply: %d\n", ret); >> + return ret; >> + } >> + >> + ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz); >> + if (ret) { >> + dev_err(dev, "failed to set vi clock rate: %d\n", ret); >> + goto disable_vdd; >> + } > > Isn't setting clock rate using assigned-clocks in a device-tree enough? > Could you please clarify why this vi_max_clk_hz is needed? > In that case it should be wrong to set the clock rate in the RPM callback because RPM works asynchronously and RPM may not be suspended on TGP -> sensor source switch.
On 4/25/20 4:25 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 02:13, Dmitry Osipenko пишет: >> 24.04.2020 06:55, Sowjanya Komatineni пишет: >>> +static int __maybe_unused vi_runtime_resume(struct device *dev) >>> +{ >>> + struct tegra_vi *vi = dev_get_drvdata(dev); >>> + int ret; >>> + >>> + ret = regulator_enable(vi->vdd); >>> + if (ret) { >>> + dev_err(dev, "failed to enable VDD supply: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz); >>> + if (ret) { >>> + dev_err(dev, "failed to set vi clock rate: %d\n", ret); >>> + goto disable_vdd; >>> + } >> Isn't setting clock rate using assigned-clocks in a device-tree enough? >> Could you please clarify why this vi_max_clk_hz is needed? >> > In that case it should be wrong to set the clock rate in the RPM > callback because RPM works asynchronously and RPM may not be suspended > on TGP -> sensor source switch. Driver will not do TPG and Sensor switch dynamically. Based on kconfig, it will only do TPG or Sensor and sensor will be default all the time once sensor support is added in next series.
24.04.2020 06:55, Sowjanya Komatineni пишет: > +static int tegra_csi_init(struct host1x_client *client) > +{ > + struct tegra_csi *csi = host1x_client_to_csi(client); > + struct tegra_video_device *vid = dev_get_drvdata(client->host); > + int ret; > + > + INIT_LIST_HEAD(&csi->csi_chans); > + > + ret = pm_runtime_get_sync(csi->dev); > + if (ret < 0) { > + dev_err(csi->dev, "failed to get runtime PM: %d\n", ret); > + pm_runtime_put_noidle(csi->dev); > + return ret; > + } The whole point of RPM is to keep hardware enabled only when needed, i.e. during of the capture process in this case. You should move all RPM handling to the capture start / stop functions.
26.04.2020 01:11, Sowjanya Komatineni пишет: > > On 4/25/20 3:08 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 25.04.2020 01:00, Sowjanya Komatineni пишет: >>> On 4/24/20 8:07 AM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 24.04.2020 06:55, Sowjanya Komatineni пишет: >>>> >>>> Is this driver compiled as a single kernel module file? >>>> >>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>>>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver"); >>>>> +MODULE_LICENSE("GPL v2"); >>>> ... >>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>>>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver"); >>>>> +MODULE_LICENSE("GPL v2"); >>>> I don't think that these macros are needed in that case. >>>> The video.c should be enough, isn't it? >>> yes these can be removed >> It will be nice to factor out the Tegra210-specific VI/CSI OPS into a >> separate driver module (say tegra210-vi) to ease supporting of other >> Tegra versions. Of course this could be done later on, although I >> suppose the amount of hassle could be reduced if it's done from the >> start. > vi/csi.c are common drivers for all Tegras. All Tegra chip specific > related programming for both vi and csi were already moved to Tegra210.c > based on prior feedbacks. Judging by the code's structure the VI/CSI drivers aren't planned to be reused by older pre-Terga210 SoCs, correct? How much of the T210 code could be reused by T186/194?
On 4/25/20 4:40 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 01:11, Sowjanya Komatineni пишет: >> On 4/25/20 3:08 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 25.04.2020 01:00, Sowjanya Komatineni пишет: >>>> On 4/24/20 8:07 AM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 24.04.2020 06:55, Sowjanya Komatineni пишет: >>>>> >>>>> Is this driver compiled as a single kernel module file? >>>>> >>>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>>>>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver"); >>>>>> +MODULE_LICENSE("GPL v2"); >>>>> ... >>>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>"); >>>>>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver"); >>>>>> +MODULE_LICENSE("GPL v2"); >>>>> I don't think that these macros are needed in that case. >>>>> The video.c should be enough, isn't it? >>>> yes these can be removed >>> It will be nice to factor out the Tegra210-specific VI/CSI OPS into a >>> separate driver module (say tegra210-vi) to ease supporting of other >>> Tegra versions. Of course this could be done later on, although I >>> suppose the amount of hassle could be reduced if it's done from the >>> start. >> vi/csi.c are common drivers for all Tegras. All Tegra chip specific >> related programming for both vi and csi were already moved to Tegra210.c >> based on prior feedbacks. > Judging by the code's structure the VI/CSI drivers aren't planned to be > reused by older pre-Terga210 SoCs, correct? > > How much of the T210 code could be reused by T186/194? vi/csi are common driver where soc structure should be populated for T186/T194 Tegra210.c can't be reused for Tegra186/t194 as programming seq is a whole lot diff
26.04.2020 02:27, Sowjanya Komatineni пишет: > > On 4/25/20 4:25 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 26.04.2020 02:13, Dmitry Osipenko пишет: >>> 24.04.2020 06:55, Sowjanya Komatineni пишет: >>>> +static int __maybe_unused vi_runtime_resume(struct device *dev) >>>> +{ >>>> + struct tegra_vi *vi = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + ret = regulator_enable(vi->vdd); >>>> + if (ret) { >>>> + dev_err(dev, "failed to enable VDD supply: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz); >>>> + if (ret) { >>>> + dev_err(dev, "failed to set vi clock rate: %d\n", ret); >>>> + goto disable_vdd; >>>> + } >>> Isn't setting clock rate using assigned-clocks in a device-tree enough? >>> Could you please clarify why this vi_max_clk_hz is needed? >>> >> In that case it should be wrong to set the clock rate in the RPM >> callback because RPM works asynchronously and RPM may not be suspended >> on TGP -> sensor source switch. > > Driver will not do TPG and Sensor switch dynamically. > > Based on kconfig, it will only do TPG or Sensor and sensor will be > default all the time once sensor support is added in next series. > Doesn't V4L have a native support for the capture source selection? Why it needs to be a compile-time option? I think other drivers use a generic V4L "Image Processing Controls" with a configurable test_pattern option.
On 4/25/20 4:44 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 02:27, Sowjanya Komatineni пишет: >> On 4/25/20 4:25 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 26.04.2020 02:13, Dmitry Osipenko пишет: >>>> 24.04.2020 06:55, Sowjanya Komatineni пишет: >>>>> +static int __maybe_unused vi_runtime_resume(struct device *dev) >>>>> +{ >>>>> + struct tegra_vi *vi = dev_get_drvdata(dev); >>>>> + int ret; >>>>> + >>>>> + ret = regulator_enable(vi->vdd); >>>>> + if (ret) { >>>>> + dev_err(dev, "failed to enable VDD supply: %d\n", ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz); >>>>> + if (ret) { >>>>> + dev_err(dev, "failed to set vi clock rate: %d\n", ret); >>>>> + goto disable_vdd; >>>>> + } >>>> Isn't setting clock rate using assigned-clocks in a device-tree enough? >>>> Could you please clarify why this vi_max_clk_hz is needed? >>>> >>> In that case it should be wrong to set the clock rate in the RPM >>> callback because RPM works asynchronously and RPM may not be suspended >>> on TGP -> sensor source switch. >> Driver will not do TPG and Sensor switch dynamically. >> >> Based on kconfig, it will only do TPG or Sensor and sensor will be >> default all the time once sensor support is added in next series. >> > Doesn't V4L have a native support for the capture source selection? Why > it needs to be a compile-time option? > > I think other drivers use a generic V4L "Image Processing Controls" with > a configurable test_pattern option. selecting test pattern thru v4l2 controls is once device graph is already set for TPG to choose test pattern modes. But as internal TPG is b/w CSI and VI only, device graph is diff and for sensor device graph is different. Based on internal discussion and discussion with Hans, decided to use kconfig for TPG Vs Sensor and TPG is rarely used only to test driver without sensor
26.04.2020 02:44, Sowjanya Komatineni пишет: ... >> How much of the T210 code could be reused by T186/194? > > vi/csi are common driver where soc structure should be populated for > T186/T194 > > Tegra210.c can't be reused for Tegra186/t194 as programming seq is a > whole lot diff > How are you going to separate Tegra210/186/194 drivers from each other? I don't think you'll want to have one "fat" driver that covers all those SoCs, won't you? In the end it should be three modules: tegra210-video.ko tegra186-video.ko tegra194-video.ko. Using a per-SoC OPS doesn't allow you to do that because the "root" driver will have to lookup OPS' code symbols of every SoC, and thus, the unwanted driver modules will get auto-loaded if you'll try to factor out the OPS into a separate driver modules.
On 4/25/20 5:19 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 02:44, Sowjanya Komatineni пишет: > ... >>> How much of the T210 code could be reused by T186/194? >> vi/csi are common driver where soc structure should be populated for >> T186/T194 >> >> Tegra210.c can't be reused for Tegra186/t194 as programming seq is a >> whole lot diff >> > How are you going to separate Tegra210/186/194 drivers from each other? > I don't think you'll want to have one "fat" driver that covers all those > SoCs, won't you? > > In the end it should be three modules: tegra210-video.ko > tegra186-video.ko tegra194-video.ko. > > Using a per-SoC OPS doesn't allow you to do that because the "root" > driver will have to lookup OPS' code symbols of every SoC, and thus, the > unwanted driver modules will get auto-loaded if you'll try to factor out > the OPS into a separate driver modules. tegra-video driver will be common for t210/186/194 we add corresponding compatibles to tegra-video and vi/csi drivers along with that need to add tegra186_vi_soc, tegra194_vi_soc and implement tegra186.c/tegra194.c tegra-video driver updated for later tegra include updating drivers list and subdevs list to add t186/t194 compatibles
25.04.2020 12:36, Hans Verkuil пишет: ... >> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path >> should better reflect the driver, IMO. >> >> It also should be better to name the compiled kernel module as tegra-vi, >> IMO. >> > > The driver name and the directory should be in sync, so either tegra-video > or tegra-vi for both. I have no preference myself, as long as they are the > same. > > This can be done as a follow-up patch. Given that this driver isn't going to be reused by older pre-Tegra210 SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or tegra210-video.
26.04.2020 03:24, Sowjanya Komatineni пишет: > > On 4/25/20 5:19 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 26.04.2020 02:44, Sowjanya Komatineni пишет: >> ... >>>> How much of the T210 code could be reused by T186/194? >>> vi/csi are common driver where soc structure should be populated for >>> T186/T194 >>> >>> Tegra210.c can't be reused for Tegra186/t194 as programming seq is a >>> whole lot diff >>> >> How are you going to separate Tegra210/186/194 drivers from each other? >> I don't think you'll want to have one "fat" driver that covers all those >> SoCs, won't you? >> >> In the end it should be three modules: tegra210-video.ko >> tegra186-video.ko tegra194-video.ko. >> >> Using a per-SoC OPS doesn't allow you to do that because the "root" >> driver will have to lookup OPS' code symbols of every SoC, and thus, the >> unwanted driver modules will get auto-loaded if you'll try to factor out >> the OPS into a separate driver modules. > > tegra-video driver will be common for t210/186/194 Oh, well. > we add corresponding compatibles to tegra-video and vi/csi drivers along > with that need to add tegra186_vi_soc, tegra194_vi_soc and implement > tegra186.c/tegra194.c > > tegra-video driver updated for later tegra include updating drivers list > and subdevs list to add t186/t194 compatibles
On 4/25/20 5:36 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 25.04.2020 12:36, Hans Verkuil пишет: > ... >>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path >>> should better reflect the driver, IMO. >>> >>> It also should be better to name the compiled kernel module as tegra-vi, >>> IMO. >>> >> The driver name and the directory should be in sync, so either tegra-video >> or tegra-vi for both. I have no preference myself, as long as they are the >> same. >> >> This can be done as a follow-up patch. > Given that this driver isn't going to be reused by older pre-Tegra210 > SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or > tegra210-video. Can you explain what do you meant by can't be reused for pre-tegra210 or for tegra186/194? support for other tegra's can be added to same tegra-video driver. tegra-video host1x driver can be updated to add other tegra's vi and csi compatibles to host1x subdevs and vi and csi driver can be updated to add other tegra soc data and need to add coresponding tegra186/194/xxx.c file with tegra specific prog sequence
On 4/25/20 4:29 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 24.04.2020 06:55, Sowjanya Komatineni пишет: >> +static int tegra_csi_init(struct host1x_client *client) >> +{ >> + struct tegra_csi *csi = host1x_client_to_csi(client); >> + struct tegra_video_device *vid = dev_get_drvdata(client->host); >> + int ret; >> + >> + INIT_LIST_HEAD(&csi->csi_chans); >> + >> + ret = pm_runtime_get_sync(csi->dev); >> + if (ret < 0) { >> + dev_err(csi->dev, "failed to get runtime PM: %d\n", ret); >> + pm_runtime_put_noidle(csi->dev); >> + return ret; >> + } > The whole point of RPM is to keep hardware enabled only when needed, > i.e. during of the capture process in this case. You should move all RPM > handling to the capture start / stop functions. Will move it to handle during stream start/stop
On 4/25/20 5:41 PM, Sowjanya Komatineni wrote: > > On 4/25/20 5:36 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 25.04.2020 12:36, Hans Verkuil пишет: >> ... >>>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path >>>> should better reflect the driver, IMO. >>>> >>>> It also should be better to name the compiled kernel module as >>>> tegra-vi, >>>> IMO. >>>> >>> The driver name and the directory should be in sync, so either >>> tegra-video >>> or tegra-vi for both. I have no preference myself, as long as they >>> are the >>> same. >>> >>> This can be done as a follow-up patch. >> Given that this driver isn't going to be reused by older pre-Tegra210 >> SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or >> tegra210-video. > > Can you explain what do you meant by can't be reused for pre-tegra210 > or for tegra186/194? > > support for other tegra's can be added to same tegra-video driver. > tegra-video host1x driver can be updated to add other tegra's vi and > csi compatibles to host1x subdevs and vi and csi driver can be updated > to add other tegra soc data and need to add coresponding > tegra186/194/xxx.c file with tegra specific prog sequence > Same tegra-video.ko can be used for all tegra soc as driver supports adding other soc related as well. Also was thinking instead of renaming media/tegra as media/tegra-vi, probably we can rename as media/tegra-video so it will be inline with module name we already chosen and also mainly we have vi and csi with in that so instead of tegra-vi probably we can use media/tegra-video?
26.04.2020 04:08, Sowjanya Komatineni пишет: > > On 4/25/20 5:41 PM, Sowjanya Komatineni wrote: >> >> On 4/25/20 5:36 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 25.04.2020 12:36, Hans Verkuil пишет: >>> ... >>>>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path >>>>> should better reflect the driver, IMO. >>>>> >>>>> It also should be better to name the compiled kernel module as >>>>> tegra-vi, >>>>> IMO. >>>>> >>>> The driver name and the directory should be in sync, so either >>>> tegra-video >>>> or tegra-vi for both. I have no preference myself, as long as they >>>> are the >>>> same. >>>> >>>> This can be done as a follow-up patch. >>> Given that this driver isn't going to be reused by older pre-Tegra210 >>> SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or >>> tegra210-video. >> >> Can you explain what do you meant by can't be reused for pre-tegra210 >> or for tegra186/194? It looks to me that at least all those hardcoded HW format IDs do not match the older SoCs. >> support for other tegra's can be added to same tegra-video driver. >> tegra-video host1x driver can be updated to add other tegra's vi and >> csi compatibles to host1x subdevs and vi and csi driver can be updated >> to add other tegra soc data and need to add coresponding >> tegra186/194/xxx.c file with tegra specific prog sequence >> > Same tegra-video.ko can be used for all tegra soc as driver supports > adding other soc related as well. Well, I'm still not sure why you would want to have all the unnecessary code of a different SoCs shared within the same kernel module, it will be quite be a lot wasted space in comparison to a used part of the driver. The driver will need to have a bit better separation if it's supposed to have a common core for all SoCs. Each incompatible VI/CSI hardware version should have its own kernel module. > Also was thinking instead of renaming media/tegra as media/tegra-vi, > probably we can rename as media/tegra-video so it will be inline with > module name we already chosen and also mainly we have vi and csi with in > that so instead of tegra-vi probably we can use media/tegra-video? The tegra-video should be okay, although the "video" part sounds a bit too broad since video could mean a lot of things. I think downstream kernel uses (or at least used) the tegra-camera name for the driver, perhaps it could be a reasonable variant as well.
On 4/25/20 6:26 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 04:08, Sowjanya Komatineni пишет: >> On 4/25/20 5:41 PM, Sowjanya Komatineni wrote: >>> On 4/25/20 5:36 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 25.04.2020 12:36, Hans Verkuil пишет: >>>> ... >>>>>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path >>>>>> should better reflect the driver, IMO. >>>>>> >>>>>> It also should be better to name the compiled kernel module as >>>>>> tegra-vi, >>>>>> IMO. >>>>>> >>>>> The driver name and the directory should be in sync, so either >>>>> tegra-video >>>>> or tegra-vi for both. I have no preference myself, as long as they >>>>> are the >>>>> same. >>>>> >>>>> This can be done as a follow-up patch. >>>> Given that this driver isn't going to be reused by older pre-Tegra210 >>>> SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or >>>> tegra210-video. >>> Can you explain what do you meant by can't be reused for pre-tegra210 >>> or for tegra186/194? > It looks to me that at least all those hardcoded HW format IDs do not > match the older SoCs. TPG hard coded formats are supported on prior Tegra. Other supported formats are SoC dependent and part of soc data in the driver already. >>> support for other tegra's can be added to same tegra-video driver. >>> tegra-video host1x driver can be updated to add other tegra's vi and >>> csi compatibles to host1x subdevs and vi and csi driver can be updated >>> to add other tegra soc data and need to add coresponding >>> tegra186/194/xxx.c file with tegra specific prog sequence >>> >> Same tegra-video.ko can be used for all tegra soc as driver supports >> adding other soc related as well. > Well, I'm still not sure why you would want to have all the unnecessary > code of a different SoCs shared within the same kernel module, it will > be quite be a lot wasted space in comparison to a used part of the driver. > > The driver will need to have a bit better separation if it's supposed to > have a common core for all SoCs. Each incompatible VI/CSI hardware > version should have its own kernel module. currently other Tegra host1x driver (drm) also does similar. Single module for all Tegra SoCs. With current tegra-video, all the v4l2 related common part of implementation is same for all tegra's and only tegra210.c/tegra186.c/tegra194.c will have corresponding tegra soc specific vi/csi programming sequence. >> Also was thinking instead of renaming media/tegra as media/tegra-vi, >> probably we can rename as media/tegra-video so it will be inline with >> module name we already chosen and also mainly we have vi and csi with in >> that so instead of tegra-vi probably we can use media/tegra-video? > The tegra-video should be okay, although the "video" part sounds a bit > too broad since video could mean a lot of things. I think downstream > kernel uses (or at least used) the tegra-camera name for the driver, > perhaps it could be a reasonable variant as well. prior feedback suggests not to use camera variant instead to use video
26.04.2020 04:43, Sowjanya Komatineni пишет: ... >> It looks to me that at least all those hardcoded HW format IDs do not >> match the older SoCs. > > TPG hard coded formats are supported on prior Tegra. > > Other supported formats are SoC dependent and part of soc data in the > driver already. But I don't see where that SoC-dependent definition is made in terga210.c. That tegra_image_format enum looks T210-specific, isn't it? ... >> The driver will need to have a bit better separation if it's supposed to >> have a common core for all SoCs. Each incompatible VI/CSI hardware >> version should have its own kernel module. > > currently other Tegra host1x driver (drm) also does similar. Single > module for all Tegra SoCs. DRM driver has a proper separation of the sub-drivers where sub-driver won't load on unsupported hardware. The tegra-video driver should do the same, i.e. VI and CSI should be individual drivers (and not OPS). There could be a some common core, but for now it's not obvious to me what that core should be, maybe just the video.c. > With current tegra-video, all the v4l2 related common part of > implementation is same for all tegra's and only > tegra210.c/tegra186.c/tegra194.c will have corresponding tegra soc > specific vi/csi programming sequence. This code shouldn't be shared within the same driver module, IMO. >> The tegra-video should be okay, although the "video" part sounds a bit >> too broad since video could mean a lot of things. I think downstream >> kernel uses (or at least used) the tegra-camera name for the driver, >> perhaps it could be a reasonable variant as well. > prior feedback suggests not to use camera variant instead to use video Alright, then the tegra-video should be fine.
On 4/25/20 7:10 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 04:43, Sowjanya Komatineni пишет: > ... >>> It looks to me that at least all those hardcoded HW format IDs do not >>> match the older SoCs. >> TPG hard coded formats are supported on prior Tegra. >> >> Other supported formats are SoC dependent and part of soc data in the >> driver already. > But I don't see where that SoC-dependent definition is made in > terga210.c. That tegra_image_format enum looks T210-specific, isn't it? > > ... Video formats which are SoC variants are made soc specific in driver already tegra_vi_soc structure member video_formats tegra_image_format enum is same for T210 and T186 For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT using corresponding Tegra194 video format enums >>> The driver will need to have a bit better separation if it's supposed to >>> have a common core for all SoCs. Each incompatible VI/CSI hardware >>> version should have its own kernel module. >> currently other Tegra host1x driver (drm) also does similar. Single >> module for all Tegra SoCs. > DRM driver has a proper separation of the sub-drivers where sub-driver > won't load on unsupported hardware. The tegra-video driver should do the > same, i.e. VI and CSI should be individual drivers (and not OPS). There > could be a some common core, but for now it's not obvious to me what > that core should be, maybe just the video.c. > >> With current tegra-video, all the v4l2 related common part of >> implementation is same for all tegra's and only >> tegra210.c/tegra186.c/tegra194.c will have corresponding tegra soc >> specific vi/csi programming sequence. > This code shouldn't be shared within the same driver module, IMO. > > >>> The tegra-video should be okay, although the "video" part sounds a bit >>> too broad since video could mean a lot of things. I think downstream >>> kernel uses (or at least used) the tegra-camera name for the driver, >>> perhaps it could be a reasonable variant as well. >> prior feedback suggests not to use camera variant instead to use video > Alright, then the tegra-video should be fine.
26.04.2020 05:10, Dmitry Osipenko пишет: ... >> currently other Tegra host1x driver (drm) also does similar. Single >> module for all Tegra SoCs. > > DRM driver has a proper separation of the sub-drivers where sub-driver > won't load on unsupported hardware. The tegra-video driver should do the > same, i.e. VI and CSI should be individual drivers (and not OPS). There > could be a some common core, but for now it's not obvious to me what > that core should be, maybe just the video.c. Maybe video.c csi.c vi.c could be moved into a separate module, somewhat like a common driver framework. Then the individual CSI/VI drivers will use those common helpers.. Just a quick thought.
26.04.2020 05:19, Sowjanya Komatineni пишет: > > On 4/25/20 7:10 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 26.04.2020 04:43, Sowjanya Komatineni пишет: >> ... >>>> It looks to me that at least all those hardcoded HW format IDs do not >>>> match the older SoCs. >>> TPG hard coded formats are supported on prior Tegra. >>> >>> Other supported formats are SoC dependent and part of soc data in the >>> driver already. >> But I don't see where that SoC-dependent definition is made in >> terga210.c. That tegra_image_format enum looks T210-specific, isn't it? >> >> ... > > Video formats which are SoC variants are made soc specific in driver > already tegra_vi_soc structure member video_formats > > tegra_image_format enum is same for T210 and T186 > > For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT > using corresponding Tegra194 video format enums But it is also not the same for older SoCs, correct? All the T210-specific things should be separated better, unique parts shouldn't be kept in the common code. Hence the tegra_image_format should be renamed to tegra210_image_format and moved out to t210.h, since it's not common. But then you'll probably need to rename all TEGRA_ defines to TEGRA210_ to make t210.h reusable by T186. Also, in the end it may not worth the effort to share anything at all, it could be cleaner to have a bit of duplication. Although, I have no idea how T186 code will look like and what other parts of T210 could be reused by T186. All this needs to be taken into account in order to avoid struggling with the code's reshuffling in the future.
On 4/25/20 7:38 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 05:19, Sowjanya Komatineni пишет: >> On 4/25/20 7:10 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 26.04.2020 04:43, Sowjanya Komatineni пишет: >>> ... >>>>> It looks to me that at least all those hardcoded HW format IDs do not >>>>> match the older SoCs. >>>> TPG hard coded formats are supported on prior Tegra. >>>> >>>> Other supported formats are SoC dependent and part of soc data in the >>>> driver already. >>> But I don't see where that SoC-dependent definition is made in >>> terga210.c. That tegra_image_format enum looks T210-specific, isn't it? >>> >>> ... >> Video formats which are SoC variants are made soc specific in driver >> already tegra_vi_soc structure member video_formats >> >> tegra_image_format enum is same for T210 and T186 >> >> For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT >> using corresponding Tegra194 video format enums > But it is also not the same for older SoCs, correct? All the > T210-specific things should be separated better, unique parts shouldn't > be kept in the common code. > > Hence the tegra_image_format should be renamed to tegra210_image_format > and moved out to t210.h, since it's not common. But then you'll probably > need to rename all TEGRA_ defines to TEGRA210_ to make t210.h reusable > by T186. We can't make t210.h reusable for t186 as all register defines are diff. only video format enums are same b/w them so to reuse that for t186 I had that in common. Regarding defines, will change prefix as Tegra210 > > Also, in the end it may not worth the effort to share anything at all, > it could be cleaner to have a bit of duplication. Although, I have no > idea how T186 code will look like and what other parts of T210 could be > reused by T186. All this needs to be taken into account in order to > avoid struggling with the code's reshuffling in the future. Currently as image formats are same for t210 and t186 I had them in common.h and for t194 where they are diff new enums will be added. Other tegra210 soc specific only are all part of tegra210.c/h
On 4/25/20 7:48 PM, Sowjanya Komatineni wrote: > > On 4/25/20 7:38 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 26.04.2020 05:19, Sowjanya Komatineni пишет: >>> On 4/25/20 7:10 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 26.04.2020 04:43, Sowjanya Komatineni пишет: >>>> ... >>>>>> It looks to me that at least all those hardcoded HW format IDs do >>>>>> not >>>>>> match the older SoCs. >>>>> TPG hard coded formats are supported on prior Tegra. >>>>> >>>>> Other supported formats are SoC dependent and part of soc data in >>>>> the >>>>> driver already. >>>> But I don't see where that SoC-dependent definition is made in >>>> terga210.c. That tegra_image_format enum looks T210-specific, isn't >>>> it? >>>> >>>> ... >>> Video formats which are SoC variants are made soc specific in driver >>> already tegra_vi_soc structure member video_formats >>> >>> tegra_image_format enum is same for T210 and T186 >>> >>> For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT >>> using corresponding Tegra194 video format enums >> But it is also not the same for older SoCs, correct? All the >> T210-specific things should be separated better, unique parts shouldn't >> be kept in the common code. >> >> Hence the tegra_image_format should be renamed to tegra210_image_format >> and moved out to t210.h, since it's not common. But then you'll probably >> need to rename all TEGRA_ defines to TEGRA210_ to make t210.h reusable >> by T186. > > We can't make t210.h reusable for t186 as all register defines are diff. > > only video format enums are same b/w them so to reuse that for t186 I > had that in common. > > Regarding defines, will change prefix as Tegra210 > >> >> Also, in the end it may not worth the effort to share anything at all, >> it could be cleaner to have a bit of duplication. Although, I have no >> idea how T186 code will look like and what other parts of T210 could be >> reused by T186. All this needs to be taken into account in order to >> avoid struggling with the code's reshuffling in the future. > > Currently as image formats are same for t210 and t186 I had them in > common.h and for t194 where they are diff new enums will be added. > > Other tegra210 soc specific only are all part of tegra210.c/h > Will move video formats to tegra specific file (tegra210.c)...
On 4/25/20 7:19 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 05:10, Dmitry Osipenko пишет: > ... >>> currently other Tegra host1x driver (drm) also does similar. Single >>> module for all Tegra SoCs. >> DRM driver has a proper separation of the sub-drivers where sub-driver >> won't load on unsupported hardware. The tegra-video driver should do the >> same, i.e. VI and CSI should be individual drivers (and not OPS). There >> could be a some common core, but for now it's not obvious to me what >> that core should be, maybe just the video.c. > Maybe video.c csi.c vi.c could be moved into a separate module, somewhat > like a common driver framework. Then the individual CSI/VI drivers will > use those common helpers.. Just a quick thought. structure of driver is based on prior feedback. How about instead of re-structuring whole driver again, probably we can use conditional compatibles and also corresponding tegra210.o in Makefile based on ARCH_TEGRA? #if defined(CONFIG_ARCH_TEGRA_210_SOC) { .compatible = "nvidia,tegra210-vi", .data = &tegra210_vi_soc }, #endif
26.04.2020 05:10, Dmitry Osipenko пишет: > 26.04.2020 04:43, Sowjanya Komatineni пишет: > ... >>> It looks to me that at least all those hardcoded HW format IDs do not >>> match the older SoCs. >> >> TPG hard coded formats are supported on prior Tegra. >> >> Other supported formats are SoC dependent and part of soc data in the >> driver already. > > But I don't see where that SoC-dependent definition is made in > terga210.c. That tegra_image_format enum looks T210-specific, isn't it? > > ... >>> The driver will need to have a bit better separation if it's supposed to >>> have a common core for all SoCs. Each incompatible VI/CSI hardware >>> version should have its own kernel module. >> >> currently other Tegra host1x driver (drm) also does similar. Single >> module for all Tegra SoCs. > > DRM driver has a proper separation of the sub-drivers where sub-driver > won't load on unsupported hardware. The tegra-video driver should do the > same, i.e. VI and CSI should be individual drivers (and not OPS). There > could be a some common core, but for now it's not obvious to me what > that core should be, maybe just the video.c. Although, you're right that tegra_drm is compiled as a single module. That's not good, I'm actually not sure now whether it is possible to modularize host1x drivers properly without changing the whole host1x bus.
On 4/25/20 9:42 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 05:10, Dmitry Osipenko пишет: >> 26.04.2020 04:43, Sowjanya Komatineni пишет: >> ... >>>> It looks to me that at least all those hardcoded HW format IDs do not >>>> match the older SoCs. >>> TPG hard coded formats are supported on prior Tegra. >>> >>> Other supported formats are SoC dependent and part of soc data in the >>> driver already. >> But I don't see where that SoC-dependent definition is made in >> terga210.c. That tegra_image_format enum looks T210-specific, isn't it? >> >> ... >>>> The driver will need to have a bit better separation if it's supposed to >>>> have a common core for all SoCs. Each incompatible VI/CSI hardware >>>> version should have its own kernel module. >>> currently other Tegra host1x driver (drm) also does similar. Single >>> module for all Tegra SoCs. >> DRM driver has a proper separation of the sub-drivers where sub-driver >> won't load on unsupported hardware. The tegra-video driver should do the >> same, i.e. VI and CSI should be individual drivers (and not OPS). There >> could be a some common core, but for now it's not obvious to me what >> that core should be, maybe just the video.c. > Although, you're right that tegra_drm is compiled as a single module. > That's not good, I'm actually not sure now whether it is possible to > modularize host1x drivers properly without changing the whole host1x bus. We already went thru driver structure changes during earlier versions and internal feedbacks. Not sure of restructuring again now. Also reg wastage of space, tegra specific implementation is not huge also for T186 and later we have RTCPU firmware that driver communicates with and it light weight. Based on internal discussion, no upstream support for prior Tegra210. So, probably what we have regarding structure is ok except video formats I will move to Tegra210 and change defines to use Tegra210 prefix.
26.04.2020 07:47, Sowjanya Komatineni пишет: ... >> Although, you're right that tegra_drm is compiled as a single module. >> That's not good, I'm actually not sure now whether it is possible to >> modularize host1x drivers properly without changing the whole host1x bus. > > We already went thru driver structure changes during earlier versions > and internal feedbacks. > > Not sure of restructuring again now. Also reg wastage of space, tegra > specific implementation is not huge also for T186 and later we have > RTCPU firmware that driver communicates with and it light weight. Based > on internal discussion, no upstream support for prior Tegra210. NVIDIA doesn't support older SoCs in downstream, but this doesn't apply to the upstream where anyone could write a driver. > So, probably what we have regarding structure is ok except video formats > I will move to Tegra210 and change defines to use Tegra210 prefix. Seems so.
26.04.2020 07:23, Sowjanya Komatineni пишет: > > On 4/25/20 7:19 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 26.04.2020 05:10, Dmitry Osipenko пишет: >> ... >>>> currently other Tegra host1x driver (drm) also does similar. Single >>>> module for all Tegra SoCs. >>> DRM driver has a proper separation of the sub-drivers where sub-driver >>> won't load on unsupported hardware. The tegra-video driver should do the >>> same, i.e. VI and CSI should be individual drivers (and not OPS). There >>> could be a some common core, but for now it's not obvious to me what >>> that core should be, maybe just the video.c. >> Maybe video.c csi.c vi.c could be moved into a separate module, somewhat >> like a common driver framework. Then the individual CSI/VI drivers will >> use those common helpers.. Just a quick thought. > > structure of driver is based on prior feedback. > > How about instead of re-structuring whole driver again, probably we can > use conditional compatibles and also corresponding tegra210.o in > Makefile based on ARCH_TEGRA? > > #if defined(CONFIG_ARCH_TEGRA_210_SOC) > { .compatible = "nvidia,tegra210-vi", .data = &tegra210_vi_soc }, > #endif Yes, allowing user to disable the unneeded code should be good.
26.04.2020 07:47, Sowjanya Komatineni пишет: > > So, probably what we have regarding structure is ok except video formats > I will move to Tegra210 and change defines to use Tegra210 prefix. If those defines aren't reusable by T186, then you could move them all to t210.c.
On 26/04/2020 02:19, Dmitry Osipenko wrote: > 26.04.2020 02:44, Sowjanya Komatineni пишет: > ... >>> How much of the T210 code could be reused by T186/194? >> >> vi/csi are common driver where soc structure should be populated for >> T186/T194 >> >> Tegra210.c can't be reused for Tegra186/t194 as programming seq is a >> whole lot diff >> > > How are you going to separate Tegra210/186/194 drivers from each other? > I don't think you'll want to have one "fat" driver that covers all those > SoCs, won't you? As long as the differences between SoCs are small, the media subsystem policy is to keep it all in one driver. You might split off some of it into separate SoC-specific sources that are included only if selected in the Kconfig (see e.g. drivers/staging/media/hantro/ or drivers/staging/media/imx/). If that makes sense for the Tegra, then that's a perfectly fine option. But creating multiple drivers for SoCs that only differ in relatively minor ways is not recommended. Also, these drivers allocate *huge* amounts of memory when streaming video, so a somewhat bigger driver is not something you'll notice. Keeping things readable, simple and maintainable is much more important. Regards, Hans > > In the end it should be three modules: tegra210-video.ko > tegra186-video.ko tegra194-video.ko. > > Using a per-SoC OPS doesn't allow you to do that because the "root" > driver will have to lookup OPS' code symbols of every SoC, and thus, the > unwanted driver modules will get auto-loaded if you'll try to factor out > the OPS into a separate driver modules. >
On 4/25/20 10:51 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 26.04.2020 07:47, Sowjanya Komatineni пишет: >> So, probably what we have regarding structure is ok except video formats >> I will move to Tegra210 and change defines to use Tegra210 prefix. > If those defines aren't reusable by T186, then you could move them all > to t210.c. ok, will combine defines and soc specific programming into tegra210.c Also will then get rid of common.h with video formats moving to tegra210