* [PATCH 01/17] media: atomisp: pci: add missing media_device_cleanup() in atomisp_unregister_entities()
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case Tsuchiya Yuto
` (20 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Aline Santana Cordeiro, Yang Yingliang, Alan,
Dinghao Liu, linux-media, linux-staging, linux-kernel
After the commit 9832e155f1ed ("[media] media-device: split media
initialization and registration"), calling media_device_cleanup()
is needed it seems. However, currently it is missing for the module
unload path.
Note that for the probe failure path, it is already added in
atomisp_register_entities().
This patch adds the missing call of media_device_cleanup() in
atomisp_unregister_entities().
Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 1e324f1f656e..0511c454e769 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1182,6 +1182,7 @@ static void atomisp_unregister_entities(struct atomisp_device *isp)
v4l2_device_unregister(&isp->v4l2_dev);
media_device_unregister(&isp->media_dev);
+ media_device_cleanup(&isp->media_dev);
}
static int atomisp_register_entities(struct atomisp_device *isp)
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 01/17] media: atomisp: pci: add missing media_device_cleanup() in atomisp_unregister_entities() Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-18 11:07 ` Andy Shevchenko
2021-11-02 11:26 ` Dan Carpenter
2021-10-17 16:19 ` [PATCH 03/17] media: atomisp: pci: fix inverted logic in buffers_needed() Tsuchiya Yuto
` (19 subsequent siblings)
21 siblings, 2 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Aline Santana Cordeiro, Yang Yingliang,
Dinghao Liu, Alan, linux-media, linux-staging, linux-kernel
When comparing with intel-aero atomisp [1], it looks like
punit_ddr_dvfs_enable() should take `false` as an argument on mrfld_power
up case.
Code from the intel-aero kernel [1]:
int atomisp_mrfld_power_down(struct atomisp_device *isp)
{
[...]
/*WA:Enable DVFS*/
if (IS_CHT)
punit_ddr_dvfs_enable(true);
int atomisp_mrfld_power_up(struct atomisp_device *isp)
{
[...]
/*WA for PUNIT, if DVFS enabled, ISP timeout observed*/
if (IS_CHT)
punit_ddr_dvfs_enable(false);
This patch fixes the inverted argument as per the intel-aero code, as
well as its comment. While here, fix space issues for comments in
atomisp_mrfld_power().
Note that it does not seem to be possible to unify the up/down cases for
punit_ddr_dvfs_enable(), i.e., we can't do something like the following:
if (IS_CHT)
punit_ddr_dvfs_enable(!enable);
because according to the intel-aero code [1], the DVFS is disabled
before "writing 0x0 to ISPSSPM0 bit[1:0]" and the DVFS is enabled after
"writing 0x3 to ISPSSPM0 bit[1:0]".
[1] https://github.com/intel-aero/linux-kernel/blob/a1b673258feb915268377275130c5c5df0eafc82/drivers/media/pci/atomisp/atomisp_driver/atomisp_v4l2.c#L431-L514
Fixes: 0f441fd70b1e ("media: atomisp: simplify the power down/up code")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 0511c454e769..f5362554638e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
- /*WA:Enable DVFS*/
+ /* WA for PUNIT, if DVFS enabled, ISP timeout observed */
if (IS_CHT && enable)
- punit_ddr_dvfs_enable(true);
+ punit_ddr_dvfs_enable(false);
/*
* FIXME:WA for ECS28A, with this sleep, CTS
* android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
* PASS, no impact on other platforms
- */
+ */
if (IS_BYT && enable)
msleep(10);
@@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
val, MRFLD_ISPSSPM0_ISPSSC_MASK);
- /*WA:Enable DVFS*/
+ /* WA:Enable DVFS */
if (IS_CHT && !enable)
punit_ddr_dvfs_enable(true);
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case
2021-10-17 16:19 ` [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case Tsuchiya Yuto
@ 2021-10-18 11:07 ` Andy Shevchenko
2021-10-20 13:25 ` Tsuchiya Yuto
2021-11-02 11:26 ` Dan Carpenter
1 sibling, 1 reply; 79+ messages in thread
From: Andy Shevchenko @ 2021-10-18 11:07 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu, Alan,
linux-media, linux-staging, linux-kernel
On Mon, Oct 18, 2021 at 01:19:42AM +0900, Tsuchiya Yuto wrote:
> When comparing with intel-aero atomisp [1], it looks like
> punit_ddr_dvfs_enable() should take `false` as an argument on mrfld_power
> up case.
>
> Code from the intel-aero kernel [1]:
>
> int atomisp_mrfld_power_down(struct atomisp_device *isp)
> {
> [...]
> /*WA:Enable DVFS*/
> if (IS_CHT)
> punit_ddr_dvfs_enable(true);
>
> int atomisp_mrfld_power_up(struct atomisp_device *isp)
> {
> [...]
> /*WA for PUNIT, if DVFS enabled, ISP timeout observed*/
> if (IS_CHT)
> punit_ddr_dvfs_enable(false);
>
> This patch fixes the inverted argument as per the intel-aero code, as
> well as its comment. While here, fix space issues for comments in
> atomisp_mrfld_power().
>
> Note that it does not seem to be possible to unify the up/down cases for
> punit_ddr_dvfs_enable(), i.e., we can't do something like the following:
>
> if (IS_CHT)
> punit_ddr_dvfs_enable(!enable);
>
> because according to the intel-aero code [1], the DVFS is disabled
> before "writing 0x0 to ISPSSPM0 bit[1:0]" and the DVFS is enabled after
> "writing 0x3 to ISPSSPM0 bit[1:0]".
>
> [1] https://github.com/intel-aero/linux-kernel/blob/a1b673258feb915268377275130c5c5df0eafc82/drivers/media/pci/atomisp/atomisp_driver/atomisp_v4l2.c#L431-L514
>
> Fixes: 0f441fd70b1e ("media: atomisp: simplify the power down/up code")
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> ---
> drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index 0511c454e769..f5362554638e 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
>
> dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
>
> - /*WA:Enable DVFS*/
> + /* WA for PUNIT, if DVFS enabled, ISP timeout observed */
P-Unit
> if (IS_CHT && enable)
> - punit_ddr_dvfs_enable(true);
> + punit_ddr_dvfs_enable(false);
>
> /*
> * FIXME:WA for ECS28A, with this sleep, CTS
> * android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
> * PASS, no impact on other platforms
> - */
> + */
> if (IS_BYT && enable)
> msleep(10);
>
> @@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
> val, MRFLD_ISPSSPM0_ISPSSC_MASK);
>
> - /*WA:Enable DVFS*/
> + /* WA:Enable DVFS */
> if (IS_CHT && !enable)
> punit_ddr_dvfs_enable(true);
>
> --
> 2.33.1
>
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case
2021-10-18 11:07 ` Andy Shevchenko
@ 2021-10-20 13:25 ` Tsuchiya Yuto
2021-12-01 12:07 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-20 13:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu, Alan,
linux-media, linux-staging, linux-kernel
On Mon, 2021-10-18 at 14:07 +0300, Andy Shevchenko wrote:
> On Mon, Oct 18, 2021 at 01:19:42AM +0900, Tsuchiya Yuto wrote:
> > When comparing with intel-aero atomisp [1], it looks like
> > punit_ddr_dvfs_enable() should take `false` as an argument on mrfld_power
> > up case.
> >
> > Code from the intel-aero kernel [1]:
> >
> > int atomisp_mrfld_power_down(struct atomisp_device *isp)
> > {
> > [...]
> > /*WA:Enable DVFS*/
> > if (IS_CHT)
> > punit_ddr_dvfs_enable(true);
> >
> > int atomisp_mrfld_power_up(struct atomisp_device *isp)
> > {
> > [...]
> > /*WA for PUNIT, if DVFS enabled, ISP timeout observed*/
> > if (IS_CHT)
> > punit_ddr_dvfs_enable(false);
> >
> > This patch fixes the inverted argument as per the intel-aero code, as
> > well as its comment. While here, fix space issues for comments in
> > atomisp_mrfld_power().
> >
> > Note that it does not seem to be possible to unify the up/down cases for
> > punit_ddr_dvfs_enable(), i.e., we can't do something like the following:
> >
> > if (IS_CHT)
> > punit_ddr_dvfs_enable(!enable);
> >
> > because according to the intel-aero code [1], the DVFS is disabled
> > before "writing 0x0 to ISPSSPM0 bit[1:0]" and the DVFS is enabled after
> > "writing 0x3 to ISPSSPM0 bit[1:0]".
> >
> > [1] https://github.com/intel-aero/linux-kernel/blob/a1b673258feb915268377275130c5c5df0eafc82/drivers/media/pci/atomisp/atomisp_driver/atomisp_v4l2.c#L431-L514
> >
> > Fixes: 0f441fd70b1e ("media: atomisp: simplify the power down/up code")
> > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > ---
> > drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > index 0511c454e769..f5362554638e 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > @@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> >
> > dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
> >
> > - /*WA:Enable DVFS*/
> > + /* WA for PUNIT, if DVFS enabled, ISP timeout observed */
>
> P-Unit
Thanks, I'll fix this next time I send.
> > if (IS_CHT && enable)
> > - punit_ddr_dvfs_enable(true);
> > + punit_ddr_dvfs_enable(false);
> >
> > /*
> > * FIXME:WA for ECS28A, with this sleep, CTS
> > * android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
> > * PASS, no impact on other platforms
> > - */
> > + */
> > if (IS_BYT && enable)
> > msleep(10);
> >
> > @@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
> > val, MRFLD_ISPSSPM0_ISPSSC_MASK);
> >
> > - /*WA:Enable DVFS*/
> > + /* WA:Enable DVFS */
> > if (IS_CHT && !enable)
> > punit_ddr_dvfs_enable(true);
> >
> > --
> > 2.33.1
> >
> >
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case
2021-10-20 13:25 ` Tsuchiya Yuto
@ 2021-12-01 12:07 ` Tsuchiya Yuto
0 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-12-01 12:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu, Alan,
linux-media, linux-staging, linux-kernel
On Wed, 2021-10-20 at 22:25 +0900, Tsuchiya Yuto wrote:
> On Mon, 2021-10-18 at 14:07 +0300, Andy Shevchenko wrote:
> > On Mon, Oct 18, 2021 at 01:19:42AM +0900, Tsuchiya Yuto wrote:
> > > When comparing with intel-aero atomisp [1], it looks like
> > > punit_ddr_dvfs_enable() should take `false` as an argument on mrfld_power
> > > up case.
> > >
> > > Code from the intel-aero kernel [1]:
> > >
> > > int atomisp_mrfld_power_down(struct atomisp_device *isp)
> > > {
> > > [...]
> > > /*WA:Enable DVFS*/
> > > if (IS_CHT)
> > > punit_ddr_dvfs_enable(true);
> > >
> > > int atomisp_mrfld_power_up(struct atomisp_device *isp)
> > > {
> > > [...]
> > > /*WA for PUNIT, if DVFS enabled, ISP timeout observed*/
> > > if (IS_CHT)
> > > punit_ddr_dvfs_enable(false);
> > >
> > > This patch fixes the inverted argument as per the intel-aero code, as
> > > well as its comment. While here, fix space issues for comments in
> > > atomisp_mrfld_power().
> > >
> > > Note that it does not seem to be possible to unify the up/down cases for
> > > punit_ddr_dvfs_enable(), i.e., we can't do something like the following:
> > >
> > > if (IS_CHT)
> > > punit_ddr_dvfs_enable(!enable);
> > >
> > > because according to the intel-aero code [1], the DVFS is disabled
> > > before "writing 0x0 to ISPSSPM0 bit[1:0]" and the DVFS is enabled after
> > > "writing 0x3 to ISPSSPM0 bit[1:0]".
> > >
> > > [1] https://github.com/intel-aero/linux-kernel/blob/a1b673258feb915268377275130c5c5df0eafc82/drivers/media/pci/atomisp/atomisp_driver/atomisp_v4l2.c#L431-L514
> > >
> > > Fixes: 0f441fd70b1e ("media: atomisp: simplify the power down/up code")
> > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > ---
> > > drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > index 0511c454e769..f5362554638e 100644
> > > --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > @@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > >
> > > dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
> > >
> > > - /*WA:Enable DVFS*/
> > > + /* WA for PUNIT, if DVFS enabled, ISP timeout observed */
> >
> > P-Unit
>
> Thanks, I'll fix this next time I send.
For the record, this is already fixed in the latest media_stage tree.
> > > if (IS_CHT && enable)
> > > - punit_ddr_dvfs_enable(true);
> > > + punit_ddr_dvfs_enable(false);
> > >
> > > /*
> > > * FIXME:WA for ECS28A, with this sleep, CTS
> > > * android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
> > > * PASS, no impact on other platforms
> > > - */
> > > + */
> > > if (IS_BYT && enable)
> > > msleep(10);
> > >
> > > @@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > > iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
> > > val, MRFLD_ISPSSPM0_ISPSSC_MASK);
> > >
> > > - /*WA:Enable DVFS*/
> > > + /* WA:Enable DVFS */
> > > if (IS_CHT && !enable)
> > > punit_ddr_dvfs_enable(true);
> > >
> > > --
> > > 2.33.1
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case
2021-10-17 16:19 ` [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case Tsuchiya Yuto
2021-10-18 11:07 ` Andy Shevchenko
@ 2021-11-02 11:26 ` Dan Carpenter
2021-11-08 14:48 ` Tsuchiya Yuto
1 sibling, 1 reply; 79+ messages in thread
From: Dan Carpenter @ 2021-11-02 11:26 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu, Alan,
linux-media, linux-staging, linux-kernel
On Mon, Oct 18, 2021 at 01:19:42AM +0900, Tsuchiya Yuto wrote:
> When comparing with intel-aero atomisp [1], it looks like
> punit_ddr_dvfs_enable() should take `false` as an argument on mrfld_power
> up case.
>
> Code from the intel-aero kernel [1]:
>
> int atomisp_mrfld_power_down(struct atomisp_device *isp)
> {
> [...]
> /*WA:Enable DVFS*/
> if (IS_CHT)
> punit_ddr_dvfs_enable(true);
>
> int atomisp_mrfld_power_up(struct atomisp_device *isp)
> {
> [...]
> /*WA for PUNIT, if DVFS enabled, ISP timeout observed*/
> if (IS_CHT)
> punit_ddr_dvfs_enable(false);
>
> This patch fixes the inverted argument as per the intel-aero code, as
> well as its comment. While here, fix space issues for comments in
> atomisp_mrfld_power().
>
> Note that it does not seem to be possible to unify the up/down cases for
> punit_ddr_dvfs_enable(), i.e., we can't do something like the following:
>
> if (IS_CHT)
> punit_ddr_dvfs_enable(!enable);
>
> because according to the intel-aero code [1], the DVFS is disabled
> before "writing 0x0 to ISPSSPM0 bit[1:0]" and the DVFS is enabled after
> "writing 0x3 to ISPSSPM0 bit[1:0]".
>
> [1] https://github.com/intel-aero/linux-kernel/blob/a1b673258feb915268377275130c5c5df0eafc82/drivers/media/pci/atomisp/atomisp_driver/atomisp_v4l2.c#L431-L514
>
> Fixes: 0f441fd70b1e ("media: atomisp: simplify the power down/up code")
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> ---
> drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index 0511c454e769..f5362554638e 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
>
> dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
>
> - /*WA:Enable DVFS*/
> + /* WA for PUNIT, if DVFS enabled, ISP timeout observed */
> if (IS_CHT && enable)
> - punit_ddr_dvfs_enable(true);
> + punit_ddr_dvfs_enable(false);
>
> /*
> * FIXME:WA for ECS28A, with this sleep, CTS
> * android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
> * PASS, no impact on other platforms
> - */
> + */
^^^
> if (IS_BYT && enable)
> msleep(10);
>
> @@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
> val, MRFLD_ISPSSPM0_ISPSSC_MASK);
>
> - /*WA:Enable DVFS*/
> + /* WA:Enable DVFS */
^^^^^^^^^^^^^^^^^^^
These two white space changes are unrelated. Please do them in a
separate patch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case
2021-11-02 11:26 ` Dan Carpenter
@ 2021-11-08 14:48 ` Tsuchiya Yuto
2021-11-08 15:10 ` Dan Carpenter
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-11-08 14:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu, Alan,
linux-media, linux-staging, linux-kernel
On Tue, 2021-11-02 at 14:26 +0300, Dan Carpenter wrote:
> On Mon, Oct 18, 2021 at 01:19:42AM +0900, Tsuchiya Yuto wrote:
> > When comparing with intel-aero atomisp [1], it looks like
> > punit_ddr_dvfs_enable() should take `false` as an argument on mrfld_power
> > up case.
> >
> > Code from the intel-aero kernel [1]:
> >
> > int atomisp_mrfld_power_down(struct atomisp_device *isp)
> > {
> > [...]
> > /*WA:Enable DVFS*/
> > if (IS_CHT)
> > punit_ddr_dvfs_enable(true);
> >
> > int atomisp_mrfld_power_up(struct atomisp_device *isp)
> > {
> > [...]
> > /*WA for PUNIT, if DVFS enabled, ISP timeout observed*/
> > if (IS_CHT)
> > punit_ddr_dvfs_enable(false);
> >
> > This patch fixes the inverted argument as per the intel-aero code, as
> > well as its comment. While here, fix space issues for comments in
> > atomisp_mrfld_power().
> >
> > Note that it does not seem to be possible to unify the up/down cases for
> > punit_ddr_dvfs_enable(), i.e., we can't do something like the following:
> >
> > if (IS_CHT)
> > punit_ddr_dvfs_enable(!enable);
> >
> > because according to the intel-aero code [1], the DVFS is disabled
> > before "writing 0x0 to ISPSSPM0 bit[1:0]" and the DVFS is enabled after
> > "writing 0x3 to ISPSSPM0 bit[1:0]".
> >
> > [1] https://github.com/intel-aero/linux-kernel/blob/a1b673258feb915268377275130c5c5df0eafc82/drivers/media/pci/atomisp/atomisp_driver/atomisp_v4l2.c#L431-L514
> >
> > Fixes: 0f441fd70b1e ("media: atomisp: simplify the power down/up code")
> > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > ---
> > drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > index 0511c454e769..f5362554638e 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > @@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> >
> > dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
> >
> > - /*WA:Enable DVFS*/
> > + /* WA for PUNIT, if DVFS enabled, ISP timeout observed */
> > if (IS_CHT && enable)
> > - punit_ddr_dvfs_enable(true);
> > + punit_ddr_dvfs_enable(false);
> >
> > /*
> > * FIXME:WA for ECS28A, with this sleep, CTS
> > * android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
> > * PASS, no impact on other platforms
> > - */
> > + */
> ^^^
>
> > if (IS_BYT && enable)
> > msleep(10);
> >
> > @@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
> > val, MRFLD_ISPSSPM0_ISPSSC_MASK);
> >
> > - /*WA:Enable DVFS*/
> > + /* WA:Enable DVFS */
> ^^^^^^^^^^^^^^^^^^^
> These two white space changes are unrelated. Please do them in a
> separate patch.
Thank you for the review :-)
I thought these space fixes were too trivial for a separate patch, so
I made the fix while at it. I have no strong reason not to separate the
space fix. I'll do so in v2.
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case
2021-11-08 14:48 ` Tsuchiya Yuto
@ 2021-11-08 15:10 ` Dan Carpenter
0 siblings, 0 replies; 79+ messages in thread
From: Dan Carpenter @ 2021-11-08 15:10 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu, Alan,
linux-media, linux-staging, linux-kernel
On Mon, Nov 08, 2021 at 11:48:42PM +0900, Tsuchiya Yuto wrote:
> > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > index 0511c454e769..f5362554638e 100644
> > > --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > > @@ -711,15 +711,15 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > >
> > > dev_dbg(isp->dev, "IUNIT power-%s.\n", enable ? "on" : "off");
> > >
> > > - /*WA:Enable DVFS*/
> > > + /* WA for PUNIT, if DVFS enabled, ISP timeout observed */
> > > if (IS_CHT && enable)
> > > - punit_ddr_dvfs_enable(true);
> > > + punit_ddr_dvfs_enable(false);
> > >
> > > /*
> > > * FIXME:WA for ECS28A, with this sleep, CTS
> > > * android.hardware.camera2.cts.CameraDeviceTest#testCameraDeviceAbort
> > > * PASS, no impact on other platforms
> > > - */
> > > + */
> > ^^^
> >
> > > if (IS_BYT && enable)
> > > msleep(10);
> > >
> > > @@ -727,7 +727,7 @@ static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
> > > iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, MRFLD_ISPSSPM0,
> > > val, MRFLD_ISPSSPM0_ISPSSC_MASK);
> > >
> > > - /*WA:Enable DVFS*/
> > > + /* WA:Enable DVFS */
> > ^^^^^^^^^^^^^^^^^^^
> > These two white space changes are unrelated. Please do them in a
> > separate patch.
>
> Thank you for the review :-)
>
> I thought these space fixes were too trivial for a separate patch, so
> I made the fix while at it. I have no strong reason not to separate the
> space fix. I'll do so in v2.
Trivial white space patches are fine. We get thousands of those.
This goes through Mauro's tree instead of Greg's and he is definitely
more flexible than Greg. Plus the atomisp stuff is broken so no one
cares.
But the rules are really clear and it does help in reviewing when people
follow them.
When I'm reviewing this patch, the patch description says it is a fix so
I review that differently. The fix is very simple and it changes true
to false.
The other patch would have been a "change comments" patch. We get
thousands of those as I mentioned. My concern there is that a UMN
researcher will try to trick us by hiding code in a "change comments"
patch so I have a script to review those. It takes one second to run.
But then when I was reviewing this patch I first had to spot the change
from true to false before I could review it. That's where most of the
time was wasted.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH 03/17] media: atomisp: pci: fix inverted logic in buffers_needed()
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 01/17] media: atomisp: pci: add missing media_device_cleanup() in atomisp_unregister_entities() Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 02/17] media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for mrfld_power up case Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 04/17] media: atomisp: pci: do not use err var when checking port validity for ISP2400 Tsuchiya Yuto
` (18 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Aline Santana Cordeiro, Yang Yingliang,
Dinghao Liu, Alan, Deepak R Varma, Alex Dewar, linux-media,
linux-staging, linux-kernel
When config.mode is IA_CSS_INPUT_MODE_BUFFERED_SENSOR, it rather needs
buffers. Fix it by inverting the return value.
Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
drivers/staging/media/atomisp/pci/sh_css_mipi.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index 75489f7d75ee..483d40a467c7 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -374,17 +374,17 @@ static bool buffers_needed(struct ia_css_pipe *pipe)
{
if (!IS_ISP2401) {
if (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR)
- return false;
- else
return true;
+ else
+ return false;
}
if (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR ||
pipe->stream->config.mode == IA_CSS_INPUT_MODE_TPG ||
pipe->stream->config.mode == IA_CSS_INPUT_MODE_PRBS)
- return false;
+ return true;
- return true;
+ return false;
}
int
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 04/17] media: atomisp: pci: do not use err var when checking port validity for ISP2400
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (2 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 03/17] media: atomisp: pci: fix inverted logic in buffers_needed() Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
[not found] ` <20211026092637.196447aa@sal.lan>
2021-10-17 16:19 ` [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid() Tsuchiya Yuto
` (17 subsequent siblings)
21 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Yang Yingliang, Hans Verkuil, Aline Santana Cordeiro, Alan,
Dinghao Liu, Deepak R Varma, Alex Dewar, linux-media,
linux-staging, linux-kernel
Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
evaluated as true because the err variable is set to `-EINVAL` on
declaration but the variable is never used until the evaluation.
Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
for ISP2400 and the err variable check is for ISP2401. Fix this issue
by adding ISP version test there accordingly.
Yes, there are other better ways to fix this issue, like adding support
for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
unify the following test:
if (!IS_ISP2401)
port = (unsigned int)pipe->stream->config.source.port.port;
else
err = ia_css_mipi_is_source_port_valid(pipe, &port);
However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
a result of real hardware difference, but just a result of the following
two different versions of driver merged by tools [1]:
- ISP2400: irci_stable_candrpv_0415_20150521_0458
- ISP2401: irci_ecr-master_20150911_0724
We should eventually remove (not unify) such tests caused by just a
driver version difference and use just one version of driver. So, for
now, let's avoid further unification.
[1] The function ia_css_mipi_is_source_port_valid() and its usage is
added on updating css version to irci_master_20150701_0213
https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
drivers/staging/media/atomisp/pci/sh_css_mipi.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index 483d40a467c7..65fc93c5d56b 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -430,7 +430,8 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
assert(port < N_CSI_PORTS);
- if (port >= N_CSI_PORTS || err) {
+ if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
+ (IS_ISP2401 && err)) {
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
"allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
pipe, port);
@@ -559,7 +560,8 @@ free_mipi_frames(struct ia_css_pipe *pipe)
assert(port < N_CSI_PORTS);
- if (port >= N_CSI_PORTS || err) {
+ if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
+ (IS_ISP2401 && err)) {
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
"free_mipi_frames(%p, %d) exit: error: pipe port is not correct.\n",
pipe, port);
@@ -670,7 +672,8 @@ send_mipi_frames(struct ia_css_pipe *pipe)
assert(port < N_CSI_PORTS);
- if (port >= N_CSI_PORTS || err) {
+ if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
+ (IS_ISP2401 && err)) {
IA_CSS_ERROR("send_mipi_frames(%p) exit: invalid port specified (port=%d).\n",
pipe, port);
return err;
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (3 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 04/17] media: atomisp: pci: do not use err var when checking port validity for ISP2400 Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-11-02 11:33 ` Dan Carpenter
2021-10-17 16:19 ` [PATCH 06/17] media: atomisp: pci: use IA_CSS_ERROR() for error messages in sh_css_mipi.c Tsuchiya Yuto
` (16 subsequent siblings)
21 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Aline Santana Cordeiro, Yang Yingliang, Alan,
Dinghao Liu, Deepak R Varma, Alex Dewar, linux-media,
linux-staging, linux-kernel
The function ia_css_mipi_is_source_port_valid() returns true if the port
is valid. So, we can't use the existing err variable as is.
To fix this issue while reusing that variable, invert the return value
when assigning it to the variable.
Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../staging/media/atomisp/pci/sh_css_mipi.c | 24 ++++++++++++-------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index 65fc93c5d56b..c1f2f6151c5f 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
return 0; /* AM TODO: Check */
}
- if (!IS_ISP2401)
+ if (!IS_ISP2401) {
port = (unsigned int)pipe->stream->config.source.port.port;
- else
- err = ia_css_mipi_is_source_port_valid(pipe, &port);
+ } else {
+ /* Returns true if port is valid. So, invert it */
+ err = !ia_css_mipi_is_source_port_valid(pipe, &port);
+ }
assert(port < N_CSI_PORTS);
@@ -553,10 +555,12 @@ free_mipi_frames(struct ia_css_pipe *pipe)
return err;
}
- if (!IS_ISP2401)
+ if (!IS_ISP2401) {
port = (unsigned int)pipe->stream->config.source.port.port;
- else
- err = ia_css_mipi_is_source_port_valid(pipe, &port);
+ } else {
+ /* Returns true if port is valid. So, invert it */
+ err = !ia_css_mipi_is_source_port_valid(pipe, &port);
+ }
assert(port < N_CSI_PORTS);
@@ -665,10 +669,12 @@ send_mipi_frames(struct ia_css_pipe *pipe)
/* TODO: AM: maybe this should be returning an error. */
}
- if (!IS_ISP2401)
+ if (!IS_ISP2401) {
port = (unsigned int)pipe->stream->config.source.port.port;
- else
- err = ia_css_mipi_is_source_port_valid(pipe, &port);
+ } else {
+ /* Returns true if port is valid. So, invert it */
+ err = !ia_css_mipi_is_source_port_valid(pipe, &port);
+ }
assert(port < N_CSI_PORTS);
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-10-17 16:19 ` [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid() Tsuchiya Yuto
@ 2021-11-02 11:33 ` Dan Carpenter
2021-11-08 15:00 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Dan Carpenter @ 2021-11-02 11:33 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Alan, Dinghao Liu,
Deepak R Varma, Alex Dewar, linux-media, linux-staging,
linux-kernel
On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> The function ia_css_mipi_is_source_port_valid() returns true if the port
> is valid. So, we can't use the existing err variable as is.
>
> To fix this issue while reusing that variable, invert the return value
> when assigning it to the variable.
>
> Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> ---
> .../staging/media/atomisp/pci/sh_css_mipi.c | 24 ++++++++++++-------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> index 65fc93c5d56b..c1f2f6151c5f 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> return 0; /* AM TODO: Check */
> }
>
> - if (!IS_ISP2401)
> + if (!IS_ISP2401) {
> port = (unsigned int)pipe->stream->config.source.port.port;
> - else
> - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> + } else {
> + /* Returns true if port is valid. So, invert it */
> + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
Don't invert it... This isn't supposed to return 1 on failure it's
supposed to return negative error codes.
> + }
>
> assert(port < N_CSI_PORTS);
>
> @@ -553,10 +555,12 @@ free_mipi_frames(struct ia_css_pipe *pipe)
> return err;
> }
>
> - if (!IS_ISP2401)
> + if (!IS_ISP2401) {
> port = (unsigned int)pipe->stream->config.source.port.port;
> - else
> - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> + } else {
> + /* Returns true if port is valid. So, invert it */
> + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
Presumably the same here?
> + }
>
> assert(port < N_CSI_PORTS);
>
> @@ -665,10 +669,12 @@ send_mipi_frames(struct ia_css_pipe *pipe)
> /* TODO: AM: maybe this should be returning an error. */
> }
>
> - if (!IS_ISP2401)
> + if (!IS_ISP2401) {
> port = (unsigned int)pipe->stream->config.source.port.port;
> - else
> - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> + } else {
> + /* Returns true if port is valid. So, invert it */
> + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
Same?
> + }
>
> assert(port < N_CSI_PORTS);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-11-02 11:33 ` Dan Carpenter
@ 2021-11-08 15:00 ` Tsuchiya Yuto
2021-11-08 15:14 ` Dan Carpenter
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-11-08 15:00 UTC (permalink / raw)
To: Dan Carpenter
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Alan, Dinghao Liu,
Deepak R Varma, Alex Dewar, linux-media, linux-staging,
linux-kernel
On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > is valid. So, we can't use the existing err variable as is.
> >
> > To fix this issue while reusing that variable, invert the return value
> > when assigning it to the variable.
> >
> > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > ---
> > .../staging/media/atomisp/pci/sh_css_mipi.c | 24 ++++++++++++-------
> > 1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > index 65fc93c5d56b..c1f2f6151c5f 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > return 0; /* AM TODO: Check */
> > }
> >
> > - if (!IS_ISP2401)
> > + if (!IS_ISP2401) {
> > port = (unsigned int)pipe->stream->config.source.port.port;
> > - else
> > - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > + } else {
> > + /* Returns true if port is valid. So, invert it */
> > + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
>
> Don't invert it... This isn't supposed to return 1 on failure it's
> supposed to return negative error codes.
You mean I should instead modify the return value of
ia_css_mipi_is_source_port_valid() ?
Yeah, I also thought that the current true/false return value was a little
bit confusing.
In another words, should the function return:
- negative values (maybe -EINVAL for this case) for invalid case
- 0 for valid case
instead? and if we go this way, we should also rename the function name
like
- check_ia_css_mipi_source_port_validity
or something. How does this sound?
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-11-08 15:00 ` Tsuchiya Yuto
@ 2021-11-08 15:14 ` Dan Carpenter
2021-11-08 15:25 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Dan Carpenter @ 2021-11-08 15:14 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Alan, Dinghao Liu,
Deepak R Varma, Alex Dewar, linux-media, linux-staging,
linux-kernel
On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:
> On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > is valid. So, we can't use the existing err variable as is.
> > >
> > > To fix this issue while reusing that variable, invert the return value
> > > when assigning it to the variable.
> > >
> > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > ---
> > > .../staging/media/atomisp/pci/sh_css_mipi.c | 24 ++++++++++++-------
> > > 1 file changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > return 0; /* AM TODO: Check */
> > > }
> > >
> > > - if (!IS_ISP2401)
> > > + if (!IS_ISP2401) {
> > > port = (unsigned int)pipe->stream->config.source.port.port;
> > > - else
> > > - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > + } else {
> > > + /* Returns true if port is valid. So, invert it */
> > > + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
> >
> > Don't invert it... This isn't supposed to return 1 on failure it's
> > supposed to return negative error codes.
>
> You mean I should instead modify the return value of
> ia_css_mipi_is_source_port_valid() ?
>
No. ia_css_mipi_is_source_port_valid() is fine. It has a boolean name
so returning bool is fine. What I'm saying is that allocate_mipi_frames()
should do:
if (!ia_css_mipi_is_source_port_valid(pipe, &port))
err = -EINVAL;
Otherwise it returns negative error codes and 1 on failure.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-11-08 15:14 ` Dan Carpenter
@ 2021-11-08 15:25 ` Tsuchiya Yuto
2021-11-08 15:33 ` Dan Carpenter
2021-11-08 16:35 ` Mauro Carvalho Chehab
0 siblings, 2 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-11-08 15:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu,
Deepak R Varma, Alex Dewar, linux-media, linux-staging,
linux-kernel
<removed Alan from Cc as the mail address not reachable>
On Mon, 2021-11-08 at 18:14 +0300, Dan Carpenter wrote:
> On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:
> > On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> > > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > > is valid. So, we can't use the existing err variable as is.
> > > >
> > > > To fix this issue while reusing that variable, invert the return value
> > > > when assigning it to the variable.
> > > >
> > > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > > ---
> > > > .../staging/media/atomisp/pci/sh_css_mipi.c | 24 ++++++++++++-------
> > > > 1 file changed, 15 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > > return 0; /* AM TODO: Check */
> > > > }
> > > >
> > > > - if (!IS_ISP2401)
> > > > + if (!IS_ISP2401) {
> > > > port = (unsigned int)pipe->stream->config.source.port.port;
> > > > - else
> > > > - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > + } else {
> > > > + /* Returns true if port is valid. So, invert it */
> > > > + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
> > >
> > > Don't invert it... This isn't supposed to return 1 on failure it's
> > > supposed to return negative error codes.
> >
> > You mean I should instead modify the return value of
> > ia_css_mipi_is_source_port_valid() ?
> >
>
> No. ia_css_mipi_is_source_port_valid() is fine. It has a boolean name
> so returning bool is fine. What I'm saying is that allocate_mipi_frames()
> should do:
>
> if (!ia_css_mipi_is_source_port_valid(pipe, &port))
> err = -EINVAL;
>
> Otherwise it returns negative error codes and 1 on failure.
Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-11-08 15:25 ` Tsuchiya Yuto
@ 2021-11-08 15:33 ` Dan Carpenter
2021-11-08 16:35 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 79+ messages in thread
From: Dan Carpenter @ 2021-11-08 15:33 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Hans Verkuil,
Aline Santana Cordeiro, Yang Yingliang, Dinghao Liu,
Deepak R Varma, Alex Dewar, linux-media, linux-staging,
linux-kernel
On Tue, Nov 09, 2021 at 12:25:52AM +0900, Tsuchiya Yuto wrote:
>
> Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.
>
Not at all.
It's easy for me to not introduce bugs because I never write new code.
I just sit here all day obsessing about error handling.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-11-08 15:25 ` Tsuchiya Yuto
2021-11-08 15:33 ` Dan Carpenter
@ 2021-11-08 16:35 ` Mauro Carvalho Chehab
2021-12-01 12:15 ` Tsuchiya Yuto
1 sibling, 1 reply; 79+ messages in thread
From: Mauro Carvalho Chehab @ 2021-11-08 16:35 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Dan Carpenter, Hans de Goede, Patrik Gfeller, Sakari Ailus,
Greg Kroah-Hartman, Hans Verkuil, Aline Santana Cordeiro,
Yang Yingliang, Dinghao Liu, Deepak R Varma, Alex Dewar,
linux-media, linux-staging, linux-kernel
Em Tue, 09 Nov 2021 00:25:52 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> <removed Alan from Cc as the mail address not reachable>
>
> On Mon, 2021-11-08 at 18:14 +0300, Dan Carpenter wrote:
> > On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:
> > > On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> > > > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > > > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > > > is valid. So, we can't use the existing err variable as is.
> > > > >
> > > > > To fix this issue while reusing that variable, invert the return value
> > > > > when assigning it to the variable.
> > > > >
> > > > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > > > ---
> > > > > .../staging/media/atomisp/pci/sh_css_mipi.c | 24 ++++++++++++-------
> > > > > 1 file changed, 15 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > > > return 0; /* AM TODO: Check */
> > > > > }
> > > > >
> > > > > - if (!IS_ISP2401)
> > > > > + if (!IS_ISP2401) {
> > > > > port = (unsigned int)pipe->stream->config.source.port.port;
> > > > > - else
> > > > > - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > > + } else {
> > > > > + /* Returns true if port is valid. So, invert it */
> > > > > + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
> > > >
> > > > Don't invert it... This isn't supposed to return 1 on failure it's
> > > > supposed to return negative error codes.
> > >
> > > You mean I should instead modify the return value of
> > > ia_css_mipi_is_source_port_valid() ?
> > >
> >
> > No. ia_css_mipi_is_source_port_valid() is fine. It has a boolean name
> > so returning bool is fine. What I'm saying is that allocate_mipi_frames()
> > should do:
> >
> > if (!ia_css_mipi_is_source_port_valid(pipe, &port))
> > err = -EINVAL;
> >
> > Otherwise it returns negative error codes and 1 on failure.
>
> Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.
I would prefer if you could send such changes on new patches.
Regards,
Mauro
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid()
2021-11-08 16:35 ` Mauro Carvalho Chehab
@ 2021-12-01 12:15 ` Tsuchiya Yuto
0 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-12-01 12:15 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Dan Carpenter, Hans de Goede, Patrik Gfeller, Sakari Ailus,
Greg Kroah-Hartman, Hans Verkuil, Aline Santana Cordeiro,
Yang Yingliang, Dinghao Liu, Deepak R Varma, Alex Dewar,
linux-media, linux-staging, linux-kernel
On Mon, 2021-11-08 at 16:35 +0000, Mauro Carvalho Chehab wrote:
> Em Tue, 09 Nov 2021 00:25:52 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
>
> > <removed Alan from Cc as the mail address not reachable>
> >
> > On Mon, 2021-11-08 at 18:14 +0300, Dan Carpenter wrote:
> > > On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:
> > > > On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> > > > > On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> > > > > > The function ia_css_mipi_is_source_port_valid() returns true if the port
> > > > > > is valid. So, we can't use the existing err variable as is.
> > > > > >
> > > > > > To fix this issue while reusing that variable, invert the return value
> > > > > > when assigning it to the variable.
> > > > > >
> > > > > > Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> > > > > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > > > > ---
> > > > > > .../staging/media/atomisp/pci/sh_css_mipi.c | 24 ++++++++++++-------
> > > > > > 1 file changed, 15 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > > index 65fc93c5d56b..c1f2f6151c5f 100644
> > > > > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > > > > @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > > > > return 0; /* AM TODO: Check */
> > > > > > }
> > > > > >
> > > > > > - if (!IS_ISP2401)
> > > > > > + if (!IS_ISP2401) {
> > > > > > port = (unsigned int)pipe->stream->config.source.port.port;
> > > > > > - else
> > > > > > - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > > > + } else {
> > > > > > + /* Returns true if port is valid. So, invert it */
> > > > > > + err = !ia_css_mipi_is_source_port_valid(pipe, &port);
> > > > >
> > > > > Don't invert it... This isn't supposed to return 1 on failure it's
> > > > > supposed to return negative error codes.
> > > >
> > > > You mean I should instead modify the return value of
> > > > ia_css_mipi_is_source_port_valid() ?
> > > >
> > >
> > > No. ia_css_mipi_is_source_port_valid() is fine. It has a boolean name
> > > so returning bool is fine. What I'm saying is that allocate_mipi_frames()
> > > should do:
> > >
> > > if (!ia_css_mipi_is_source_port_valid(pipe, &port))
> > > err = -EINVAL;
> > >
> > > Otherwise it returns negative error codes and 1 on failure.
> >
> > Ah, I see! Thank you. I feel I'm a stupid... I'll do so in v2.
>
> I would prefer if you could send such changes on new patches.
I'm a little bit too late, sorry. For the record, the return value issue
pointed out here is already gone with patch ("media: atomisp: sh_css_mipi:
cleanup the code") [1]. Thanks!
[1] https://lore.kernel.org/linux-media/b541d4c9923154be7ae0518d01ce994acbef3f9b.1637142905.git.mchehab+huawei@kernel.org/
^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH 06/17] media: atomisp: pci: use IA_CSS_ERROR() for error messages in sh_css_mipi.c
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (4 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 05/17] media: atomisp: pci: fix inverted error check for ia_css_mipi_is_source_port_valid() Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-11-02 11:35 ` Dan Carpenter
2021-10-17 16:19 ` [PATCH 07/17] media: atomisp: pci: fix ifdefs in sh_css.c Tsuchiya Yuto
` (15 subsequent siblings)
21 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Yang Yingliang, Aline Santana Cordeiro, Hans Verkuil, Alan,
Dinghao Liu, Deepak R Varma, Alex Dewar, linux-media,
linux-staging, linux-kernel
Some debug messages for error cases (messages contain "error: ") use
IA_CSS_DEBUG_TRACE_PRIVATE debug level. This causes these error messages
not to appear unless users raise debug output level to 7 or higher (using
module parameter, dbg_level=7).
So, use IA_CSS_DEBUG_ERROR debug level (dbg_level=1) instead considering
that these are error messages. There is already a macro IA_CSS_ERROR()
for this use case. Let's use it. It automatically appends "error: " at
the beginning and a newline to a message. Therefore, we can remove them
from these messages.
While here, remove the unnecessary newline from one IA_CSS_ERROR()
occurrence in the same file.
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../staging/media/atomisp/pci/sh_css_mipi.c | 32 ++++++++-----------
1 file changed, 13 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index c1f2f6151c5f..de56a1da754d 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -434,9 +434,8 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
(IS_ISP2401 && err)) {
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
- pipe, port);
+ IA_CSS_ERROR("allocate_mipi_frames(%p) exit: port is not correct (port=%d).",
+ pipe, port);
return -EINVAL;
}
@@ -497,9 +496,8 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
my_css.mipi_frames[port][j] = NULL;
}
}
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "allocate_mipi_frames(%p, %d) exit: error: allocation failed.\n",
- pipe, port);
+ IA_CSS_ERROR("allocate_mipi_frames(%p, %d) exit: allocation failed.",
+ pipe, port);
return err;
}
}
@@ -542,16 +540,14 @@ free_mipi_frames(struct ia_css_pipe *pipe)
if (pipe) {
assert(pipe->stream);
if ((!pipe) || (!pipe->stream)) {
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "free_mipi_frames(%p) exit: error: pipe or stream is null.\n",
- pipe);
+ IA_CSS_ERROR("free_mipi_frames(%p) exit: pipe or stream is null.",
+ pipe);
return -EINVAL;
}
if (!buffers_needed(pipe)) {
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "free_mipi_frames(%p) exit: error: wrong mode.\n",
- pipe);
+ IA_CSS_ERROR("free_mipi_frames(%p) exit: wrong mode.",
+ pipe);
return err;
}
@@ -566,9 +562,8 @@ free_mipi_frames(struct ia_css_pipe *pipe)
if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
(IS_ISP2401 && err)) {
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "free_mipi_frames(%p, %d) exit: error: pipe port is not correct.\n",
- pipe, port);
+ IA_CSS_ERROR("free_mipi_frames(%p, %d) exit: pipe port is not correct.",
+ pipe, port);
return err;
}
@@ -576,9 +571,8 @@ free_mipi_frames(struct ia_css_pipe *pipe)
#if !defined(ISP2401)
assert(ref_count_mipi_allocation[port] == 1);
if (ref_count_mipi_allocation[port] != 1) {
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "free_mipi_frames(%p) exit: error: wrong ref_count (ref_count=%d).\n",
- pipe, ref_count_mipi_allocation[port]);
+ IA_CSS_ERROR("free_mipi_frames(%p) exit: wrong ref_count (ref_count=%d).",
+ pipe, ref_count_mipi_allocation[port]);
return err;
}
#endif
@@ -680,7 +674,7 @@ send_mipi_frames(struct ia_css_pipe *pipe)
if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
(IS_ISP2401 && err)) {
- IA_CSS_ERROR("send_mipi_frames(%p) exit: invalid port specified (port=%d).\n",
+ IA_CSS_ERROR("send_mipi_frames(%p) exit: invalid port specified (port=%d).",
pipe, port);
return err;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH 06/17] media: atomisp: pci: use IA_CSS_ERROR() for error messages in sh_css_mipi.c
2021-10-17 16:19 ` [PATCH 06/17] media: atomisp: pci: use IA_CSS_ERROR() for error messages in sh_css_mipi.c Tsuchiya Yuto
@ 2021-11-02 11:35 ` Dan Carpenter
2021-11-08 15:39 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Dan Carpenter @ 2021-11-02 11:35 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Yang Yingliang,
Aline Santana Cordeiro, Hans Verkuil, Alan, Dinghao Liu,
Deepak R Varma, Alex Dewar, linux-media, linux-staging,
linux-kernel
On Mon, Oct 18, 2021 at 01:19:46AM +0900, Tsuchiya Yuto wrote:
> .../staging/media/atomisp/pci/sh_css_mipi.c | 32 ++++++++-----------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> index c1f2f6151c5f..de56a1da754d 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> @@ -434,9 +434,8 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>
> if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
> (IS_ISP2401 && err)) {
> - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> - "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> - pipe, port);
> + IA_CSS_ERROR("allocate_mipi_frames(%p) exit: port is not correct (port=%d).",
> + pipe, port);
Not related to this patch but these printks should be using __func__
instead of hard coding it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 06/17] media: atomisp: pci: use IA_CSS_ERROR() for error messages in sh_css_mipi.c
2021-11-02 11:35 ` Dan Carpenter
@ 2021-11-08 15:39 ` Tsuchiya Yuto
2021-11-08 16:39 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-11-08 15:39 UTC (permalink / raw)
To: Dan Carpenter
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Yang Yingliang,
Aline Santana Cordeiro, Hans Verkuil, Dinghao Liu,
Deepak R Varma, Alex Dewar, linux-media, linux-staging,
linux-kernel
<removed Alan from Cc as the mail address not reachable>
On Tue, 2021-11-02 at 14:35 +0300, Dan Carpenter wrote:
> On Mon, Oct 18, 2021 at 01:19:46AM +0900, Tsuchiya Yuto wrote:
> > .../staging/media/atomisp/pci/sh_css_mipi.c | 32 ++++++++-----------
> > 1 file changed, 13 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > index c1f2f6151c5f..de56a1da754d 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > @@ -434,9 +434,8 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> >
> > if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
> > (IS_ISP2401 && err)) {
> > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > - "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> > - pipe, port);
> > + IA_CSS_ERROR("allocate_mipi_frames(%p) exit: port is not correct (port=%d).",
> > + pipe, port);
>
> Not related to this patch but these printks should be using __func__
> instead of hard coding it.
OK, considering that I'll add a separate space issue fix patch in v2 as
discussed in another mail, I'll also add the separate fix for minor
issue fixes here, including the usage of `__func__` and dropping
the unneeded newline `\n` I'm currently doing while here.
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 06/17] media: atomisp: pci: use IA_CSS_ERROR() for error messages in sh_css_mipi.c
2021-11-08 15:39 ` Tsuchiya Yuto
@ 2021-11-08 16:39 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 79+ messages in thread
From: Mauro Carvalho Chehab @ 2021-11-08 16:39 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Dan Carpenter, Hans de Goede, Patrik Gfeller, Sakari Ailus,
Greg Kroah-Hartman, Yang Yingliang, Aline Santana Cordeiro,
Hans Verkuil, Dinghao Liu, Deepak R Varma, Alex Dewar,
linux-media, linux-staging, linux-kernel
Em Tue, 09 Nov 2021 00:39:16 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> <removed Alan from Cc as the mail address not reachable>
>
> On Tue, 2021-11-02 at 14:35 +0300, Dan Carpenter wrote:
> > On Mon, Oct 18, 2021 at 01:19:46AM +0900, Tsuchiya Yuto wrote:
> > > .../staging/media/atomisp/pci/sh_css_mipi.c | 32 ++++++++-----------
> > > 1 file changed, 13 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > index c1f2f6151c5f..de56a1da754d 100644
> > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > @@ -434,9 +434,8 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> > >
> > > if ((!IS_ISP2401 && port >= N_CSI_PORTS) ||
> > > (IS_ISP2401 && err)) {
> > > - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > > - "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> > > - pipe, port);
> > > + IA_CSS_ERROR("allocate_mipi_frames(%p) exit: port is not correct (port=%d).",
> > > + pipe, port);
> >
> > Not related to this patch but these printks should be using __func__
> > instead of hard coding it.
>
> OK, considering that I'll add a separate space issue fix patch in v2 as
> discussed in another mail, I'll also add the separate fix for minor
> issue fixes here, including the usage of `__func__` and dropping
> the unneeded newline `\n` I'm currently doing while here.
Better to keep the \n. The right fix - not only here but everywhere - would
be to convert all those into dev_info/dev_dbg/..., but this is a huge
change.
I would prefer to do such changes on a separate patch series that
will do only this kind of changes. After the conversion, the string
should finish with a \n. So, dropping it will just make the conversion
more error-prone.
Regards,
Mauro
>
> Regards,
> Tsuchiya Yuto
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH 07/17] media: atomisp: pci: fix ifdefs in sh_css.c
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (5 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 06/17] media: atomisp: pci: use IA_CSS_ERROR() for error messages in sh_css_mipi.c Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 08/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 1/5 Tsuchiya Yuto
` (14 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Aline Santana Cordeiro, Yang Yingliang, Hans Verkuil,
Dinghao Liu, Alan, Martiros Shakhzadyan, Kees Cook, linux-media,
linux-staging, linux-kernel
## `if (pipe->stream->config.mode == IA_CSS_INPUT_MODE_TPG) {` case
The intel-aero atomisp has `#if defined(IS_ISP_2400_SYSTEM)` [1]. It is
to be defined in the following two places [2]:
- css/hive_isp_css_common/system_global.h
- css/css_2401_csi2p_system/system_global.h
and the former file is to be included on ISP2400 devices, too. So, it
is to be defined for both ISP2400 and ISP2401 devices.
Because the upstreamed atomisp driver now supports only ISP2400 and
ISP2401, just remove the ISP version test again. This matches the other
upstream commits like 3c0538fbad9f ("media: atomisp: get rid of most
checks for ISP2401 version").
While here, moved the comment for define GP_ISEL_TPG_MODE to the
appropriate place.
[1] https://github.com/intel-aero/linux-kernel/blob/a1b673258feb915268377275130c5c5df0eafc82/drivers/media/pci/atomisp/css/sh_css.c#L552-L558
[2] https://github.com/intel-aero/linux-kernel/search?q=IS_ISP_2400_SYSTEM
## `isys_stream_descr->polling_mode` case
This does not exist on the intel-aero atomisp. This is because it is
based on css version irci_stable_candrpv_0415_20150521_0458.
On the other hand, the upstreamed atomisp is based on the following css
version depending on the ISP version using ifdefs:
- ISP2400: irci_stable_candrpv_0415_20150521_0458
- ISP2401: irci_master_20150911_0724
The `isys_stream_descr->polling_mode` usage was added on updating css
version to irci_master_20150701_0213 [3].
So, it is not a ISP version specific thing, but css version specific
thing. Because the upstreamed atomisp driver uses irci_master_20150911_0724
for ISP2401, re-add the ISP version check for now.
I say "for now" because ISP2401 should eventually use the same css
version with ISP2400 (i.e., irci_stable_candrpv_0415_20150521_0458)
[3] https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
Link to Intel's Android kernel patch.
## `coord = &me->config.internal_frame_origin_bqs_on_sctbl;` case
it was added on commit 4f744a573db3 ("media: atomisp: make
sh_css_sp_init_pipeline() ISP version independent") for ISP2401. Because
the upstreamed atomisp for the ISP2401 part is based on
irci_master_20150911_0724, hence the difference.
Because the upstreamed atomisp driver uses irci_master_20150911_0724
for ISP2401, revert the test back to `if (IS_ISP2401)`.
Fixes: 27333dadef57 ("media: atomisp: adjust some code at sh_css that could be broken")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
drivers/staging/media/atomisp/pci/sh_css.c | 27 +++++++++-------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index c4b35cbab373..ba25d0da8b81 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -522,6 +522,7 @@ ia_css_stream_input_format_bits_per_pixel(struct ia_css_stream *stream)
return bpp;
}
+/* TODO: move define to proper file in tools */
#define GP_ISEL_TPG_MODE 0x90058
#if !defined(ISP2401)
@@ -573,12 +574,8 @@ sh_css_config_input_network(struct ia_css_stream *stream)
vblank_cycles = vblank_lines * (width + hblank_cycles);
sh_css_sp_configure_sync_gen(width, height, hblank_cycles,
vblank_cycles);
- if (!IS_ISP2401) {
- if (pipe->stream->config.mode == IA_CSS_INPUT_MODE_TPG) {
- /* TODO: move define to proper file in tools */
- ia_css_device_store_uint32(GP_ISEL_TPG_MODE, 0);
- }
- }
+ if (pipe->stream->config.mode == IA_CSS_INPUT_MODE_TPG)
+ ia_css_device_store_uint32(GP_ISEL_TPG_MODE, 0);
}
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
"sh_css_config_input_network() leave:\n");
@@ -1009,16 +1006,14 @@ static bool sh_css_translate_stream_cfg_to_isys_stream_descr(
* ia_css_isys_stream_capture_indication() instead of
* ia_css_pipeline_sp_wait_for_isys_stream_N() as isp processing of
* capture takes longer than getting an ISYS frame
- *
- * Only 2401 relevant ??
*/
-#if 0 // FIXME: NOT USED on Yocto Aero
- isys_stream_descr->polling_mode
- = early_polling ? INPUT_SYSTEM_POLL_ON_CAPTURE_REQUEST
- : INPUT_SYSTEM_POLL_ON_WAIT_FOR_FRAME;
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "sh_css_translate_stream_cfg_to_isys_stream_descr() leave:\n");
-#endif
+ if (IS_ISP2401) {
+ isys_stream_descr->polling_mode
+ = early_polling ? INPUT_SYSTEM_POLL_ON_CAPTURE_REQUEST
+ : INPUT_SYSTEM_POLL_ON_WAIT_FOR_FRAME;
+ ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
+ "sh_css_translate_stream_cfg_to_isys_stream_descr() leave:\n");
+ }
return rc;
}
@@ -1433,7 +1428,7 @@ static void start_pipe(
assert(me); /* all callers are in this file and call with non null argument */
- if (!IS_ISP2401) {
+ if (IS_ISP2401) {
coord = &me->config.internal_frame_origin_bqs_on_sctbl;
params = me->stream->isp_params_configs;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 08/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 1/5
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (6 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 07/17] media: atomisp: pci: fix ifdefs in sh_css.c Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 09/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 2/5 Tsuchiya Yuto
` (13 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Yang Yingliang, Aline Santana Cordeiro,
Dinghao Liu, Alan, linux-media, linux-staging, linux-kernel
This is one of the patches which partially revert incompatible changes
in the current css version for ISP2401 (irci_ecr-master_20150911_0724)
back to irci_stable_candrpv_0415_20150521_0458.
Some `struct`s are `sizeof()`ed in sh_css_firmware.c file. So, I guess
issues will happen if these sizes are changed. Therefore, keep them the
same as css version irci_stable_candrpv_0415_20150521_0458 to make atomisp
work for firmware made for such css version since we don't have firmware
made for the current css version.
This patch removes luma_only, input_yuv and input_raw from
`struct ia_css_binary_info` as well as its usage [1]. Note that for
input_yuv and input_raw, only the definitions were removed because these
were not used anywhere.
[1] added on updating css version to irci_master_20150701_0213
https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../atomisp/pci/camera/pipe/src/pipe_binarydesc.c | 5 -----
.../staging/media/atomisp/pci/ia_css_acc_types.h | 5 -----
.../staging/media/atomisp/pci/ia_css_pipe_public.h | 3 ---
.../pci/runtime/binary/interface/ia_css_binary.h | 1 -
.../media/atomisp/pci/runtime/binary/src/binary.c | 14 --------------
5 files changed, 28 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/camera/pipe/src/pipe_binarydesc.c b/drivers/staging/media/atomisp/pci/camera/pipe/src/pipe_binarydesc.c
index f20c9b02fbe0..3e3e5a4f8117 100644
--- a/drivers/staging/media/atomisp/pci/camera/pipe/src/pipe_binarydesc.c
+++ b/drivers/staging/media/atomisp/pci/camera/pipe/src/pipe_binarydesc.c
@@ -58,7 +58,6 @@ static void pipe_binarydesc_get_offline(
descr->enable_dz = true;
descr->enable_xnr = false;
descr->enable_dpc = false;
- descr->enable_luma_only = false;
descr->enable_tnr = false;
descr->enable_capture_pp_bli = false;
descr->enable_fractional_ds = false;
@@ -390,8 +389,6 @@ int ia_css_pipe_get_video_binarydesc(
pipe->extra_config.enable_fractional_ds;
video_descr->enable_dpc =
pipe->config.enable_dpc;
- video_descr->enable_luma_only =
- pipe->config.enable_luma_only;
video_descr->enable_tnr =
pipe->config.enable_tnr;
@@ -600,8 +597,6 @@ void ia_css_pipe_get_primary_binarydesc(
prim_descr->isp_pipe_version = pipe->config.isp_pipe_version;
prim_descr->enable_fractional_ds =
pipe->extra_config.enable_fractional_ds;
- prim_descr->enable_luma_only =
- pipe->config.enable_luma_only;
/* We have both striped and non-striped primary binaries,
* if continuous viewfinder is required, then we must select
* a striped one. Otherwise we prefer to use a non-striped
diff --git a/drivers/staging/media/atomisp/pci/ia_css_acc_types.h b/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
index 36583ab12e3f..d0ce2f8ba653 100644
--- a/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
+++ b/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
@@ -222,11 +222,6 @@ struct ia_css_binary_info {
struct ia_css_isp_param_isp_segments mem_initializers;
/* MW: Packing (related) bools in an integer ?? */
struct {
- /* ISP2401 */
- u8 luma_only;
- u8 input_yuv;
- u8 input_raw;
-
u8 reduced_pipe;
u8 vf_veceven;
u8 dis;
diff --git a/drivers/staging/media/atomisp/pci/ia_css_pipe_public.h b/drivers/staging/media/atomisp/pci/ia_css_pipe_public.h
index 4affd21f9e3f..45e8fe36cb74 100644
--- a/drivers/staging/media/atomisp/pci/ia_css_pipe_public.h
+++ b/drivers/staging/media/atomisp/pci/ia_css_pipe_public.h
@@ -123,9 +123,6 @@ struct ia_css_pipe_config {
processing stages. */
/* ISP2401 */
- bool enable_luma_only;
- /** Enabling of monochrome mode for a pipeline. If enabled only luma processing
- will be done. */
bool enable_tnr;
/** Enabling of TNR (temporal noise reduction). This is only applicable to video
pipes. Non video-pipes should always set this parameter to false. */
diff --git a/drivers/staging/media/atomisp/pci/runtime/binary/interface/ia_css_binary.h b/drivers/staging/media/atomisp/pci/runtime/binary/interface/ia_css_binary.h
index b44099dbdacd..6f110d241836 100644
--- a/drivers/staging/media/atomisp/pci/runtime/binary/interface/ia_css_binary.h
+++ b/drivers/staging/media/atomisp/pci/runtime/binary/interface/ia_css_binary.h
@@ -94,7 +94,6 @@ struct ia_css_binary_descr {
bool enable_dpc;
/* ISP2401 */
- bool enable_luma_only;
bool enable_tnr;
bool enable_capture_pp_bli;
diff --git a/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c b/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c
index 060d38749570..8c0e02e4e1af 100644
--- a/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c
+++ b/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c
@@ -1394,9 +1394,6 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr,
bool enable_dvs_6axis;
bool enable_reduced_pipe;
bool enable_capture_pp_bli;
-#ifdef ISP2401
- bool enable_luma_only;
-#endif
int err = -EINVAL;
bool continuous;
unsigned int isp_pipe_version;
@@ -1450,9 +1447,6 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr,
enable_dvs_6axis = descr->enable_dvs_6axis;
enable_reduced_pipe = descr->enable_reduced_pipe;
enable_capture_pp_bli = descr->enable_capture_pp_bli;
-#ifdef ISP2401
- enable_luma_only = descr->enable_luma_only;
-#endif
continuous = descr->continuous;
striped = descr->striped;
isp_pipe_version = descr->isp_pipe_version;
@@ -1748,14 +1742,6 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr,
}
#ifdef ISP2401
- if (candidate->enable.luma_only != enable_luma_only) {
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
- "ia_css_binary_find() [%d] continue: %d != %d\n",
- __LINE__, candidate->enable.luma_only,
- descr->enable_luma_only);
- continue;
- }
-
if (!candidate->enable.tnr && need_tnr) {
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
"ia_css_binary_find() [%d] continue: !%d && %d\n",
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 09/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 2/5
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (7 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 08/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 1/5 Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 10/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 3/5 Tsuchiya Yuto
` (12 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Yang Yingliang, Aline Santana Cordeiro,
Dinghao Liu, Alan, Deepak R Varma, linux-media, linux-staging,
linux-kernel
This is one of the patches which partially revert incompatible changes
in the current css version for ISP2401 (irci_ecr-master_20150911_0724)
back to irci_stable_candrpv_0415_20150521_0458.
Some `struct`s are `sizeof()`ed in sh_css_firmware.c file. So, I guess
issues will happen if these sizes are changed. Therefore, keep them the
same as css version irci_stable_candrpv_0415_20150521_0458 to make atomisp
work for firmware made for such css version since we don't have firmware
made for the current css version.
This patch removes `struct ia_css_isp_parameter sc` from
`struct ia_css_config_memory_offsets` as well as its usage [1].
[1] added on updating css version to irci_master_20150701_0213
https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../media/atomisp/pci/ia_css_isp_configs.h | 8 ---
.../isp/kernels/sc/sc_1.0/ia_css_sc.host.c | 68 -------------------
.../isp/kernels/sc/sc_1.0/ia_css_sc.host.h | 33 ---------
.../isp/kernels/sc/sc_1.0/ia_css_sc_types.h | 14 ----
drivers/staging/media/atomisp/pci/sh_css_sp.c | 4 --
5 files changed, 127 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/ia_css_isp_configs.h b/drivers/staging/media/atomisp/pci/ia_css_isp_configs.h
index 1abb2fd6a913..0364b932e79b 100644
--- a/drivers/staging/media/atomisp/pci/ia_css_isp_configs.h
+++ b/drivers/staging/media/atomisp/pci/ia_css_isp_configs.h
@@ -23,10 +23,6 @@
#include "isp/kernels/raw/raw_1.0/ia_css_raw.host.h"
#include "isp/kernels/ref/ref_1.0/ia_css_ref.host.h"
#include "isp/kernels/s3a/s3a_1.0/ia_css_s3a.host.h"
-
-/* ISP2401 */
-#include "isp/kernels/sc/sc_1.0/ia_css_sc.host.h"
-
#include "isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.h"
#include "isp/kernels/vf/vf_1.0/ia_css_vf.host.h"
#include "isp/kernels/iterator/iterator_1.0/ia_css_iterator.host.h"
@@ -73,10 +69,6 @@ struct ia_css_config_memory_offsets {
struct ia_css_isp_parameter output0;
struct ia_css_isp_parameter output1;
struct ia_css_isp_parameter output;
-
- /* ISP2401 */
- struct ia_css_isp_parameter sc;
-
struct ia_css_isp_parameter raw;
struct ia_css_isp_parameter tnr;
struct ia_css_isp_parameter ref;
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.c
index f3fb4b9b3c82..6974b3424d91 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.c
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.c
@@ -23,35 +23,6 @@
#include "ia_css_sc.host.h"
-/* Code generated by genparam/genconfig.c:gen_configure_function() */
-
-/* ISP2401 */
-static void
-ia_css_configure_sc(
- const struct ia_css_binary *binary,
- const struct ia_css_sc_configuration *config_dmem)
-{
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "ia_css_configure_sc() enter:\n");
-
- {
- unsigned int offset = 0;
- unsigned int size = 0;
-
- if (binary->info->mem_offsets.offsets.config) {
- size = binary->info->mem_offsets.offsets.config->dmem.sc.size;
- offset = binary->info->mem_offsets.offsets.config->dmem.sc.offset;
- }
- if (size) {
- ia_css_sc_config((struct sh_css_isp_sc_isp_config *)
- &binary->mem_params.params[IA_CSS_PARAM_CLASS_CONFIG][IA_CSS_ISP_DMEM].address[offset],
- config_dmem, size);
- }
- }
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "ia_css_configure_sc() leave:\n");
-}
-
void
ia_css_sc_encode(
struct sh_css_isp_sc_params *to,
@@ -73,45 +44,6 @@ ia_css_sc_dump(
"sc_gain_shift", sc->gain_shift);
}
-/* ISP2401 */
-void
-ia_css_sc_config(
- struct sh_css_isp_sc_isp_config *to,
- const struct ia_css_sc_configuration *from,
- unsigned int size)
-{
- u32 internal_org_x_bqs = from->internal_frame_origin_x_bqs_on_sctbl;
- u32 internal_org_y_bqs = from->internal_frame_origin_y_bqs_on_sctbl;
- u32 slice, rest, i;
-
- (void)size;
-
- /* The internal_frame_origin_x_bqs_on_sctbl is separated to 8 times of slice_vec. */
- rest = internal_org_x_bqs;
- for (i = 0; i < SH_CSS_SC_INTERPED_GAIN_HOR_SLICE_TIMES; i++) {
- slice = min(rest, ((uint32_t)ISP_SLICE_NELEMS));
- rest = rest - slice;
- to->interped_gain_hor_slice_bqs[i] = slice;
- }
-
- to->internal_frame_origin_y_bqs_on_sctbl = internal_org_y_bqs;
-}
-
-/* ISP2401 */
-void
-ia_css_sc_configure(
- const struct ia_css_binary *binary,
- u32 internal_frame_origin_x_bqs_on_sctbl,
- uint32_t internal_frame_origin_y_bqs_on_sctbl)
-{
- const struct ia_css_sc_configuration config = {
- internal_frame_origin_x_bqs_on_sctbl,
- internal_frame_origin_y_bqs_on_sctbl
- };
-
- ia_css_configure_sc(binary, &config);
-}
-
/* ------ deprecated(bz675) : from ------ */
/* It looks like @parameter{} (in *.pipe) is used to generate the process/get/set functions,
for parameters which should be used in the isp kernels.
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.h b/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.h
index f1eb568f23d4..d103103c9a87 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.h
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc.host.h
@@ -32,39 +32,6 @@ ia_css_sc_dump(
const struct sh_css_isp_sc_params *sc,
unsigned int level);
-/* @brief Configure the shading correction.
- * @param[out] to Parameters used in the shading correction kernel in the isp.
- * @param[in] from Parameters passed from the host.
- * @param[in] size Size of the sh_css_isp_sc_isp_config structure.
- *
- * This function passes the parameters for the shading correction from the host to the isp.
- */
-/* ISP2401 */
-void
-ia_css_sc_config(
- struct sh_css_isp_sc_isp_config *to,
- const struct ia_css_sc_configuration *from,
- unsigned int size);
-
-/* @brief Configure the shading correction.
- * @param[in] binary The binary, which has the shading correction.
- * @param[in] internal_frame_origin_x_bqs_on_sctbl
- * X coordinate (in bqs) of the origin of the internal frame on the shading table.
- * @param[in] internal_frame_origin_y_bqs_on_sctbl
- * Y coordinate (in bqs) of the origin of the internal frame on the shading table.
- *
- * This function calls the ia_css_configure_sc() function.
- * (The ia_css_configure_sc() function is automatically generated in ia_css_isp.configs.c.)
- * The ia_css_configure_sc() function calls the ia_css_sc_config() function
- * to pass the parameters for the shading correction from the host to the isp.
- */
-/* ISP2401 */
-void
-ia_css_sc_configure(
- const struct ia_css_binary *binary,
- u32 internal_frame_origin_x_bqs_on_sctbl,
- uint32_t internal_frame_origin_y_bqs_on_sctbl);
-
/* ------ deprecated(bz675) : from ------ */
void
sh_css_get_shading_settings(const struct ia_css_isp_parameters *params,
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc_types.h b/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc_types.h
index aae534521b7b..1d70f6b9a0ec 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc_types.h
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/sc/sc_1.0/ia_css_sc_types.h
@@ -118,18 +118,4 @@ struct ia_css_shading_settings {
/* ------ deprecated(bz675) : to ------ */
-/* Shading Correction configuration.
- *
- * NOTE: The shading table size is larger than or equal to the internal frame size.
- */
-/* ISP2401 */
-struct ia_css_sc_configuration {
- u32 internal_frame_origin_x_bqs_on_sctbl; /** Origin X (in bqs) of internal frame on shading table. */
- u32 internal_frame_origin_y_bqs_on_sctbl; /** Origin Y (in bqs) of internal frame on shading table. */
- /** NOTE: bqs = size in BQ(Bayer Quad) unit.
- 1BQ means {Gr,R,B,Gb}(2x2 pixels).
- Horizontal 1 bqs corresponds to horizontal 2 pixels.
- Vertical 1 bqs corresponds to vertical 2 pixels. */
-};
-
#endif /* __IA_CSS_SC_TYPES_H */
diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index a73e8ca1e225..13b15a5a33bc 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -827,10 +827,6 @@ configure_isp_from_args(
ia_css_output1_configure(binary, &args->out_vf_frame->info);
ia_css_copy_output_configure(binary, args->copy_output);
ia_css_output0_configure(binary, &args->out_frame[0]->info);
-#ifdef ISP2401
- ia_css_sc_configure(binary, pipeline->shading.internal_frame_origin_x_bqs_on_sctbl,
- pipeline->shading.internal_frame_origin_y_bqs_on_sctbl);
-#endif
ia_css_iterator_configure(binary, &args->in_frame->info);
ia_css_dvs_configure(binary, &args->out_frame[0]->info);
ia_css_output_configure(binary, &args->out_frame[0]->info);
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 10/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 3/5
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (8 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 09/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 2/5 Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 11/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 4/5 Tsuchiya Yuto
` (11 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Aline Santana Cordeiro, Yang Yingliang, Hans Verkuil,
Dinghao Liu, Alan, linux-media, linux-staging, linux-kernel
This is one of the patches which partially revert incompatible changes
in the current css version for ISP2401 (irci_ecr-master_20150911_0724)
back to irci_stable_candrpv_0415_20150521_0458.
Some `struct`s are `sizeof()`ed in sh_css_firmware.c file. So, I guess
issues will happen if these sizes are changed. Therefore, keep them the
same as css version irci_stable_candrpv_0415_20150521_0458 to make atomisp
work for firmware made for such css version since we don't have firmware
made for the current css version.
This patch removes `struct ia_css_isp_parameter xnr3` from
`struct ia_css_memory_offsets` as well as its usage [1].
[1] added on updating css version to irci_master_20150701_0213
https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../css_2401_system/hive/ia_css_isp_params.c | 23 -------------------
.../media/atomisp/pci/ia_css_isp_params.h | 3 ---
2 files changed, 26 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/css_2401_system/hive/ia_css_isp_params.c b/drivers/staging/media/atomisp/pci/css_2401_system/hive/ia_css_isp_params.c
index d9c672d8904e..503ac65da69b 100644
--- a/drivers/staging/media/atomisp/pci/css_2401_system/hive/ia_css_isp_params.c
+++ b/drivers/staging/media/atomisp/pci/css_2401_system/hive/ia_css_isp_params.c
@@ -1721,29 +1721,6 @@ ia_css_process_xnr3(
"ia_css_process_xnr3() leave:\n");
}
}
- {
- unsigned int size =
- stage->binary->info->mem_offsets.offsets.param->vmem.xnr3.size;
-
- unsigned int offset =
- stage->binary->info->mem_offsets.offsets.param->vmem.xnr3.offset;
-
- if (size) {
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "ia_css_process_xnr3() enter:\n");
-
- ia_css_xnr3_vmem_encode((struct sh_css_isp_xnr3_vmem_params *)
- &stage->binary->mem_params.params[IA_CSS_PARAM_CLASS_PARAM][IA_CSS_ISP_VMEM].address[offset],
- ¶ms->xnr3_config,
- size);
- params->isp_params_changed = true;
- params->isp_mem_params_changed[pipe_id][stage->stage_num][IA_CSS_ISP_VMEM] =
- true;
-
- ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
- "ia_css_process_xnr3() leave:\n");
- }
- }
}
/* Code generated by genparam/gencode.c:gen_param_process_table() */
diff --git a/drivers/staging/media/atomisp/pci/ia_css_isp_params.h b/drivers/staging/media/atomisp/pci/ia_css_isp_params.h
index 6e3082b39ed6..c2de689877d1 100644
--- a/drivers/staging/media/atomisp/pci/ia_css_isp_params.h
+++ b/drivers/staging/media/atomisp/pci/ia_css_isp_params.h
@@ -121,9 +121,6 @@ struct ia_css_memory_offsets {
struct ia_css_isp_parameter sdis_vertcoef;
struct ia_css_isp_parameter sdis2_horicoef;
struct ia_css_isp_parameter sdis2_vertcoef;
-
- /* ISP2401 */
- struct ia_css_isp_parameter xnr3;
} vmem;
struct {
struct ia_css_isp_parameter bh;
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 11/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 4/5
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (9 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 10/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 3/5 Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 12/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 5/5 Tsuchiya Yuto
` (10 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Aline Santana Cordeiro, Yang Yingliang,
Dinghao Liu, Alan, Aditya Srivastava, Martiros Shakhzadyan,
Kees Cook, linux-media, linux-staging, linux-kernel
This is one of the patches which partially revert incompatible changes
in the current css version for ISP2401 (irci_ecr-master_20150911_0724)
back to irci_stable_candrpv_0415_20150521_0458.
Some `struct`s are `sizeof()`ed in sh_css_firmware.c file. So, I guess
issues will happen if these sizes are changed. Therefore, keep them the
same as css version irci_stable_candrpv_0415_20150521_0458 to make atomisp
work for firmware made for such css version since we don't have firmware
made for the current css version.
This patch removes polling_mode and subscr_index from
`struct virtual_input_system_stream_s` as well as its usage [1]. Note
that for subscr_index, only the definition were removed because it was
not used anywhere.
[1] added on updating css version to irci_master_20150701_0213
https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../atomisp/pci/isp2401_input_system_global.h | 12 ------------
.../atomisp/pci/runtime/isys/src/virtual_isys.c | 11 -----------
drivers/staging/media/atomisp/pci/sh_css.c | 14 ++------------
3 files changed, 2 insertions(+), 35 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/isp2401_input_system_global.h b/drivers/staging/media/atomisp/pci/isp2401_input_system_global.h
index f38773842646..e3c86069b390 100644
--- a/drivers/staging/media/atomisp/pci/isp2401_input_system_global.h
+++ b/drivers/staging/media/atomisp/pci/isp2401_input_system_global.h
@@ -44,11 +44,6 @@ typedef enum {
N_INPUT_SYSTEM_SOURCE_TYPE
} input_system_source_type_t;
-typedef enum {
- INPUT_SYSTEM_POLL_ON_WAIT_FOR_FRAME,
- INPUT_SYSTEM_POLL_ON_CAPTURE_REQUEST,
-} input_system_polling_mode_t;
-
typedef struct input_system_channel_s input_system_channel_t;
struct input_system_channel_s {
stream2mmio_ID_t stream2mmio_id;
@@ -111,9 +106,6 @@ struct isp2401_input_system_cfg_s {
input_system_source_type_t mode;
- /* ISP2401 */
- input_system_polling_mode_t polling_mode;
-
bool online;
bool raw_packed;
s8 linked_isys_stream_id;
@@ -165,10 +157,6 @@ struct virtual_input_system_stream_s {
u8 online;
s8 linked_isys_stream_id;
u8 valid;
-
- /* ISP2401 */
- input_system_polling_mode_t polling_mode;
- s32 subscr_index;
};
typedef struct virtual_input_system_stream_cfg_s
diff --git a/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c b/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c
index 82f3c19dc455..8fc7746f8639 100644
--- a/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c
+++ b/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c
@@ -189,17 +189,6 @@ ia_css_isys_error_t ia_css_isys_stream_create(
return false;
}
-#ifdef ISP2401
- /*
- * Early polling is required for timestamp accuracy in certain cause.
- * The ISYS HW polling is started on
- * ia_css_isys_stream_capture_indication() instead of
- * ia_css_pipeline_sp_wait_for_isys_stream_N() as isp processing of
- * capture takes longer than getting an ISYS frame
- */
- isys_stream->polling_mode = isys_stream_descr->polling_mode;
-
-#endif
/* create metadata channel */
if (isys_stream_descr->metadata.enable) {
rc = create_input_system_channel(isys_stream_descr, true,
diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index ba25d0da8b81..79003077f390 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -1000,20 +1000,10 @@ static bool sh_css_translate_stream_cfg_to_isys_stream_descr(
isys_stream_descr->raw_packed = stream_cfg->pack_raw_pixels;
isys_stream_descr->linked_isys_stream_id = (int8_t)
stream_cfg->isys_config[isys_stream_idx].linked_isys_stream_id;
- /*
- * Early polling is required for timestamp accuracy in certain case.
- * The ISYS HW polling is started on
- * ia_css_isys_stream_capture_indication() instead of
- * ia_css_pipeline_sp_wait_for_isys_stream_N() as isp processing of
- * capture takes longer than getting an ISYS frame
- */
- if (IS_ISP2401) {
- isys_stream_descr->polling_mode
- = early_polling ? INPUT_SYSTEM_POLL_ON_CAPTURE_REQUEST
- : INPUT_SYSTEM_POLL_ON_WAIT_FOR_FRAME;
+
+ if (IS_ISP2401)
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
"sh_css_translate_stream_cfg_to_isys_stream_descr() leave:\n");
- }
return rc;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 12/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 5/5
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (10 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 11/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 4/5 Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 13/17] media: atomisp: pci: release_version is now irci_stable_candrpv_0415_20150521_0458 Tsuchiya Yuto
` (9 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Aline Santana Cordeiro, Yang Yingliang, Alan,
Dinghao Liu, Alexey Dobriyan, Ard Biesheuvel, Masahiro Yamada,
Rafael J. Wysocki, Deepak R Varma, linux-media, linux-staging,
linux-kernel
This is one of the patches which partially revert incompatible changes
in the current css version for ISP2401 (irci_ecr-master_20150911_0724)
back to irci_stable_candrpv_0415_20150521_0458.
Some `struct`s are `sizeof()`ed in sh_css_firmware.c file. So, I guess
issues will happen if these sizes are changed. Therefore, keep them the
same as css version irci_stable_candrpv_0415_20150521_0458 to make atomisp
work for firmware made for such css version since we don't have firmware
made for the current css version.
This patch removes the unnamed struct `shading` from
`struct sh_css_sp_pipeline` as well as its usage [1].
[1] added on updating css version to irci_master_20150701_0213
https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../media/atomisp/pci/sh_css_internal.h | 8 --------
drivers/staging/media/atomisp/pci/sh_css_sp.c | 18 ------------------
2 files changed, 26 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
index 496faa7297a5..92fb7e67610c 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
+++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
@@ -551,14 +551,6 @@ struct sh_css_sp_pipeline {
u32 raw_bit_depth;
} raw;
} copy;
-
-/* ISP2401 */
-
- /* Parameters passed to Shading Correction kernel. */
- struct {
- u32 internal_frame_origin_x_bqs_on_sctbl; /* Origin X (bqs) of internal frame on shading table */
- u32 internal_frame_origin_y_bqs_on_sctbl; /* Origin Y (bqs) of internal frame on shading table */
- } shading;
};
/*
diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index 13b15a5a33bc..fa74ac172f94 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -1308,24 +1308,6 @@ sh_css_sp_init_pipeline(struct ia_css_pipeline *me,
}
#endif
- if (IS_ISP2401) {
- /* For the shading correction type 1 (the legacy shading table conversion in css is not used),
- * the parameters are passed to the isp for the shading table centering.
- */
- if (internal_frame_origin_bqs_on_sctbl &&
- params && params->shading_settings.enable_shading_table_conversion == 0) {
- sh_css_sp_group.pipe[thread_id].shading.internal_frame_origin_x_bqs_on_sctbl
- = (uint32_t)internal_frame_origin_bqs_on_sctbl->x;
- sh_css_sp_group.pipe[thread_id].shading.internal_frame_origin_y_bqs_on_sctbl
- = (uint32_t)internal_frame_origin_bqs_on_sctbl->y;
- } else {
- sh_css_sp_group.pipe[thread_id].shading.internal_frame_origin_x_bqs_on_sctbl =
- 0;
- sh_css_sp_group.pipe[thread_id].shading.internal_frame_origin_y_bqs_on_sctbl =
- 0;
- }
- }
-
IA_CSS_LOG("pipe_id %d port_config %08x",
pipe_id, sh_css_sp_group.pipe[thread_id].inout_port_config);
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 13/17] media: atomisp: pci: release_version is now irci_stable_candrpv_0415_20150521_0458
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (11 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 12/17] media: atomisp: pci: make fw ver irci_stable_candrpv_0415_20150521_0458 work - 5/5 Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 14/17] media: atomisp: pci: Remove remaining instance of call to trace_printk Tsuchiya Yuto
` (8 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Aline Santana Cordeiro, Yang Yingliang, Alan,
Dinghao Liu, Fabio M. De Francesco, Jiri Slaby, linux-media,
linux-staging, linux-kernel
Now that we made atomisp work with firmware version
irci_stable_candrpv_0415_20150521_0458 also for ISP2401, the
release_version for ISP2401 is not irci_ecr-master_20150911_0724
anymore.
So, use the same release_version for both ISP2400 and ISP2401 (i.e.,
irci_stable_candrpv_0415_20150521_0458).
Referred to the following diff to make this patch:
- https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/drivers/staging/media/atomisp/pci/sh_css_firmware.c?id=3c0538fbad9f1d07d588f631e380256d941e3d3a
("media: atomisp: get rid of most checks for ISP2401 version")
changes for file sh_css_firmware.c
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../pci/isp/kernels/hdr/ia_css_hdr.host.c | 1 -
.../pci/isp/kernels/hdr/ia_css_hdr.host.h | 1 -
.../pci/isp/kernels/hdr/ia_css_hdr_param.h | 1 -
.../pci/isp/kernels/hdr/ia_css_hdr_types.h | 1 -
.../staging/media/atomisp/pci/sh_css_firmware.c | 16 +---------------
5 files changed, 1 insertion(+), 19 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.c
index 698550cc2fcc..85a02b6adb52 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.c
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.c
@@ -1,6 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
/* Release Version: irci_stable_candrpv_0415_20150521_0458 */
-/* Release Version: irci_ecr-master_20150911_0724 */
/*
* Support for Intel Camera Imaging ISP subsystem.
* Copyright (c) 2015, Intel Corporation.
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.h b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.h
index 04599ab590cd..83277b683c47 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.h
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr.host.h
@@ -1,6 +1,5 @@
/* SPDX-License-Identifier: GPL-2.0 */
/* Release Version: irci_stable_candrpv_0415_20150521_0458 */
-/* Release Version: irci_ecr-master_20150911_0724 */
/*
* Support for Intel Camera Imaging ISP subsystem.
* Copyright (c) 2015, Intel Corporation.
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_param.h b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_param.h
index 97a89fd3cfda..998c6d801756 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_param.h
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_param.h
@@ -1,6 +1,5 @@
/* SPDX-License-Identifier: GPL-2.0 */
/* Release Version: irci_stable_candrpv_0415_20150521_0458 */
-/* Release Version: irci_ecr-master_20150911_0724 */
/*
* Support for Intel Camera Imaging ISP subsystem.
* Copyright (c) 2015, Intel Corporation.
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_types.h b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_types.h
index 1b4090880201..175c301ee96a 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_types.h
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/hdr/ia_css_hdr_types.h
@@ -1,6 +1,5 @@
/* SPDX-License-Identifier: GPL-2.0 */
/* Release Version: irci_stable_candrpv_0415_20150521_0458 */
-/* Release Version: irci_ecr-master_20150911_0724 */
/*
* Support for Intel Camera Imaging ISP subsystem.
* Copyright (c) 2015, Intel Corporation.
diff --git a/drivers/staging/media/atomisp/pci/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
index e1a16a50e588..94149647b98b 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
@@ -56,8 +56,7 @@ static struct firmware_header *firmware_header;
* which will be replaced with the actual RELEASE_VERSION
* during package generation. Please do not modify
*/
-static const char *isp2400_release_version = STR(irci_stable_candrpv_0415_20150521_0458);
-static const char *isp2401_release_version = STR(irci_ecr - master_20150911_0724);
+static const char *release_version = STR(irci_stable_candrpv_0415_20150521_0458);
#define MAX_FW_REL_VER_NAME 300
static char FW_rel_ver_name[MAX_FW_REL_VER_NAME] = "---";
@@ -190,13 +189,6 @@ sh_css_check_firmware_version(struct device *dev, const char *fw_data)
{
struct sh_css_fw_bi_file_h *file_header;
- const char *release_version;
-
- if (!IS_ISP2401)
- release_version = isp2400_release_version;
- else
- release_version = isp2401_release_version;
-
firmware_header = (struct firmware_header *)fw_data;
file_header = &firmware_header->file_header;
@@ -232,12 +224,6 @@ sh_css_load_firmware(struct device *dev, const char *fw_data,
struct ia_css_fw_info *binaries;
struct sh_css_fw_bi_file_h *file_header;
int ret;
- const char *release_version;
-
- if (!IS_ISP2401)
- release_version = isp2400_release_version;
- else
- release_version = isp2401_release_version;
firmware_header = (struct firmware_header *)fw_data;
file_header = &firmware_header->file_header;
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 14/17] media: atomisp: pci: Remove remaining instance of call to trace_printk
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (12 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 13/17] media: atomisp: pci: release_version is now irci_stable_candrpv_0415_20150521_0458 Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
[not found] ` <20211026093224.6c7f7fbf@sal.lan>
2021-10-17 16:19 ` [PATCH 15/17] media: atomsip: pci: add Microsoft Surface 3 ACPI vars Tsuchiya Yuto
` (7 subsequent siblings)
21 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Kaixu Xia, Baokun Li, Hans Verkuil, Aditya Srivastava,
Aline Santana Cordeiro, Yang Yingliang, Alan, Dinghao Liu,
linux-media, linux-staging, linux-kernel
(patch based on intel-aero kernel patch:
https://github.com/intel-aero/meta-intel-aero-base/commit/26fc9fe5030b63bc9dcf0b5f32981948911ca272)
Here is the original commit message from the aforementioned patch:
From 26fc9fe5030b63bc9dcf0b5f32981948911ca272 Mon Sep 17 00:00:00 2001
From: Lucas De Marchi <lucas.demarchi@intel.com>
Date: Fri, 7 Jul 2017 14:23:53 -0700
Subject: [PATCH] linux-yocto: Remove remaining instance of call to
trace_printk
It's not sufficient to leave trace_printk() out of "normal call chains" since
the way trace infrastructure works is that it will allocate the trace_printk
buffers if the symbol is there (by using a separate section for the function
and checking if __start_* and __stop_* symbols are different.
Therefore, even if the default value for the param tells the module to use
printk(), just the fact that it can be changed to trace_printk() means the
initialization code will be called.
The trace_printk() was replaced by pr_info() on commit 3d81099c75a6
("media: atomisp: Replace trace_printk by pr_info") for the upstreamed
atomisp, too. However, as the aforementioned commit message says, there
is still a remaining instance. This causes the "trace_printk() being
used" kernel warning message to still appear on the first driver load.
Based on the aforementioned patch, this patch removes the call to
ftrace_vprintk(). This removes that kernel warning.
In addition to this, this patch also removes the following now unused
things:
- now empty atomisp_css2_dbg_ftrace_print()
- trace_printk option from dbg_func kernel parameter
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../staging/media/atomisp/pci/atomisp_compat_css20.c | 10 ----------
drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 4 ++--
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index 99a632f33d2d..d81d55c6f1fa 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -159,13 +159,6 @@ static void atomisp_css2_hw_load(hrt_address addr, void *to, uint32_t n)
spin_unlock_irqrestore(&mmio_lock, flags);
}
-static int __printf(1, 0) atomisp_css2_dbg_ftrace_print(const char *fmt,
- va_list args)
-{
- ftrace_vprintk(fmt, args);
- return 0;
-}
-
static int __printf(1, 0) atomisp_vprintk(const char *fmt, va_list args)
{
vprintk(fmt, args);
@@ -860,9 +853,6 @@ static inline int __set_css_print_env(struct atomisp_device *isp, int opt)
if (opt == 0)
isp->css_env.isp_css_env.print_env.debug_print = NULL;
else if (opt == 1)
- isp->css_env.isp_css_env.print_env.debug_print =
- atomisp_css2_dbg_ftrace_print;
- else if (opt == 2)
isp->css_env.isp_css_env.print_env.debug_print = atomisp_vprintk;
else
ret = -EINVAL;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index f5362554638e..720963156d24 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -87,10 +87,10 @@ module_param(dbg_level, int, 0644);
MODULE_PARM_DESC(dbg_level, "debug message level (default:0)");
/* log function switch */
-int dbg_func = 2;
+int dbg_func = 1;
module_param(dbg_func, int, 0644);
MODULE_PARM_DESC(dbg_func,
- "log function switch non/trace_printk/printk (default:printk)");
+ "log function switch non/printk (default:printk)");
int mipicsi_flag;
module_param(mipicsi_flag, int, 0644);
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 15/17] media: atomsip: pci: add Microsoft Surface 3 ACPI vars
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (13 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 14/17] media: atomisp: pci: Remove remaining instance of call to trace_printk Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-17 16:19 ` [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB) Tsuchiya Yuto
` (6 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Aniket Bhattacharyea, Hans Verkuil, Yang Yingliang,
Aline Santana Cordeiro, Dinghao Liu, Alan, linux-media,
linux-staging, linux-kernel
Microsoft Surface 3 does not describe CsiPort/CsiLanes in DSDT [1] or
EFI, or at least not desctibed in the forms the current atomisp driver
expects. This results in the default values (port: 0 lanes: 1) to be
used, which does not work.
So, define them ourselves in the driver.
The user-facing camera is AR0330 (2-lane) and the world-facing camera
is OV8835 (4-lane).
According to the portconfigs definition in atomisp_csi_lane_config()
[atomisp_v4l2.c]:
} portconfigs[] = {
/* Tangier/Merrifield available lane configurations */
{ 0x00, { 4, 1, 0 } }, /* 00000 */
{ 0x01, { 3, 1, 0 } }, /* 00001 */
{ 0x02, { 2, 1, 0 } }, /* 00010 */
{ 0x03, { 1, 1, 0 } }, /* 00011 */
{ 0x04, { 2, 1, 2 } }, /* 00100 */
{ 0x08, { 3, 1, 1 } }, /* 01000 */
{ 0x09, { 2, 1, 1 } }, /* 01001 */
{ 0x0a, { 1, 1, 1 } }, /* 01010 */
/* Anniedale/Moorefield only configurations */
{ 0x10, { 4, 2, 0 } }, /* 10000 */
{ 0x11, { 3, 2, 0 } }, /* 10001 */
{ 0x12, { 2, 2, 0 } }, /* 10010 */
{ 0x13, { 1, 2, 0 } }, /* 10011 */
{ 0x14, { 2, 2, 2 } }, /* 10100 */
{ 0x18, { 3, 2, 1 } }, /* 11000 */
{ 0x19, { 2, 2, 1 } }, /* 11001 */
{ 0x1a, { 1, 2, 1 } }, /* 11010 */
};
4-lane camera is always connected to the PRIMARY (port1) port [2]. In
this case, 2-lane camera can be connected to the only SECONDARY (port0)
port.
So, add these data accordingly as the gmin_cfg_var ACPI variables for
Surface 3.
[1] https://github.com/linux-surface/acpidumps/blob/7da48a392b4085c2021952290def1fc28505a643/surface_3/dsdt.dsl#L5879-L6278
[2] Yes, the PRIMARY port is port1 according to atomisp_camera_port
[atomisp.h]:
enum atomisp_camera_port {
ATOMISP_CAMERA_PORT_SECONDARY,
ATOMISP_CAMERA_PORT_PRIMARY,
ATOMISP_CAMERA_PORT_TERTIARY,
ATOMISP_CAMERA_NR_PORTS
};
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../media/atomisp/pci/atomisp_gmin_platform.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index d8c9e31314b2..948eb6f809f5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -321,6 +321,18 @@ static struct gmin_cfg_var i8880_vars[] = {
{},
};
+/*
+ * Surface 3 does not describe CsiPort/CsiLanes in both DSDT and EFI.
+ */
+static struct gmin_cfg_var surface3_vars[] = {
+ {"APTA0330:00_CsiPort", "0"},
+ {"APTA0330:00_CsiLanes", "2"},
+
+ {"OVTI8835:00_CsiPort", "1"},
+ {"OVTI8835:00_CsiLanes", "4"},
+ {},
+};
+
static const struct dmi_system_id gmin_vars[] = {
{
.ident = "BYT-T FFD8",
@@ -358,6 +370,13 @@ static const struct dmi_system_id gmin_vars[] = {
},
.driver_data = i8880_vars,
},
+ {
+ .ident = "Surface 3",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_NAME, "Surface 3"),
+ },
+ .driver_data = surface3_vars,
+ },
{}
};
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB)
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (14 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 15/17] media: atomsip: pci: add Microsoft Surface 3 ACPI vars Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-18 7:56 ` Hans de Goede
2021-10-18 7:56 ` Hans de Goede
2021-10-17 16:19 ` [PATCH 17/17] [NOT-FOR-MERGE] atomisp: Fix up the open v load race Tsuchiya Yuto
` (5 subsequent siblings)
21 siblings, 2 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Aniket Bhattacharyea, Aline Santana Cordeiro, Yang Yingliang,
Hans Verkuil, Alan, Dinghao Liu, linux-media, linux-staging,
linux-kernel
This commit is added for Surface 3 with broken DMI table. HACK-ish.
Not intended for upstreaming. Thus, NOT-FOR-MERGE. But, if someone
knows a nicer way to address this, comments are welcome...
>8-----------------------------------------------------------------8<
On some Microsoft Surface 3, the DMI table gets corrupted for unknown
reasons and breaks existing DMI matching used for device-specific quirks.
This commit adds the (broken) DMI data into dmi_system_id tables used
for quirks so that the driver can enable quirks even on the affected
systems.
On affected systems, the DMI data will look like this:
$ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
chassis_vendor,product_name,sys_vendor}
/sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
/sys/devices/virtual/dmi/id/board_name:OEMB
/sys/devices/virtual/dmi/id/board_vendor:OEMB
/sys/devices/virtual/dmi/id/chassis_vendor:OEMB
/sys/devices/virtual/dmi/id/product_name:OEMB
/sys/devices/virtual/dmi/id/sys_vendor:OEMB
Expected:
$ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
chassis_vendor,product_name,sys_vendor}
/sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
/sys/devices/virtual/dmi/id/board_name:Surface 3
/sys/devices/virtual/dmi/id/board_vendor:Microsoft Corporation
/sys/devices/virtual/dmi/id/chassis_vendor:Microsoft Corporation
/sys/devices/virtual/dmi/id/product_name:Surface 3
/sys/devices/virtual/dmi/id/sys_vendor:Microsoft Corporation
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
.../staging/media/atomisp/pci/atomisp_gmin_platform.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 948eb6f809f5..3868d64cbc2b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -377,6 +377,17 @@ static const struct dmi_system_id gmin_vars[] = {
},
.driver_data = surface3_vars,
},
+ {
+ .ident = "Surface 3",
+ .matches = {
+ /* DMI data for Surface 3 with broken DMI table */
+ DMI_MATCH(DMI_BIOS_VENDOR, "American Megatrends Inc."),
+ DMI_MATCH(DMI_BOARD_NAME, "OEMB"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "OEMB"),
+ DMI_MATCH(DMI_SYS_VENDOR, "OEMB"),
+ },
+ .driver_data = surface3_vars,
+ },
{}
};
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB)
2021-10-17 16:19 ` [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB) Tsuchiya Yuto
@ 2021-10-18 7:56 ` Hans de Goede
2021-10-21 9:46 ` Tsuchiya Yuto
2021-10-18 7:56 ` Hans de Goede
1 sibling, 1 reply; 79+ messages in thread
From: Hans de Goede @ 2021-10-18 7:56 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Aniket Bhattacharyea, Aline Santana Cordeiro,
Yang Yingliang, Hans Verkuil, Alan, Dinghao Liu, linux-media,
linux-staging, linux-kernel
Hi,
On 10/17/21 18:19, Tsuchiya Yuto wrote:
> This commit is added for Surface 3 with broken DMI table. HACK-ish.
> Not intended for upstreaming. Thus, NOT-FOR-MERGE. But, if someone
> knows a nicer way to address this, comments are welcome...
>
>> 8-----------------------------------------------------------------8<
>
> On some Microsoft Surface 3, the DMI table gets corrupted for unknown
> reasons and breaks existing DMI matching used for device-specific quirks.
>
> This commit adds the (broken) DMI data into dmi_system_id tables used
> for quirks so that the driver can enable quirks even on the affected
> systems.
>
> On affected systems, the DMI data will look like this:
>
> $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
> chassis_vendor,product_name,sys_vendor}
> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> /sys/devices/virtual/dmi/id/board_name:OEMB
> /sys/devices/virtual/dmi/id/board_vendor:OEMB
> /sys/devices/virtual/dmi/id/chassis_vendor:OEMB
> /sys/devices/virtual/dmi/id/product_name:OEMB
> /sys/devices/virtual/dmi/id/sys_vendor:OEMB
I wonder what the bios_date field contains ? Typically when the DMI strings
are no good (e.g. often they contain "Default String" or "To be filled by OEM")
we add a check on the bios-date, which together with the broken strings is
considered unique enough to still allow a match with broken strings in the
kernel.
Also have you tried doing something like "load bios/setup defaults" in
the BIOS setup ? Maybe that helps ?
Regards,
Hans
>
> Expected:
>
> $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
> chassis_vendor,product_name,sys_vendor}
> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> /sys/devices/virtual/dmi/id/board_name:Surface 3
> /sys/devices/virtual/dmi/id/board_vendor:Microsoft Corporation
> /sys/devices/virtual/dmi/id/chassis_vendor:Microsoft Corporation
> /sys/devices/virtual/dmi/id/product_name:Surface 3
> /sys/devices/virtual/dmi/id/sys_vendor:Microsoft Corporation
>
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> ---
> .../staging/media/atomisp/pci/atomisp_gmin_platform.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 948eb6f809f5..3868d64cbc2b 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -377,6 +377,17 @@ static const struct dmi_system_id gmin_vars[] = {
> },
> .driver_data = surface3_vars,
> },
> + {
> + .ident = "Surface 3",
> + .matches = {
> + /* DMI data for Surface 3 with broken DMI table */
> + DMI_MATCH(DMI_BIOS_VENDOR, "American Megatrends Inc."),
> + DMI_MATCH(DMI_BOARD_NAME, "OEMB"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "OEMB"),
> + DMI_MATCH(DMI_SYS_VENDOR, "OEMB"),
> + },
> + .driver_data = surface3_vars,
> + },
> {}
> };
>
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB)
2021-10-18 7:56 ` Hans de Goede
@ 2021-10-21 9:46 ` Tsuchiya Yuto
2021-10-21 18:46 ` Hans de Goede
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-21 9:46 UTC (permalink / raw)
To: Hans de Goede
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Aniket Bhattacharyea, Aline Santana Cordeiro,
Yang Yingliang, Hans Verkuil, Alan, Dinghao Liu, linux-media,
linux-staging, linux-kernel
On Mon, 2021-10-18 at 09:56 +0200, Hans de Goede wrote:
> Hi,
>
> On 10/17/21 18:19, Tsuchiya Yuto wrote:
> > This commit is added for Surface 3 with broken DMI table. HACK-ish.
> > Not intended for upstreaming. Thus, NOT-FOR-MERGE. But, if someone
> > knows a nicer way to address this, comments are welcome...
> >
> > > 8-----------------------------------------------------------------8<
> >
> > On some Microsoft Surface 3, the DMI table gets corrupted for unknown
> > reasons and breaks existing DMI matching used for device-specific quirks.
> >
> > This commit adds the (broken) DMI data into dmi_system_id tables used
> > for quirks so that the driver can enable quirks even on the affected
> > systems.
> >
> > On affected systems, the DMI data will look like this:
> >
> > $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
> > chassis_vendor,product_name,sys_vendor}
> > /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> > /sys/devices/virtual/dmi/id/board_name:OEMB
> > /sys/devices/virtual/dmi/id/board_vendor:OEMB
> > /sys/devices/virtual/dmi/id/chassis_vendor:OEMB
> > /sys/devices/virtual/dmi/id/product_name:OEMB
> > /sys/devices/virtual/dmi/id/sys_vendor:OEMB
>
> I wonder what the bios_date field contains ? Typically when the DMI strings
> are no good (e.g. often they contain "Default String" or "To be filled by OEM")
> we add a check on the bios-date, which together with the broken strings is
> considered unique enough to still allow a match with broken strings in the
> kernel.
Thank you so much for the comment :-)
Here is the full output of "/sys/devices/virtual/dmi/id/*" (not showing
files that need root permission to read):
$ grep . /sys/devices/virtual/dmi/id/*
/sys/devices/virtual/dmi/id/bios_date:03/09/2015
/sys/devices/virtual/dmi/id/bios_release:5.6
/sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
/sys/devices/virtual/dmi/id/bios_version:1.51116.238
/sys/devices/virtual/dmi/id/board_name:OEMB
grep: /sys/devices/virtual/dmi/id/board_serial: Permission denied
/sys/devices/virtual/dmi/id/board_vendor:OEMB
/sys/devices/virtual/dmi/id/board_version:00
grep: /sys/devices/virtual/dmi/id/chassis_serial: Permission denied
/sys/devices/virtual/dmi/id/chassis_type:9
/sys/devices/virtual/dmi/id/chassis_vendor:OEMB
/sys/devices/virtual/dmi/id/modalias:dmi:bvnAmericanMegatrendsInc.:bvr1.51116.238:bd03/09/2015:br5.6:svnOEMB:pnOEMB:pvrB16D0SM1C4G1X1:rvnOEMB:rnOEMB:rvr00:cvnOEMB:ct9:cvr:sku:
grep: /sys/devices/virtual/dmi/id/power: Is a directory
/sys/devices/virtual/dmi/id/product_name:OEMB
grep: /sys/devices/virtual/dmi/id/product_serial: Permission denied
grep: /sys/devices/virtual/dmi/id/product_uuid: Permission denied
/sys/devices/virtual/dmi/id/product_version:B16D0SM1C4G1X1
grep: /sys/devices/virtual/dmi/id/subsystem: Is a directory
/sys/devices/virtual/dmi/id/sys_vendor:OEMB
/sys/devices/virtual/dmi/id/uevent:MODALIAS=dmi:bvnAmericanMegatrendsInc.:bvr1.51116.238:bd03/09/2015:br5.6:svnOEMB:pnOEMB:pvrB16D0SM1C4G1X1:rvnOEMB:rnOEMB:rvr00:cvnOEMB:ct9:cvr:sku:
The "bios_date" ("03/09/2015") looks not broken.
I also noticed when writing this mail, regarding the ones that need root
permission to read, "board_serial" and "chassis_serial" are now empty.
"product_serial" now shows "OEM":
$ sudo cat /sys/devices/virtual/dmi/id/product_serial
OEM
"product_uuid" looks not broken.
> Also have you tried doing something like "load bios/setup defaults" in
> the BIOS setup ? Maybe that helps ?
Unfortunately, there is no option like this...
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB)
2021-10-21 9:46 ` Tsuchiya Yuto
@ 2021-10-21 18:46 ` Hans de Goede
2021-10-27 14:47 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Hans de Goede @ 2021-10-21 18:46 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Aniket Bhattacharyea, Aline Santana Cordeiro,
Yang Yingliang, Hans Verkuil, Alan, Dinghao Liu, linux-media,
linux-staging, linux-kernel
Hi,
On 10/21/21 11:46, Tsuchiya Yuto wrote:
> On Mon, 2021-10-18 at 09:56 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 10/17/21 18:19, Tsuchiya Yuto wrote:
>>> This commit is added for Surface 3 with broken DMI table. HACK-ish.
>>> Not intended for upstreaming. Thus, NOT-FOR-MERGE. But, if someone
>>> knows a nicer way to address this, comments are welcome...
>>>
>>>> 8-----------------------------------------------------------------8<
>>>
>>> On some Microsoft Surface 3, the DMI table gets corrupted for unknown
>>> reasons and breaks existing DMI matching used for device-specific quirks.
>>>
>>> This commit adds the (broken) DMI data into dmi_system_id tables used
>>> for quirks so that the driver can enable quirks even on the affected
>>> systems.
>>>
>>> On affected systems, the DMI data will look like this:
>>>
>>> $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
>>> chassis_vendor,product_name,sys_vendor}
>>> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
>>> /sys/devices/virtual/dmi/id/board_name:OEMB
>>> /sys/devices/virtual/dmi/id/board_vendor:OEMB
>>> /sys/devices/virtual/dmi/id/chassis_vendor:OEMB
>>> /sys/devices/virtual/dmi/id/product_name:OEMB
>>> /sys/devices/virtual/dmi/id/sys_vendor:OEMB
>>
>> I wonder what the bios_date field contains ? Typically when the DMI strings
>> are no good (e.g. often they contain "Default String" or "To be filled by OEM")
>> we add a check on the bios-date, which together with the broken strings is
>> considered unique enough to still allow a match with broken strings in the
>> kernel.
>
> Thank you so much for the comment :-)
>
> Here is the full output of "/sys/devices/virtual/dmi/id/*" (not showing
> files that need root permission to read):
>
> $ grep . /sys/devices/virtual/dmi/id/*
> /sys/devices/virtual/dmi/id/bios_date:03/09/2015
> /sys/devices/virtual/dmi/id/bios_release:5.6
> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> /sys/devices/virtual/dmi/id/bios_version:1.51116.238
Interesting, this is the latest BIOS from july 2019 according to:
https://support.microsoft.com/en-us/surface/surface-3-update-history-5d86a7bc-03f7-2d27-d858-e90ce637fb52
yet the date is still set to 03/09/2015.
I just checked and the BIOS with not corrupted DMI strings also keeps
the date at 03/09/2015 in BIOS updates.
So the date is correct, and together with matching a coupleof the OEMB-s
(which I've never seen anywhere else either) this should be plenty
unique.
So this not only allows adding this mathc to atomisp, but also to fix
sound + wmi on bad DMI data OEMB Surface 3-s, by updating this patch:
https://github.com/linux-surface/linux-surface/blob/2fb7e9ae91350098db9a280277f424272816a9ab/patches/5.5/0003-surface3-oemb.patch
To include the BIOS-date match and then submitting this upstream
(as 2 separate patches please).
Tsuchiya, I take it that your Surface 3 has the OEMB issue, so you
can actually test this ?
If you can prepare 2 patches for the sound + wmi then; and submit
them upstream that would be great. Please Cc me on both patches.
Regards,
Hans
> /sys/devices/virtual/dmi/id/board_name:OEMB
> grep: /sys/devices/virtual/dmi/id/board_serial: Permission denied
> /sys/devices/virtual/dmi/id/board_vendor:OEMB
> /sys/devices/virtual/dmi/id/board_version:00
> grep: /sys/devices/virtual/dmi/id/chassis_serial: Permission denied
> /sys/devices/virtual/dmi/id/chassis_type:9
> /sys/devices/virtual/dmi/id/chassis_vendor:OEMB
> /sys/devices/virtual/dmi/id/modalias:dmi:bvnAmericanMegatrendsInc.:bvr1.51116.238:bd03/09/2015:br5.6:svnOEMB:pnOEMB:pvrB16D0SM1C4G1X1:rvnOEMB:rnOEMB:rvr00:cvnOEMB:ct9:cvr:sku:
> grep: /sys/devices/virtual/dmi/id/power: Is a directory
> /sys/devices/virtual/dmi/id/product_name:OEMB
> grep: /sys/devices/virtual/dmi/id/product_serial: Permission denied
> grep: /sys/devices/virtual/dmi/id/product_uuid: Permission denied
> /sys/devices/virtual/dmi/id/product_version:B16D0SM1C4G1X1
> grep: /sys/devices/virtual/dmi/id/subsystem: Is a directory
> /sys/devices/virtual/dmi/id/sys_vendor:OEMB
> /sys/devices/virtual/dmi/id/uevent:MODALIAS=dmi:bvnAmericanMegatrendsInc.:bvr1.51116.238:bd03/09/2015:br5.6:svnOEMB:pnOEMB:pvrB16D0SM1C4G1X1:rvnOEMB:rnOEMB:rvr00:cvnOEMB:ct9:cvr:sku:
>
> The "bios_date" ("03/09/2015") looks not broken.
>
> I also noticed when writing this mail, regarding the ones that need root
> permission to read, "board_serial" and "chassis_serial" are now empty.
> "product_serial" now shows "OEM":
>
> $ sudo cat /sys/devices/virtual/dmi/id/product_serial
> OEM
>
> "product_uuid" looks not broken.
>
>> Also have you tried doing something like "load bios/setup defaults" in
>> the BIOS setup ? Maybe that helps ?
>
> Unfortunately, there is no option like this...
>
> Regards,
> Tsuchiya Yuto
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB)
2021-10-21 18:46 ` Hans de Goede
@ 2021-10-27 14:47 ` Tsuchiya Yuto
2021-10-27 15:30 ` Hans de Goede
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-27 14:47 UTC (permalink / raw)
To: Hans de Goede
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Aniket Bhattacharyea, Aline Santana Cordeiro,
Yang Yingliang, Hans Verkuil, Alan, Dinghao Liu, linux-media,
linux-staging, linux-kernel
On Thu, 2021-10-21 at 20:46 +0200, Hans de Goede wrote:
> Hi,
>
> On 10/21/21 11:46, Tsuchiya Yuto wrote:
> > On Mon, 2021-10-18 at 09:56 +0200, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 10/17/21 18:19, Tsuchiya Yuto wrote:
> > > > This commit is added for Surface 3 with broken DMI table. HACK-ish.
> > > > Not intended for upstreaming. Thus, NOT-FOR-MERGE. But, if someone
> > > > knows a nicer way to address this, comments are welcome...
> > > >
> > > > > 8-----------------------------------------------------------------8<
> > > >
> > > > On some Microsoft Surface 3, the DMI table gets corrupted for unknown
> > > > reasons and breaks existing DMI matching used for device-specific quirks.
> > > >
> > > > This commit adds the (broken) DMI data into dmi_system_id tables used
> > > > for quirks so that the driver can enable quirks even on the affected
> > > > systems.
> > > >
> > > > On affected systems, the DMI data will look like this:
> > > >
> > > > $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
> > > > chassis_vendor,product_name,sys_vendor}
> > > > /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> > > > /sys/devices/virtual/dmi/id/board_name:OEMB
> > > > /sys/devices/virtual/dmi/id/board_vendor:OEMB
> > > > /sys/devices/virtual/dmi/id/chassis_vendor:OEMB
> > > > /sys/devices/virtual/dmi/id/product_name:OEMB
> > > > /sys/devices/virtual/dmi/id/sys_vendor:OEMB
> > >
> > > I wonder what the bios_date field contains ? Typically when the DMI strings
> > > are no good (e.g. often they contain "Default String" or "To be filled by OEM")
> > > we add a check on the bios-date, which together with the broken strings is
> > > considered unique enough to still allow a match with broken strings in the
> > > kernel.
> >
> > Thank you so much for the comment :-)
> >
> > Here is the full output of "/sys/devices/virtual/dmi/id/*" (not showing
> > files that need root permission to read):
> >
> > $ grep . /sys/devices/virtual/dmi/id/*
> > /sys/devices/virtual/dmi/id/bios_date:03/09/2015
> > /sys/devices/virtual/dmi/id/bios_release:5.6
> > /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> > /sys/devices/virtual/dmi/id/bios_version:1.51116.238
>
> Interesting, this is the latest BIOS from july 2019 according to:
> https://support.microsoft.com/en-us/surface/surface-3-update-history-5d86a7bc-03f7-2d27-d858-e90ce637fb52
> yet the date is still set to 03/09/2015.
Yeah, I'm a little bit confused about this.
> I just checked and the BIOS with not corrupted DMI strings also keeps
> the date at 03/09/2015 in BIOS updates.
>
> So the date is correct, and together with matching a coupleof the OEMB-s
> (which I've never seen anywhere else either) this should be plenty
> unique.
>
> So this not only allows adding this mathc to atomisp, but also to fix
> sound + wmi on bad DMI data OEMB Surface 3-s, by updating this patch:
>
> https://github.com/linux-surface/linux-surface/blob/2fb7e9ae91350098db9a280277f424272816a9ab/patches/5.5/0003-surface3-oemb.patch
>
> To include the BIOS-date match and then submitting this upstream
> (as 2 separate patches please).
>
> Tsuchiya, I take it that your Surface 3 has the OEMB issue, so you
> can actually test this ?
Yes, my surface3 is also affected and I can test this.
> If you can prepare 2 patches for the sound + wmi then; and submit
> them upstream that would be great. Please Cc me on both patches.
Thank you for the suggestion, but I started having a mixed feeling about
sending this kind of patches... This "OEMB" issue is not a design by
manufacturers, but simply just it got broken after something (maybe a
force power off?). On the other hand, I know there are also indeed some
people affected by this issue other than me...
If possible, I rather want to fix this broken DMI table, but I couldn't
find the way until now though.
But again, thank you for the suggestion. I'll consider sending the
patches when I gave up fixing it...
<Below is completely off topic from atomisp>
I think some useful BIOS option might be just hidden. So, I'd like to
try this way. I already find the string "Restore Defaults" using
uefitool/ifrextract:
0x13429 Form: Save & Exit, FormId: 0x2719 {01 86 19 27 4C 00}
[...]
0x134E0 Suppress If {0A 82}
0x134E2 QuestionId: 0x1C3 equals value 0x5 {12 06 C3 01 05 00}
0x134E8 Ref: Restore Defaults, VarStoreInfo (VarOffset/VarName): 0xFFFF, VarStore: 0x0, QuestionId: 0x1BC, FormId: 0x2719 {0F 0F 5B 00 5C 00 BC 01 00 00 FF FF 00 19 27}
0x134F7 End If {29 02}
[...]
I currently don't know how I can call this. I want to try this way when
I have some time...
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB)
2021-10-27 14:47 ` Tsuchiya Yuto
@ 2021-10-27 15:30 ` Hans de Goede
0 siblings, 0 replies; 79+ messages in thread
From: Hans de Goede @ 2021-10-27 15:30 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Aniket Bhattacharyea, Aline Santana Cordeiro,
Yang Yingliang, Hans Verkuil, Alan, Dinghao Liu, linux-media,
linux-staging, linux-kernel
Hi,
On 10/27/21 16:47, Tsuchiya Yuto wrote:
> On Thu, 2021-10-21 at 20:46 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 10/21/21 11:46, Tsuchiya Yuto wrote:
>>> On Mon, 2021-10-18 at 09:56 +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 10/17/21 18:19, Tsuchiya Yuto wrote:
>>>>> This commit is added for Surface 3 with broken DMI table. HACK-ish.
>>>>> Not intended for upstreaming. Thus, NOT-FOR-MERGE. But, if someone
>>>>> knows a nicer way to address this, comments are welcome...
>>>>>
>>>>>> 8-----------------------------------------------------------------8<
>>>>>
>>>>> On some Microsoft Surface 3, the DMI table gets corrupted for unknown
>>>>> reasons and breaks existing DMI matching used for device-specific quirks.
>>>>>
>>>>> This commit adds the (broken) DMI data into dmi_system_id tables used
>>>>> for quirks so that the driver can enable quirks even on the affected
>>>>> systems.
>>>>>
>>>>> On affected systems, the DMI data will look like this:
>>>>>
>>>>> $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
>>>>> chassis_vendor,product_name,sys_vendor}
>>>>> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
>>>>> /sys/devices/virtual/dmi/id/board_name:OEMB
>>>>> /sys/devices/virtual/dmi/id/board_vendor:OEMB
>>>>> /sys/devices/virtual/dmi/id/chassis_vendor:OEMB
>>>>> /sys/devices/virtual/dmi/id/product_name:OEMB
>>>>> /sys/devices/virtual/dmi/id/sys_vendor:OEMB
>>>>
>>>> I wonder what the bios_date field contains ? Typically when the DMI strings
>>>> are no good (e.g. often they contain "Default String" or "To be filled by OEM")
>>>> we add a check on the bios-date, which together with the broken strings is
>>>> considered unique enough to still allow a match with broken strings in the
>>>> kernel.
>>>
>>> Thank you so much for the comment :-)
>>>
>>> Here is the full output of "/sys/devices/virtual/dmi/id/*" (not showing
>>> files that need root permission to read):
>>>
>>> $ grep . /sys/devices/virtual/dmi/id/*
>>> /sys/devices/virtual/dmi/id/bios_date:03/09/2015
>>> /sys/devices/virtual/dmi/id/bios_release:5.6
>>> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
>>> /sys/devices/virtual/dmi/id/bios_version:1.51116.238
>>
>> Interesting, this is the latest BIOS from july 2019 according to:
>> https://support.microsoft.com/en-us/surface/surface-3-update-history-5d86a7bc-03f7-2d27-d858-e90ce637fb52
>> yet the date is still set to 03/09/2015.
>
> Yeah, I'm a little bit confused about this.
Yes this is unusual, bit it just seems to be how Microsoft does things
on the Surface 3.
>> I just checked and the BIOS with not corrupted DMI strings also keeps
>> the date at 03/09/2015 in BIOS updates.
>>
>> So the date is correct, and together with matching a coupleof the OEMB-s
>> (which I've never seen anywhere else either) this should be plenty
>> unique.
>>
>> So this not only allows adding this mathc to atomisp, but also to fix
>> sound + wmi on bad DMI data OEMB Surface 3-s, by updating this patch:
>>
>> https://github.com/linux-surface/linux-surface/blob/2fb7e9ae91350098db9a280277f424272816a9ab/patches/5.5/0003-surface3-oemb.patch
>>
>> To include the BIOS-date match and then submitting this upstream
>> (as 2 separate patches please).
>>
>> Tsuchiya, I take it that your Surface 3 has the OEMB issue, so you
>> can actually test this ?
>
> Yes, my surface3 is also affected and I can test this.
>
>> If you can prepare 2 patches for the sound + wmi then; and submit
>> them upstream that would be great. Please Cc me on both patches.
>
> Thank you for the suggestion, but I started having a mixed feeling about
> sending this kind of patches... This "OEMB" issue is not a design by
> manufacturers, but simply just it got broken after something (maybe a
> force power off?). On the other hand, I know there are also indeed some
> people affected by this issue other than me...
>
> If possible, I rather want to fix this broken DMI table, but I couldn't
> find the way until now though.
>
> But again, thank you for the suggestion. I'll consider sending the
> patches when I gave up fixing it...
Since other people are affected too, fixing this is only really useful
if the fix is doable by others too.
OTOH I just checked:
https://github.com/linuxhw/DMI
Which is a database of all dmidecode-s uploaded to:
https://linux-hardware.org/
And _zero_ of the 56358 dmidecode outputs available
there contains OEMB, so this seems to be quite unique to the
Surface 3. Which, esp. together with the BIOS date makes the DMI
match definitely unique enough. Sometimes DMI strings change
on a BIOS update (not break like this, but just change) so
having 2 entries for a single device-model is not unheard of.
So my cents is to just go with adding the extra entry to
dmi_system_id tables and to not spend too much time on this.
> <Below is completely off topic from atomisp>
>
> I think some useful BIOS option might be just hidden. So, I'd like to
> try this way. I already find the string "Restore Defaults" using
> uefitool/ifrextract:
>
> 0x13429 Form: Save & Exit, FormId: 0x2719 {01 86 19 27 4C 00}
> [...]
> 0x134E0 Suppress If {0A 82}
> 0x134E2 QuestionId: 0x1C3 equals value 0x5 {12 06 C3 01 05 00}
> 0x134E8 Ref: Restore Defaults, VarStoreInfo (VarOffset/VarName): 0xFFFF, VarStore: 0x0, QuestionId: 0x1BC, FormId: 0x2719 {0F 0F 5B 00 5C 00 BC 01 00 00 FF FF 00 19 27}
> 0x134F7 End If {29 02}
> [...]
>
> I currently don't know how I can call this. I want to try this way when
> I have some time...
So this is not a normal hidden item which just changes a setting (for which
there are known ways to change the setting without flashing the BIOS).
But this is some sorta action item which can only be enabled by flashing
a custom BIOS, assuming we can somehow replace the Surface 3 custom
setup menu with the standard IFR based menu, which in itself is a problem...
Regards,
Hans
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB)
2021-10-17 16:19 ` [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB) Tsuchiya Yuto
2021-10-18 7:56 ` Hans de Goede
@ 2021-10-18 7:56 ` Hans de Goede
1 sibling, 0 replies; 79+ messages in thread
From: Hans de Goede @ 2021-10-18 7:56 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Aniket Bhattacharyea, Aline Santana Cordeiro,
Yang Yingliang, Hans Verkuil, Alan, Dinghao Liu, linux-media,
linux-staging, linux-kernel
Hi,
On 10/17/21 18:19, Tsuchiya Yuto wrote:
> This commit is added for Surface 3 with broken DMI table. HACK-ish.
> Not intended for upstreaming. Thus, NOT-FOR-MERGE. But, if someone
> knows a nicer way to address this, comments are welcome...
>
>> 8-----------------------------------------------------------------8<
>
> On some Microsoft Surface 3, the DMI table gets corrupted for unknown
> reasons and breaks existing DMI matching used for device-specific quirks.
>
> This commit adds the (broken) DMI data into dmi_system_id tables used
> for quirks so that the driver can enable quirks even on the affected
> systems.
>
> On affected systems, the DMI data will look like this:
>
> $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
> chassis_vendor,product_name,sys_vendor}
> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> /sys/devices/virtual/dmi/id/board_name:OEMB
> /sys/devices/virtual/dmi/id/board_vendor:OEMB
> /sys/devices/virtual/dmi/id/chassis_vendor:OEMB
> /sys/devices/virtual/dmi/id/product_name:OEMB
> /sys/devices/virtual/dmi/id/sys_vendor:OEMB
I wonder what the bios_date field contains ? Typically when the DMI strings
are no good (e.g. often they contain "Default String" or "To be filled by OEM")
we add a check on the bios-date, which together with the broken strings is
considered unique enough to still allow a match with broken strings in the
kernel.
Also have you tried doing something like "load bios/setup defaults" in
the BIOS setup ? Maybe that helps ?
Regards,
Hans
>
> Expected:
>
> $ grep . /sys/devices/virtual/dmi/id/{bios_vendor,board_name,board_vendor,\
> chassis_vendor,product_name,sys_vendor}
> /sys/devices/virtual/dmi/id/bios_vendor:American Megatrends Inc.
> /sys/devices/virtual/dmi/id/board_name:Surface 3
> /sys/devices/virtual/dmi/id/board_vendor:Microsoft Corporation
> /sys/devices/virtual/dmi/id/chassis_vendor:Microsoft Corporation
> /sys/devices/virtual/dmi/id/product_name:Surface 3
> /sys/devices/virtual/dmi/id/sys_vendor:Microsoft Corporation
>
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> ---
> .../staging/media/atomisp/pci/atomisp_gmin_platform.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 948eb6f809f5..3868d64cbc2b 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -377,6 +377,17 @@ static const struct dmi_system_id gmin_vars[] = {
> },
> .driver_data = surface3_vars,
> },
> + {
> + .ident = "Surface 3",
> + .matches = {
> + /* DMI data for Surface 3 with broken DMI table */
> + DMI_MATCH(DMI_BIOS_VENDOR, "American Megatrends Inc."),
> + DMI_MATCH(DMI_BOARD_NAME, "OEMB"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "OEMB"),
> + DMI_MATCH(DMI_SYS_VENDOR, "OEMB"),
> + },
> + .driver_data = surface3_vars,
> + },
> {}
> };
>
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH 17/17] [NOT-FOR-MERGE] atomisp: Fix up the open v load race
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (15 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 16/17] [NOT-FOR-MERGE] media: atomsip: pci: add DMI match for Microsoft Surface 3 with broken DMI (OEMB) Tsuchiya Yuto
@ 2021-10-17 16:19 ` Tsuchiya Yuto
2021-10-18 7:48 ` [PATCH 00/17] various fixes for atomisp to make it work Hans de Goede
` (4 subsequent siblings)
21 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-17 16:19 UTC (permalink / raw)
Cc: Hans de Goede, Patrik Gfeller, Alan, Tsuchiya Yuto,
Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
Hans Verkuil, Laurent Pinchart, Arnd Bergmann, Tomi Valkeinen,
Yang Yingliang, Aline Santana Cordeiro, Dinghao Liu, linux-media,
linux-staging, linux-kernel
From: Alan <alan@linux.intel.com>
Applied this patch as-is for now to pass firmware load on probe.
I haven't looked into this and not written by me anyway.
Thus, NOT-FOR-MERGE. At least I can say this patch works just fine.
>8-----------------------------------------------------------------8<
From: Alan <alan@linux.intel.com>
Patch from https://lore.kernel.org/linux-media/151001137594.77201.4306351721772580664.stgit@alans-desktop/raw
Here is the original commit message:
Subject: [PATCH 1/3] atomisp: Fix up the open v load race
From: Alan <alan@linux.intel.com>
Date: Mon, 06 Nov 2017 23:36:36 +0000
This isn't the ideal final solution but it stops the main problem for now
where an open (often from udev) races the device initialization and we try
and load the firmware twice at the same time. This needless to say doesn't
usually end well.
Signed-off-by: Alan Cox <alan@linux.intel.com>
This patch fixes the following "css init failed" error related to
firmware load. This issue often occurs when I insmod the module after
boot.
kern :err : [ 234.658869] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.755023] atomisp-isp2 0000:00:03.0: can't change power state from D0 to D3hot (config space inaccessible)
kern :err : [ 234.755053] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.755581] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.756146] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.757115] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.757582] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.758885] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.760317] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.760818] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.769102] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.769527] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.771353] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.771804] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.772299] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.774617] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.775090] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.775607] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.776230] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.779059] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
kern :err : [ 234.787726] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :err : [ 234.788078] atomisp-isp2 0000:00:03.0: css init failed --- bad firmware?
Patch is adapted to v5.15-rc4.
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
drivers/staging/media/atomisp/pci/atomisp_fops.c | 12 ++++++++++++
drivers/staging/media/atomisp/pci/atomisp_internal.h | 5 +++++
drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 6 ++++++
3 files changed, 23 insertions(+)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index f82bf082aa79..62535e0f4429 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -772,6 +772,18 @@ static int atomisp_open(struct file *file)
dev_dbg(isp->dev, "open device %s\n", vdev->name);
+ /* Ensure that if we are still loading we block. Once the loading
+ is over we can proceed. We can't blindly hold the lock until
+ that occurs as if the load fails we'll deadlock the unload */
+ rt_mutex_lock(&isp->loading);
+ /* Revisit this with a better check once the code structure is
+ cleaned up a bit more FIXME */
+ if (!isp->ready) {
+ rt_mutex_unlock(&isp->loading);
+ return -ENXIO;
+ }
+ rt_mutex_unlock(&isp->loading);
+
rt_mutex_lock(&isp->mutex);
acc_node = !strcmp(vdev->name, "ATOMISP ISP ACC");
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index c01db10bb735..356a7f1bb757 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -246,6 +246,11 @@ struct atomisp_device {
/* Purpose of mutex is to protect and serialize use of isp data
* structures and css API calls. */
struct rt_mutex mutex;
+ /* This mutex ensures that we don't allow an open to succeed while
+ * the initialization process is incomplete */
+ struct rt_mutex loading;
+ /* Set once the ISP is ready to allow opens */
+ bool ready;
/*
* Serialise streamoff: mutex is dropped during streamoff to
* cancel the watchdog queue. MUST be acquired BEFORE
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 720963156d24..e2ccace77a73 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1567,6 +1567,7 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
dev_dbg(&pdev->dev, "atomisp mmio base: %p\n", isp->base);
rt_mutex_init(&isp->mutex);
+ rt_mutex_init(&isp->loading);
mutex_init(&isp->streamoff_mutex);
spin_lock_init(&isp->lock);
@@ -1749,6 +1750,8 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
pci_write_config_dword(pdev, MRFLD_PCI_CSI_AFE_TRIM_CONTROL, csi_afe_trim);
}
+ rt_mutex_lock(&isp->loading);
+
err = atomisp_initialize_modules(isp);
if (err < 0) {
dev_err(&pdev->dev, "atomisp_initialize_modules (%d)\n", err);
@@ -1806,6 +1809,8 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
release_firmware(isp->firmware);
isp->firmware = NULL;
isp->css_env.isp_css_fw.data = NULL;
+ isp->ready = true;
+ rt_mutex_unlock(&isp->loading);
atomisp_drvfs_init(isp);
@@ -1825,6 +1830,7 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
register_entities_fail:
atomisp_uninitialize_modules(isp);
initialize_modules_fail:
+ rt_mutex_unlock(&isp->loading);
cpu_latency_qos_remove_request(&isp->pm_qos);
atomisp_msi_irq_uninit(isp);
pci_free_irq_vectors(pdev);
--
2.33.1
^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (16 preceding siblings ...)
2021-10-17 16:19 ` [PATCH 17/17] [NOT-FOR-MERGE] atomisp: Fix up the open v load race Tsuchiya Yuto
@ 2021-10-18 7:48 ` Hans de Goede
2021-10-19 13:50 ` Tsuchiya Yuto
2021-10-18 7:56 ` Andy Shevchenko
` (3 subsequent siblings)
21 siblings, 1 reply; 79+ messages in thread
From: Hans de Goede @ 2021-10-18 7:48 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, Alan, linux-media,
linux-staging, linux-kernel, Nable, Andy Shevchenko, Fabio Aiuto,
andrey.i.trufanov
<Added some more people to the Cc who are also definitely interested in this>
Hi Tsuchiya,
On 10/17/21 18:19, Tsuchiya Yuto wrote:
> Hi all,
>
> This patch series contains fixes for atomisp to work (again). Tested on
> Microsoft Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with
> v5.15-rc5. Both are Cherry Trail (ISP2401) devices.
>
> I'm still not used to Linux patch sending flow. Sorry in advance
> if there is some weirdness :-) but I did my best.
You actually did pretty good AFAICT.
Also thank you so much for working on this. Finally getting working
atomisp2 support is awesome!
I actually have been working on and off on this myself
(even did a bit of work on it this weekend). My plan was to:
1. Find android (5.1) x86 kernel sources which I can build from
source and get a working Android (on a device with Android pre-installed)
with a kernel build from source -> Done
2. Get a regular Linux distro to boot with the kernel from 1. -> Done
(normal Linux v4l2 utils do not give a picture, might be the need
to set preview mode)
3. Add ioctl debugging to the kernel from 1. capture a trace to see what
Android userspace is doing -> Done . See here for an example log:
https://fedorapeople.org/~jwrdegoede/atomisp-logs-t116/
4. Write a userspace utility mimicking Android userspace, I started on
this this weekend
5. Once I've 4. working the plan was a bit vague, the idea was to see if
I could use that to quickly get the mainline staging version to work; or
alternatively forward port the working Android kernel sources to
mainline from scratch.
But it looks like 4 and 5 will no longer be necessary, which is awesome,
thank you so much!
I'll try to look into this series in more detail; and try to get thing
working on one of my own devices when I can make some time for this.
>
> I'll send another series that contains RFC patches later named ("bug
> reports for atomisp to make it work"). To try to capture images, applying
> those RFC patches are also needed.
>
> The following 1st-7th patches are fixes for the upstreamed atomisp:
>
> media: atomisp: pci: add missing media_device_cleanup() in
> atomisp_unregister_entities()
> media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for
> mrfld_power up case
> media: atomisp: pci: fix inverted logic in buffers_needed()
> media: atomisp: pci: do not use err var when checking port validity
> for ISP2400
> media: atomisp: pci: fix inverted error check for
> ia_css_mipi_is_source_port_valid()
> media: atomisp: pci: use IA_CSS_ERROR() for error messages in
> sh_css_mipi.c
> media: atomisp: pci: fix ifdefs in sh_css.c
>
> The following 8th-13th patches partially reverts driver version back
> to irci_stable_candrpv_0415_20150521_0458:
>
> media: atomisp: pci: make fw ver
> irci_stable_candrpv_0415_20150521_0458 work - 1/5
> media: atomisp: pci: make fw ver
> irci_stable_candrpv_0415_20150521_0458 work - 2/5
> media: atomisp: pci: make fw ver
> irci_stable_candrpv_0415_20150521_0458 work - 3/5
> media: atomisp: pci: make fw ver
> irci_stable_candrpv_0415_20150521_0458 work - 4/5
> media: atomisp: pci: make fw ver
> irci_stable_candrpv_0415_20150521_0458 work - 5/5
> media: atomisp: pci: release_version is now
> irci_stable_candrpv_0415_20150521_0458
>
> One of the issues on the upstreamed atomisp is, the driver is a result
> of the following two different versions of driver merged by tools using
> `ifdef ISP2401`:
>
> - ISP2400: irci_stable_candrpv_0415_20150521_0458
> - ISP2401: irci_master_20150911_0724
>
> and we don't have such firmware made for irci_master_20150911_0724.
Right this is something which I also noticed (but I did not do anything with /
about yet)
> I confirmed that the driver version irci_stable_candrpv_0415_20150521_0458
> works well on the intel-aero version atomisp for ISP2401, too.
"irci_stable_candrpv_0415_20150521_0458" is also the version of the atomisp
firmware shipped in CHT tablets which come with Android pre-installed. So
I agree that this is the version which we should go for.
> Here is
> my port, if someone is interested [2]:
>
> So, eventually, such ISP version tests caused by just the driver version
> difference can be removed (not just being unified but removed).
Ack.
>
> That said, it may take longer time until we remove such tests. So, for
> now I thought it may be better to focus on just making atomisp work by
> partially reverting the incompatible things for the firmware version
> irci_stable_candrpv_0415_20150521_0458.
Ack.
<snip>
> ## about (a lot of) ISP2401 ifdef tests
>
> When porting intel-aero version atomisp to mainline, I thought almost
> all the `ifdef ISP2401` things are the result of two different driver
> version merged by tools.
>
> To confirm that, I tried removing ifdefs for the initial commit of
> upstreamed atomisp [1]. And I can successfully take a picture there on
> surface3.
>
> Currently, I can remove ifdefs up to commit bd674b5a413c ("media: atomisp:
> cleanup ifdefs from ia_css_debug.c") [2] which is before 641c2292bf19 ("media:
> atomisp: get rid of version-dependent globals"). Up to there, I stopped
> and realized it may take some time to remove ifdefs for current atomisp.
> So, instead of removing ifdefs, I partially reverted incompatible parts
> in this series for now.
>
> The ifdefs for the real hardware difference is like the following which
> were removed or integrated into `ifdef ISP2401` on commit 641c2292bf19
> ("media: atomisp: get rid of version-dependent globals") and bd674b5a413c
> ("media: atomisp: cleanup ifdefs from ia_css_debug.c"):
>
> - HAS_NO_INPUT_FORMATTER
> - USE_INPUT_SYSTEM_VERSION_2
> - USE_INPUT_SYSTEM_VERSION_2401
> ...
>
> I need to elaborate on this ifdef thing later (and I'll do later), but
> for now, let's focus on make it just work...
Just focusing on making it work sounds good to me. I also have quite a few
Bay Trail devices, so I would love to also see the 2400 version working.
But one step at a time lets focus on CHT / the 2401 for now.
> For devices which use intel_pmic_bytcrc driver, you need to add i2c
> address. I sent RFC patch earlier named ("ACPI / PMIC: Add i2c address
> to intel_pmic_bytcrc driver").
I'll reply to that patch in its own thread (it needs some work,
but we should be able to get something ready for upstream easily).
> Also, sensor drivers are not upstream. Take a look at my working tree
> if someone is interested [1].
>
> I made world-facing camera (OV8835) work, which the driver is from the
> old Android kernel tree. Unfortunately, the user-facing camera (AR0330)
> is not working yet; the output is completely black. I'm not sure why,
> maybe the sensor power issue (atomisp_gmin_platform) or sensor driver
> issue, which the driver is from non-atomisp driver.
>
> [1] https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp
>
> ## for mipad2 (and whiskey cove pmic based devices)
>
> For devices which equip whiskey cove PMIC, you need to add non-upstream
> regulator driver [1].
>
> [1] work done by jekhor, which seems to be from intel-aero or old
> Android kernel
> https://github.com/jekhor/yogabook-linux-kernel/commit/11c05b365fb2eeb4fced5aa66b362c511be32a34
> ("intel_soc_pmic_chtwc: Add regulator driver and definition for VPROG1B")
Interesting I recently bought a 2nd hand mipad2 myself too. I still need
to put Linux on there. I'm definitely motivated to do that now :)
Regards,
Hans
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-18 7:48 ` [PATCH 00/17] various fixes for atomisp to make it work Hans de Goede
@ 2021-10-19 13:50 ` Tsuchiya Yuto
2021-10-19 16:36 ` Andy Shevchenko
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-19 13:50 UTC (permalink / raw)
To: Hans de Goede
Cc: Patrik Gfeller, Mauro Carvalho Chehab, Sakari Ailus,
Greg Kroah-Hartman, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, linux-media, linux-staging,
linux-kernel, Nable, Andy Shevchenko, Fabio Aiuto,
andrey.i.trufanov
On Mon, 2021-10-18 at 09:48 +0200, Hans de Goede wrote:
> <Added some more people to the Cc who are also definitely interested in this>
[removed Alan from Cc as not reachable]
As I also mentioned on another thread, sorry, I failed to add people
to Cc who I should have definitly added...
And I might have sent all of the emails to people who I should not
by blindly using `scripts/get_maintainer.pl` for the first patch with
`--cc-cmd`...
Sorry if that's the case for some of you...
> Hi Tsuchiya,
>
> On 10/17/21 18:19, Tsuchiya Yuto wrote:
> > Hi all,
> >
> > This patch series contains fixes for atomisp to work (again). Tested on
> > Microsoft Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with
> > v5.15-rc5. Both are Cherry Trail (ISP2401) devices.
> >
> > I'm still not used to Linux patch sending flow. Sorry in advance
> > if there is some weirdness :-) but I did my best.
>
> You actually did pretty good AFAICT.
>
> Also thank you so much for working on this. Finally getting working
> atomisp2 support is awesome!
>
> I actually have been working on and off on this myself
> (even did a bit of work on it this weekend).
Thanks! and I'm glad you're also interested in this!
> 1. Find android (5.1) x86 kernel sources which I can build from
> source and get a working Android (on a device with Android pre-installed)
> with a kernel build from source -> Done
>
> 2. Get a regular Linux distro to boot with the kernel from 1. -> Done
> (normal Linux v4l2 utils do not give a picture, might be the need
> to set preview mode)
>
> 3. Add ioctl debugging to the kernel from 1. capture a trace to see what
> Android userspace is doing -> Done . See here for an example log:
>
> https://fedorapeople.org/~jwrdegoede/atomisp-logs-t116/
>
> 4. Write a userspace utility mimicking Android userspace, I started on
> this this weekend
What I did is rather, as soon as I set up chroot environment on Android
kernel (mipad2), I gave up writing userspace tools by myself and searched
for an existing ones :D
> 5. Once I've 4. working the plan was a bit vague, the idea was to see if
> I could use that to quickly get the mainline staging version to work; or
> alternatively forward port the working Android kernel sources to
> mainline from scratch.
>
> But it looks like 4 and 5 will no longer be necessary, which is awesome,
> thank you so much!
>
> I'll try to look into this series in more detail; and try to get thing
> working on one of my own devices when I can make some time for this.
Thanks. It's great if this series is tested by many upstream devs.
>
> >
> > [...]
> >
> > One of the issues on the upstreamed atomisp is, the driver is a result
> > of the following two different versions of driver merged by tools using
> > `ifdef ISP2401`:
> >
> > - ISP2400: irci_stable_candrpv_0415_20150521_0458
> > - ISP2401: irci_master_20150911_0724
> >
> > and we don't have such firmware made for irci_master_20150911_0724.
>
> Right this is something which I also noticed (but I did not do anything with /
> about yet)
>
> > I confirmed that the driver version irci_stable_candrpv_0415_20150521_0458
> > works well on the intel-aero version atomisp for ISP2401, too.
>
> "irci_stable_candrpv_0415_20150521_0458" is also the version of the atomisp
> firmware shipped in CHT tablets which come with Android pre-installed. So
> I agree that this is the version which we should go for.
OK.
> > [...]
> >
> > ## for mipad2 (and whiskey cove pmic based devices)
> >
> > For devices which equip whiskey cove PMIC, you need to add non-upstream
> > regulator driver [1].
> >
> > [1] work done by jekhor, which seems to be from intel-aero or old
> > Android kernel
> > https://github.com/jekhor/yogabook-linux-kernel/commit/11c05b365fb2eeb4fced5aa66b362c511be32a34
> > ("intel_soc_pmic_chtwc: Add regulator driver and definition for VPROG1B")
>
> Interesting I recently bought a 2nd hand mipad2 myself too. I still need
> to put Linux on there. I'm definitely motivated to do that now :)
I'm glad to hear that you also got a mipad2 :) It might be a interesting
device to look into. It even won't boot without nomodeset, no battery
charging/status on mainline kernel.
By the way, instead of adding whiskey cove regulator driver, we may also
add a "hack" like the other PMIC in atomisp_gmin_platform to control
regulators [1].
It seems that to do so, it needs to "read" value from the PMIC before
writing. So, I'm not sure if this can be achieved easily with the current
mainline kernel though.
[1] https://github.com/MiCode/Xiaomi_Kernel_OpenSource/commit/6204d4b7aeefc4db622f8ac57b87bf2c76c6c8aa
("atomisp_platform: add whiskey cove pmic support")
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-19 13:50 ` Tsuchiya Yuto
@ 2021-10-19 16:36 ` Andy Shevchenko
2021-10-20 13:17 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Andy Shevchenko @ 2021-10-19 16:36 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, linux-media, linux-staging,
linux-kernel, Nable, Fabio Aiuto, andrey.i.trufanov
On Tue, Oct 19, 2021 at 10:50:27PM +0900, Tsuchiya Yuto wrote:
> On Mon, 2021-10-18 at 09:48 +0200, Hans de Goede wrote:
> > On 10/17/21 18:19, Tsuchiya Yuto wrote:
...
> > > ## for mipad2 (and whiskey cove pmic based devices)
> > >
> > > For devices which equip whiskey cove PMIC, you need to add non-upstream
> > > regulator driver [1].
> > >
> > > [1] work done by jekhor, which seems to be from intel-aero or old
> > > Android kernel
> > > https://github.com/jekhor/yogabook-linux-kernel/commit/11c05b365fb2eeb4fced5aa66b362c511be32a34
> > > ("intel_soc_pmic_chtwc: Add regulator driver and definition for VPROG1B")
> >
> > Interesting I recently bought a 2nd hand mipad2 myself too. I still need
> > to put Linux on there. I'm definitely motivated to do that now :)
>
> I'm glad to hear that you also got a mipad2 :) It might be a interesting
> device to look into. It even won't boot without nomodeset, no battery
> charging/status on mainline kernel.
>
> By the way, instead of adding whiskey cove regulator driver, we may also
> add a "hack" like the other PMIC in atomisp_gmin_platform to control
> regulators [1].
I looked briefly into the code and if we indeed need to turn off or on
the regulators it should be a driver.
I don't like having hacks outside of driver/staging to satisfy the one from
the staging.
I.o.w. having a regulator driver is a right thing to do in my opinion.
> It seems that to do so, it needs to "read" value from the PMIC before
> writing. So, I'm not sure if this can be achieved easily with the current
> mainline kernel though.
>
> [1] https://github.com/MiCode/Xiaomi_Kernel_OpenSource/commit/6204d4b7aeefc4db622f8ac57b87bf2c76c6c8aa
> ("atomisp_platform: add whiskey cove pmic support")
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-19 16:36 ` Andy Shevchenko
@ 2021-10-20 13:17 ` Tsuchiya Yuto
0 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-20 13:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, linux-media, linux-staging,
linux-kernel, Nable, Fabio Aiuto, andrey.i.trufanov
On Tue, 2021-10-19 at 19:36 +0300, Andy Shevchenko wrote:
> On Tue, Oct 19, 2021 at 10:50:27PM +0900, Tsuchiya Yuto wrote:
> > On Mon, 2021-10-18 at 09:48 +0200, Hans de Goede wrote:
> > > On 10/17/21 18:19, Tsuchiya Yuto wrote:
>
> ...
>
> > > > ## for mipad2 (and whiskey cove pmic based devices)
> > > >
> > > > For devices which equip whiskey cove PMIC, you need to add non-upstream
> > > > regulator driver [1].
> > > >
> > > > [1] work done by jekhor, which seems to be from intel-aero or old
> > > > Android kernel
> > > > https://github.com/jekhor/yogabook-linux-kernel/commit/11c05b365fb2eeb4fced5aa66b362c511be32a34
> > > > ("intel_soc_pmic_chtwc: Add regulator driver and definition for VPROG1B")
> > >
> > > Interesting I recently bought a 2nd hand mipad2 myself too. I still need
> > > to put Linux on there. I'm definitely motivated to do that now :)
> >
> > I'm glad to hear that you also got a mipad2 :) It might be a interesting
> > device to look into. It even won't boot without nomodeset, no battery
> > charging/status on mainline kernel.
> >
> > By the way, instead of adding whiskey cove regulator driver, we may also
> > add a "hack" like the other PMIC in atomisp_gmin_platform to control
> > regulators [1].
>
> I looked briefly into the code and if we indeed need to turn off or on
> the regulators it should be a driver.
>
> I don't like having hacks outside of driver/staging to satisfy the one from
> the staging.
Yeah, if "reading" from the PMIC can't be achieved with the current
mainline kernel, it does not make sense to add code for "reading" just
for the hack inside atomisp. Rather, in this case, adding the regulator
driver is a straightforward way.
We can already write with intel_soc_pmic_exec_mipi_pmic_seq_element(),
so what I wondered is, is there equivalent for "read"?
But yes, we should eventually use regulator driver anyway.
Regards,
Tsuchiya Yuto
> I.o.w. having a regulator driver is a right thing to do in my opinion.
>
> > It seems that to do so, it needs to "read" value from the PMIC before
> > writing. So, I'm not sure if this can be achieved easily with the current
> > mainline kernel though.
> >
> > [1] https://github.com/MiCode/Xiaomi_Kernel_OpenSource/commit/6204d4b7aeefc4db622f8ac57b87bf2c76c6c8aa
> > ("atomisp_platform: add whiskey cove pmic support")
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (17 preceding siblings ...)
2021-10-18 7:48 ` [PATCH 00/17] various fixes for atomisp to make it work Hans de Goede
@ 2021-10-18 7:56 ` Andy Shevchenko
2021-10-20 6:50 ` Mauro Carvalho Chehab
2021-10-18 11:04 ` Andy Shevchenko
` (2 subsequent siblings)
21 siblings, 1 reply; 79+ messages in thread
From: Andy Shevchenko @ 2021-10-18 7:56 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Greg Kroah-Hartman, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, Alan,
Linux Media Mailing List, linux-staging,
Linux Kernel Mailing List
On Mon, Oct 18, 2021 at 6:45 AM Tsuchiya Yuto <kitakar@gmail.com> wrote:
>
> Hi all,
>
> This patch series contains fixes for atomisp to work (again). Tested on
> Microsoft Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with
> v5.15-rc5. Both are Cherry Trail (ISP2401) devices.
>
> I'm still not used to Linux patch sending flow. Sorry in advance
> if there is some weirdness :-) but I did my best.
I agree with Hans, you did an excellent job!
I will try to find time to look into this. In any case it seems to me
that this is a material more likely for v5.17-rc1, rather than v5.16.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-18 7:56 ` Andy Shevchenko
@ 2021-10-20 6:50 ` Mauro Carvalho Chehab
2021-10-20 12:44 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Mauro Carvalho Chehab @ 2021-10-20 6:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Tsuchiya Yuto, Hans de Goede, Patrik Gfeller, Sakari Ailus,
Greg Kroah-Hartman, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, Alan,
Linux Media Mailing List, linux-staging,
Linux Kernel Mailing List
Em Mon, 18 Oct 2021 10:56:40 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
> On Mon, Oct 18, 2021 at 6:45 AM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> >
> > Hi all,
> >
> > This patch series contains fixes for atomisp to work (again). Tested on
> > Microsoft Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with
> > v5.15-rc5. Both are Cherry Trail (ISP2401) devices.
> >
> > I'm still not used to Linux patch sending flow. Sorry in advance
> > if there is some weirdness :-) but I did my best.
>
> I agree with Hans, you did an excellent job!
> I will try to find time to look into this. In any case it seems to me
> that this is a material more likely for v5.17-rc1, rather than v5.16.
Yeah, it sounds to late to apply it for 5.16.
Regards,
Mauro
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-20 6:50 ` Mauro Carvalho Chehab
@ 2021-10-20 12:44 ` Tsuchiya Yuto
0 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-20 12:44 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Andy Shevchenko
Cc: Hans de Goede, Patrik Gfeller, Sakari Ailus, Greg Kroah-Hartman,
Yang Yingliang, Hans Verkuil, Aline Santana Cordeiro,
Dinghao Liu, Alan, Linux Media Mailing List, linux-staging,
Linux Kernel Mailing List
On Wed, 2021-10-20 at 07:50 +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 18 Oct 2021 10:56:40 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
>
> > On Mon, Oct 18, 2021 at 6:45 AM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> > >
> > > Hi all,
> > >
> > > This patch series contains fixes for atomisp to work (again). Tested on
> > > Microsoft Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with
> > > v5.15-rc5. Both are Cherry Trail (ISP2401) devices.
> > >
> > > I'm still not used to Linux patch sending flow. Sorry in advance
> > > if there is some weirdness :-) but I did my best.
> >
> > I agree with Hans, you did an excellent job!
> > I will try to find time to look into this.
Thank you :)
> > In any case it seems to me
> > that this is a material more likely for v5.17-rc1, rather than v5.16.
>
> Yeah, it sounds to late to apply it for 5.16.
I see, but no problem. This series does not contain any fixes for
critical issues (like NULL ptr deref)
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (18 preceding siblings ...)
2021-10-18 7:56 ` Andy Shevchenko
@ 2021-10-18 11:04 ` Andy Shevchenko
2021-10-20 12:48 ` Tsuchiya Yuto
2021-10-28 4:32 ` Tsuchiya Yuto
2021-10-28 12:51 ` Mauro Carvalho Chehab
21 siblings, 1 reply; 79+ messages in thread
From: Andy Shevchenko @ 2021-10-18 11:04 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, Alan, linux-media,
linux-staging, linux-kernel
On Mon, Oct 18, 2021 at 01:19:40AM +0900, Tsuchiya Yuto wrote:
Just a remark, your To is not filled. At some point I had written a script [1]
to help me with patch series sending, It also tries to be smart to include
necessary people and mailing lists (you can always alter it by adding more).
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-18 11:04 ` Andy Shevchenko
@ 2021-10-20 12:48 ` Tsuchiya Yuto
0 siblings, 0 replies; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-20 12:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Patrik Gfeller, Mauro Carvalho Chehab,
Sakari Ailus, Yang Yingliang, Hans Verkuil,
Aline Santana Cordeiro, Dinghao Liu, Alan, linux-media,
linux-staging, linux-kernel
On Mon, 2021-10-18 at 14:04 +0300, Andy Shevchenko wrote:
> On Mon, Oct 18, 2021 at 01:19:40AM +0900, Tsuchiya Yuto wrote:
>
> Just a remark, your To is not filled. At some point I had written a script [1]
> to help me with patch series sending, It also tries to be smart to include
> necessary people and mailing lists (you can always alter it by adding more).
> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
Ah thanks. I didn't use To but just used `--cc-cmd` with
scripts/get_maintainer.pl without thinking deeply...
I'll definitely try your script next time I send patches.
Regards,
Tsuchiya Yuto
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (19 preceding siblings ...)
2021-10-18 11:04 ` Andy Shevchenko
@ 2021-10-28 4:32 ` Tsuchiya Yuto
2021-10-28 10:58 ` Mauro Carvalho Chehab
2021-10-28 12:51 ` Mauro Carvalho Chehab
21 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-28 4:32 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
linux-media, linux-staging, linux-kernel, Nable, Andy Shevchenko,
Fabio Aiuto, andrey.i.trufanov, Patrik Gfeller
<Fixed Cc list>
On Mon, 2021-10-18 at 01:19 +0900, Tsuchiya Yuto wrote:
> [...]
> ## taking a picture with atomisp
>
> Note that to try to take a picture, please also apply at least the
> this RFC patch ("[BUG][RFC] media: atomisp: pci: assume run_mode is
> PREVIEW") I'll send as almost a BUG report later.
>
> You need to use firmware version irci_stable_candrpv_0415_20150521_0458,
> which is available from the intel-aero [1]
Just in case, the hash (as well as version) of firmware which I
downloaded from intel-aero and I use to capture is the following:
$ sha256sum /lib/firmware/shisp_2401a0_v21.bin
e89359f4e4934c410c83d525e283f34c5fcce9cb5caa75ad8a32d66d3842d95c /lib/firmware/shisp_2401a0_v21.bin
$ strings /lib/firmware/shisp_2401a0_v21.bin | grep 2015
irci_stable_candrpv_0415_20150521_0458
Regards,
Tsuchiya Yuto
> The atomisp (ipu2), like the ipu3, needs userspace support. The libcamera
> has now decent ipu3 support but does not have atomisp support yet.
>
> I found some userspace tools for atomisp that run on Linux:
>
> - capturev4l2 from intel-aero/sample-apps
> (https://github.com/intel-aero/sample-apps/tree/master/capturev4l2)
> - hd-camera from intel-aero/sample-apps
> (https://github.com/intel-aero/sample-apps/tree/master/hd-camera)
> - intel/nvt
> (https://github.com/intel/nvt)
>
> It looks like the nvt is the most feature-rich, like exposure and white
> balance. Note that current upstreamed atomisp dropped 32-bit support.
> So, you need to build it with `-m64` (change it in Makefile). Here is
> the example of usage I use on mipad2:
>
> $ ./v4l2n -o testimage_@.raw \
> --device /dev/video2 \
> --input 0 \
> --exposure=30000,30000,30000,30000 \
> --parm type=1,capturemode=CI_MODE_PREVIEW \
> --fmt type=1,width=1920,height=1080,pixelformat=NV12 \
> --reqbufs count=2,memory=USERPTR \
> --parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
> --capture=2 \
>
> ./raw2pnm -x1920 -y1080 -fNV12 testimage_001.raw testimage_001.pnm
> feh *.pnm # open the converted image
> rm testimage*
>
> Note that I see the following warn/err after capture:
>
> kern :warn : [72660.793335] atomisp-isp2 0000:00:03.0: stop stream timeout.
> kern :err : [72660.973629] atomisp-isp2 0000:00:03.0: atomisp_reset
>
> but I see the same message on the Android kernel, too. So, I think this
> is not a real issue (I hope).
>
> [1] https://github.com/intel-aero/meta-intel-aero-base/tree/master/recipes-kernel/linux/linux-yocto
> filename shisp_2401a0_v21.bin
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-28 4:32 ` Tsuchiya Yuto
@ 2021-10-28 10:58 ` Mauro Carvalho Chehab
2021-10-30 9:50 ` Tsuchiya Yuto
0 siblings, 1 reply; 79+ messages in thread
From: Mauro Carvalho Chehab @ 2021-10-28 10:58 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Sakari Ailus, Greg Kroah-Hartman, linux-media, linux-staging,
linux-kernel, Nable, Andy Shevchenko, Fabio Aiuto,
andrey.i.trufanov, Patrik Gfeller
Em Thu, 28 Oct 2021 13:32:29 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> <Fixed Cc list>
>
> On Mon, 2021-10-18 at 01:19 +0900, Tsuchiya Yuto wrote:
> > [...]
> > ## taking a picture with atomisp
> >
> > Note that to try to take a picture, please also apply at least the
> > this RFC patch ("[BUG][RFC] media: atomisp: pci: assume run_mode is
> > PREVIEW") I'll send as almost a BUG report later.
> >
> > You need to use firmware version irci_stable_candrpv_0415_20150521_0458,
> > which is available from the intel-aero [1]
>
> Just in case, the hash (as well as version) of firmware which I
> downloaded from intel-aero and I use to capture is the following:
>
> $ sha256sum /lib/firmware/shisp_2401a0_v21.bin
> e89359f4e4934c410c83d525e283f34c5fcce9cb5caa75ad8a32d66d3842d95c /lib/firmware/shisp_2401a0_v21.bin
>
> $ strings /lib/firmware/shisp_2401a0_v21.bin | grep 2015
> irci_stable_candrpv_0415_20150521_0458
>
> Regards,
> Tsuchiya Yuto
>
> > The atomisp (ipu2), like the ipu3, needs userspace support. The libcamera
> > has now decent ipu3 support but does not have atomisp support yet.
> >
> > I found some userspace tools for atomisp that run on Linux:
> >
> > - capturev4l2 from intel-aero/sample-apps
> > (https://github.com/intel-aero/sample-apps/tree/master/capturev4l2)
> > - hd-camera from intel-aero/sample-apps
> > (https://github.com/intel-aero/sample-apps/tree/master/hd-camera)
> > - intel/nvt
> > (https://github.com/intel/nvt)
> >
> > It looks like the nvt is the most feature-rich, like exposure and white
> > balance. Note that current upstreamed atomisp dropped 32-bit support.
> > So, you need to build it with `-m64` (change it in Makefile). Here is
> > the example of usage I use on mipad2:
> >
> > $ ./v4l2n -o testimage_@.raw \
> > --device /dev/video2 \
> > --input 0 \
> > --exposure=30000,30000,30000,30000 \
> > --parm type=1,capturemode=CI_MODE_PREVIEW \
> > --fmt type=1,width=1920,height=1080,pixelformat=NV12 \
> > --reqbufs count=2,memory=USERPTR \
> > --parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
> > --capture=2 \
> >
> > ./raw2pnm -x1920 -y1080 -fNV12 testimage_001.raw testimage_001.pnm
> > feh *.pnm # open the converted image
> > rm testimage*
Great! that worked for me too on Asus T101HA (CHT). I had to tweak the
resolution, as ov2680 sensor has a max of 1616x1216 30fps. I had
to use a number smaller than that, though (1600x1200).
I guess the next step is to make a generic app to also work on it.
> >
> > Note that I see the following warn/err after capture:
> >
> > kern :warn : [72660.793335] atomisp-isp2 0000:00:03.0: stop stream timeout.
> > kern :err : [72660.973629] atomisp-isp2 0000:00:03.0: atomisp_reset
> >
> > but I see the same message on the Android kernel, too. So, I think this
> > is not a real issue (I hope).
Same here.
> >
> > [1] https://github.com/intel-aero/meta-intel-aero-base/tree/master/recipes-kernel/linux/linux-yocto
> > filename shisp_2401a0_v21.bin
>
>
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-28 10:58 ` Mauro Carvalho Chehab
@ 2021-10-30 9:50 ` Tsuchiya Yuto
2021-10-30 10:49 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 79+ messages in thread
From: Tsuchiya Yuto @ 2021-10-30 9:50 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Greg Kroah-Hartman, linux-media, linux-staging,
linux-kernel, Nable, Andy Shevchenko, Fabio Aiuto,
andrey.i.trufanov, Patrik Gfeller
On Thu, 2021-10-28 at 11:58 +0100, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Oct 2021 13:32:29 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
>
> > <Fixed Cc list>
> >
> > On Mon, 2021-10-18 at 01:19 +0900, Tsuchiya Yuto wrote:
> > > [...]
> > > ## taking a picture with atomisp
> > >
> > > Note that to try to take a picture, please also apply at least the
> > > this RFC patch ("[BUG][RFC] media: atomisp: pci: assume run_mode is
> > > PREVIEW") I'll send as almost a BUG report later.
> > >
> > > You need to use firmware version irci_stable_candrpv_0415_20150521_0458,
> > > which is available from the intel-aero [1]
> >
> > Just in case, the hash (as well as version) of firmware which I
> > downloaded from intel-aero and I use to capture is the following:
> >
> > $ sha256sum /lib/firmware/shisp_2401a0_v21.bin
> > e89359f4e4934c410c83d525e283f34c5fcce9cb5caa75ad8a32d66d3842d95c /lib/firmware/shisp_2401a0_v21.bin
> >
> > $ strings /lib/firmware/shisp_2401a0_v21.bin | grep 2015
> > irci_stable_candrpv_0415_20150521_0458
Also note that the firmware file from the intel-aero only supports
hw_revision 2401a0_v21 as the filename implies. So, if someone have
Bay Trail (ISP2400) device to test, you need to get a firmware file (from
somewhere like Android installation/image as the initial commit of atomisp
mentions) made for version irci_stable_candrpv_0415_20150521_0458 and
hw_revision 2400b0_v21 then place it under /lib/firmware
> > > The atomisp (ipu2), like the ipu3, needs userspace support. The libcamera
> > > has now decent ipu3 support but does not have atomisp support yet.
> > >
> > > I found some userspace tools for atomisp that run on Linux:
> > >
> > > - capturev4l2 from intel-aero/sample-apps
> > > (https://github.com/intel-aero/sample-apps/tree/master/capturev4l2)
> > > - hd-camera from intel-aero/sample-apps
> > > (https://github.com/intel-aero/sample-apps/tree/master/hd-camera)
> > > - intel/nvt
> > > (https://github.com/intel/nvt)
> > >
> > > It looks like the nvt is the most feature-rich, like exposure and white
> > > balance. Note that current upstreamed atomisp dropped 32-bit support.
> > > So, you need to build it with `-m64` (change it in Makefile). Here is
> > > the example of usage I use on mipad2:
> > >
> > > $ ./v4l2n -o testimage_@.raw \
> > > --device /dev/video2 \
> > > --input 0 \
> > > --exposure=30000,30000,30000,30000 \
> > > --parm type=1,capturemode=CI_MODE_PREVIEW \
> > > --fmt type=1,width=1920,height=1080,pixelformat=NV12 \
> > > --reqbufs count=2,memory=USERPTR \
> > > --parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
> > > --capture=2 \
> > >
> > > ./raw2pnm -x1920 -y1080 -fNV12 testimage_001.raw testimage_001.pnm
> > > feh *.pnm # open the converted image
> > > rm testimage*
>
> Great! that worked for me too on Asus T101HA (CHT). I had to tweak the
> resolution, as ov2680 sensor has a max of 1616x1216 30fps. I had
> to use a number smaller than that, though (1600x1200).
Ah, glad to hear that!
> I guess the next step is to make a generic app to also work on it.
It's great if we can eventually add atomisp support to the libcamera.
I think this is the easiest way to support generic apps (I mean, like
cheese). Some ipu3 cameras already work on cheese with libcamera.
I don't have any knowledge about userspace support though.
> > >
> > > Note that I see the following warn/err after capture:
> > >
> > > kern :warn : [72660.793335] atomisp-isp2 0000:00:03.0: stop stream timeout.
> > > kern :err : [72660.973629] atomisp-isp2 0000:00:03.0: atomisp_reset
> > >
> > > but I see the same message on the Android kernel, too. So, I think this
> > > is not a real issue (I hope).
>
> Same here.
>
> > >
> > > [1] https://github.com/intel-aero/meta-intel-aero-base/tree/master/recipes-kernel/linux/linux-yocto
> > > filename shisp_2401a0_v21.bin
> >
> >
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-30 9:50 ` Tsuchiya Yuto
@ 2021-10-30 10:49 ` Mauro Carvalho Chehab
2021-10-30 11:01 ` Andy Shevchenko
0 siblings, 1 reply; 79+ messages in thread
From: Mauro Carvalho Chehab @ 2021-10-30 10:49 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Sakari Ailus, Greg Kroah-Hartman, linux-media, linux-staging,
linux-kernel, Nable, Andy Shevchenko, Fabio Aiuto,
andrey.i.trufanov, Patrik Gfeller
Em Sat, 30 Oct 2021 18:50:14 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> On Thu, 2021-10-28 at 11:58 +0100, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Oct 2021 13:32:29 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> >
> > > <Fixed Cc list>
> > >
> > > On Mon, 2021-10-18 at 01:19 +0900, Tsuchiya Yuto wrote:
> > > > [...]
> > > > ## taking a picture with atomisp
> > > >
> > > > Note that to try to take a picture, please also apply at least the
> > > > this RFC patch ("[BUG][RFC] media: atomisp: pci: assume run_mode is
> > > > PREVIEW") I'll send as almost a BUG report later.
> > > >
> > > > You need to use firmware version irci_stable_candrpv_0415_20150521_0458,
> > > > which is available from the intel-aero [1]
> > >
> > > Just in case, the hash (as well as version) of firmware which I
> > > downloaded from intel-aero and I use to capture is the following:
> > >
> > > $ sha256sum /lib/firmware/shisp_2401a0_v21.bin
> > > e89359f4e4934c410c83d525e283f34c5fcce9cb5caa75ad8a32d66d3842d95c /lib/firmware/shisp_2401a0_v21.bin
> > >
> > > $ strings /lib/firmware/shisp_2401a0_v21.bin | grep 2015
> > > irci_stable_candrpv_0415_20150521_0458
>
> Also note that the firmware file from the intel-aero only supports
> hw_revision 2401a0_v21 as the filename implies. So, if someone have
> Bay Trail (ISP2400) device to test, you need to get a firmware file (from
> somewhere like Android installation/image as the initial commit of atomisp
> mentions) made for version irci_stable_candrpv_0415_20150521_0458 and
> hw_revision 2400b0_v21 then place it under /lib/firmware
Yeah, understood.
> > > > The atomisp (ipu2), like the ipu3, needs userspace support. The libcamera
> > > > has now decent ipu3 support but does not have atomisp support yet.
> > > >
> > > > I found some userspace tools for atomisp that run on Linux:
> > > >
> > > > - capturev4l2 from intel-aero/sample-apps
> > > > (https://github.com/intel-aero/sample-apps/tree/master/capturev4l2)
> > > > - hd-camera from intel-aero/sample-apps
> > > > (https://github.com/intel-aero/sample-apps/tree/master/hd-camera)
> > > > - intel/nvt
> > > > (https://github.com/intel/nvt)
> > > >
> > > > It looks like the nvt is the most feature-rich, like exposure and white
> > > > balance. Note that current upstreamed atomisp dropped 32-bit support.
> > > > So, you need to build it with `-m64` (change it in Makefile). Here is
> > > > the example of usage I use on mipad2:
> > > >
> > > > $ ./v4l2n -o testimage_@.raw \
> > > > --device /dev/video2 \
> > > > --input 0 \
> > > > --exposure=30000,30000,30000,30000 \
> > > > --parm type=1,capturemode=CI_MODE_PREVIEW \
> > > > --fmt type=1,width=1920,height=1080,pixelformat=NV12 \
> > > > --reqbufs count=2,memory=USERPTR \
> > > > --parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
> > > > --capture=2 \
> > > >
> > > > ./raw2pnm -x1920 -y1080 -fNV12 testimage_001.raw testimage_001.pnm
> > > > feh *.pnm # open the converted image
> > > > rm testimage*
> >
> > Great! that worked for me too on Asus T101HA (CHT). I had to tweak the
> > resolution, as ov2680 sensor has a max of 1616x1216 30fps. I had
> > to use a number smaller than that, though (1600x1200).
>
> Ah, glad to hear that!
>
> > I guess the next step is to make a generic app to also work on it.
>
> It's great if we can eventually add atomisp support to the libcamera.
> I think this is the easiest way to support generic apps (I mean, like
> cheese). Some ipu3 cameras already work on cheese with libcamera.
> I don't have any knowledge about userspace support though.
There are a lot more to be done in order to make it ready for libcamera
(if ever).
See, libcamera assumes that the device exports its internal pipelines
via the media controller. The atomisp code setups such pipelines
internally, exposing a "normal" [1] V4L2 interface.
[1] Well, it misses several V4L2 ioctl implementations that currently
causes generic applications to fail, and mis-implement others,
but the idea behind the driver is to fully control the driver via
/dev/video? nodes, without requiring the media controller API.
Converting atomisp into a media-controller driver will require a
major rework. I suspect that it is a lot easier to make it work with
normal V4L2 applications by fixing the ioctl implementation than
to port it to MC.
Yet, in order to be able to move it from staging, we'll need to convert
it into an MC-controlled driver.
What I'm saying is that, IMHO, we should:
1. Fix the ioctls in order to allow a normal app to use it. I'm
already doing some work on this sense. We should ensure that the
driver will pass v4l2-compliance tests on this step;
2. remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
register address differences between ISP2400 and ISP2401;
3. Cleanup the driver code, removing the abstraction layers inside it;
4. Migrate the sensor drivers out of staging (or re-using existing
drivers);
5. Remove the logic which sets up pipelines inside it, moving it to
libcamera and implement MC support;
6. Move it out of staging.
This is easily said than done, as steps 2-6 are very complex and will
require lots of work. Also, both ISP2400 and 2401 should be tested
while doing some of those major reworks, in order to avoid breakages.
Btw, v4l2grab app (at v4l-utils) already works. This is a very simple
app, written to allow stream testing. It doesn't do anything fancy,
like trying to enumerate the formats, and it needs to be set to a
resolution lower than the one announced by the sensor, probably due
to some bug at the COPY pipeline settings at atomisp driver.
qv4l2, for instance, causes a driver OOPS when it calls G/S_PRIORITY
ioctls.
Regards,
Mauro
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-30 10:49 ` Mauro Carvalho Chehab
@ 2021-10-30 11:01 ` Andy Shevchenko
2021-10-30 18:30 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 79+ messages in thread
From: Andy Shevchenko @ 2021-10-30 11:01 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Tsuchiya Yuto, Sakari Ailus, Greg Kroah-Hartman, linux-media,
linux-staging, linux-kernel, Nable, Andy Shevchenko, Fabio Aiuto,
andrey.i.trufanov, Patrik Gfeller
On Sat, Oct 30, 2021 at 1:49 PM Mauro Carvalho Chehab
<mchehab@kernel.org> wrote:
> Em Sat, 30 Oct 2021 18:50:14 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
...
> What I'm saying is that, IMHO, we should:
>
> 1. Fix the ioctls in order to allow a normal app to use it. I'm
> already doing some work on this sense. We should ensure that the
> driver will pass v4l2-compliance tests on this step;
>
> 2. remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
> register address differences between ISP2400 and ISP2401;
>
> 3. Cleanup the driver code, removing the abstraction layers inside it;
>
> 4. Migrate the sensor drivers out of staging (or re-using existing
> drivers);
>
> 5. Remove the logic which sets up pipelines inside it, moving it to
> libcamera and implement MC support;
>
> 6. Move it out of staging.
>
> This is easily said than done, as steps 2-6 are very complex and will
> require lots of work. Also, both ISP2400 and 2401 should be tested
> while doing some of those major reworks, in order to avoid breakages.
This is a great roadmap nevertheless!
However, we missed one important step here, i.e. persuade Intel to
clarify license of the firmware (for distirbution) and make sure we
have it in Linux firmware project, so it won't get lost.
> Btw, v4l2grab app (at v4l-utils) already works. This is a very simple
> app, written to allow stream testing. It doesn't do anything fancy,
> like trying to enumerate the formats, and it needs to be set to a
> resolution lower than the one announced by the sensor, probably due
> to some bug at the COPY pipeline settings at atomisp driver.
>
> qv4l2, for instance, causes a driver OOPS when it calls G/S_PRIORITY
> ioctls.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-30 11:01 ` Andy Shevchenko
@ 2021-10-30 18:30 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 79+ messages in thread
From: Mauro Carvalho Chehab @ 2021-10-30 18:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Tsuchiya Yuto, Sakari Ailus, Greg Kroah-Hartman, linux-media,
linux-staging, linux-kernel, Nable, Andy Shevchenko, Fabio Aiuto,
andrey.i.trufanov, Patrik Gfeller
Em Sat, 30 Oct 2021 14:01:01 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
> On Sat, Oct 30, 2021 at 1:49 PM Mauro Carvalho Chehab
> <mchehab@kernel.org> wrote:
> > Em Sat, 30 Oct 2021 18:50:14 +0900
> > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
>
> ...
>
> > What I'm saying is that, IMHO, we should:
> >
> > 1. Fix the ioctls in order to allow a normal app to use it. I'm
> > already doing some work on this sense. We should ensure that the
> > driver will pass v4l2-compliance tests on this step;
> >
> > 2. remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
> > register address differences between ISP2400 and ISP2401;
> >
> > 3. Cleanup the driver code, removing the abstraction layers inside it;
> >
> > 4. Migrate the sensor drivers out of staging (or re-using existing
> > drivers);
> >
> > 5. Remove the logic which sets up pipelines inside it, moving it to
> > libcamera and implement MC support;
> >
> > 6. Move it out of staging.
> >
> > This is easily said than done, as steps 2-6 are very complex and will
> > require lots of work. Also, both ISP2400 and 2401 should be tested
> > while doing some of those major reworks, in order to avoid breakages.
>
> This is a great roadmap nevertheless!
> However, we missed one important step here, i.e. persuade Intel to
> clarify license of the firmware (for distirbution) and make sure we
> have it in Linux firmware project, so it won't get lost.
Heh, true!
I suspect that the firmare for ISP2401, used on Intel Aero, is probably
easier to make it available than the firmware for ISP2400.
At least looking at the github repository for Intel Aero, it says at the
package metadata that its content is under GPL:
https://github.com/intel-aero/meta-intel-aero-base/blob/master/LICENSE
The firmware itself is there, at:
https://github.com/intel-aero/meta-intel-aero-base/blob/master/recipes-kernel/linux/linux-yocto/shisp_2401a0_v21.bin
The Yocto's meta package for the firmware blob is at:
https://github.com/intel-aero/packages/tree/master/firmware-atomisp
Also says, on its copyright's notice:
https://github.com/intel-aero/packages/blob/master/firmware-atomisp/debian/copyright
that:
Source: https://github.com/intel-aero/meta-intel-aero-base
Files: *
Copyright: 2016 Intel Corporation
License: GPL-2
/usr/share/common-licenses/GPL-2
Granted, it doesn't make sense to say that a firmware blob is under
GPL. My reading is that the original intend was to allow GPL software
to use/distribute such firmware, but yeah, someone at Intel should
provide the community an explicit authorization to allow distros to
start packaging such firmware.
Regards,
Mauro
^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH 00/17] various fixes for atomisp to make it work
2021-10-17 16:19 [PATCH 00/17] various fixes for atomisp to make it work Tsuchiya Yuto
` (20 preceding siblings ...)
2021-10-28 4:32 ` Tsuchiya Yuto
@ 2021-10-28 12:51 ` Mauro Carvalho Chehab
21 siblings, 0 replies; 79+ messages in thread
From: Mauro Carvalho Chehab @ 2021-10-28 12:51 UTC (permalink / raw)
To: Tsuchiya Yuto
Cc: Hans de Goede, Patrik Gfeller, Sakari Ailus, Greg Kroah-Hartman,
Yang Yingliang, Hans Verkuil, Aline Santana Cordeiro,
Dinghao Liu, Alan, linux-media, linux-staging, linux-kernel
Em Mon, 18 Oct 2021 01:19:40 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> Hi all,
>
> This patch series contains fixes for atomisp to work (again). Tested on
> Microsoft Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with
> v5.15-rc5. Both are Cherry Trail (ISP2401) devices.
>
After testing this series, I'm opting to merge most patches at the media
stage tree:
https://git.linuxtv.org/media_stage.git/log/
That would allow more people to test the changes.
It is probably too late to merge it for 5.16, so I'll likely postpone
merging it at media_tree to happen after 5.16-rc1.
Regards,
Mauro
---
I tested it on an Asus T101HA with:
https://github.com/intel/nvt
compiled with -m64, and with the enclosed script:
#!/bin/bash
set -e
FRAMES=10
FORMAT=NV12
WIDTH=1600
HEIGHT=1200
#FORMAT=NV12
#WIDTH=1920
#HEIGHT=1080
./v4l2n -o testimage_@.raw \
--device /dev/video2 \
--input 0 \
--exposure=30000,30000,30000,30000 \
--parm type=1,capturemode=CI_MODE_PREVIEW \
--fmt type=1,width=$WIDTH,height=$HEIGHT,pixelformat=$FORMAT \
--reqbufs count=2,memory=USERPTR \
--parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
--capture=$FRAMES
for i in $(seq 0 $(($FRAMES - 1))); do
name="testimage_$(printf "%03i" $i)"
./raw2pnm -x$WIDTH -y$HEIGHT -f$FORMAT $name.raw $name.pnm
rm $name.raw
done
^ permalink raw reply [flat|nested] 79+ messages in thread