[PATCH 0/4] media: atomisp: mt9m114: Make it work on Asus T100TA

[PATCH 0/4] media: atomisp: mt9m114: Make it work on Asus T100TA

[Hans de Goede]

Hi All,

Since there is some interest in getting the mt9m114 camera to work on
the Asus T100TA:

https://github.com/jfwells/linux-asus-t100ta/issues/4
https://lore.kernel.org/platform-driver-x86/e0c8c98f-64ec-4297-bbc8-de489414515c@zonnet.nl/

I have spend some time this weekend poking at this resulting in this
series which makes things work somewhat.

Note that for now this will only work if you modify the BIOS settings
to change the ISP to be listed as a PCI device in lspci:

00:03.0 Multimedia controller: Intel Corporation Atom Processor Z36xxx/Z37xxx Series Camera ISP (rev 09)

If you do not have this in lspci then things will not work. The BIOS
does not allow changing this setting from its menu, so we need to
use the grub setup_var_3 command form a patched grub.

Note this requires your Asus T100TA to be at the latest v314 BIOS version,
this has not been tested with other versions!

Download grubia32.efi from here:

https://fedorapeople.org/~jwrdegoede/grub-efi-setup_var/grubia32.efi

Boot this once and then run:

setup_var_3 6f

If the ISP is not visible in lspci this should show 01 as value
for the 0x6f offset.

Now change this to 2:

setup_var_3 6f 02

and then reboot. After this the ISP should be visible in lspci.

For more details on how this works see:
https://hansdegoede.dreamwidth.org/25589.html

Note this series is just a quick hack, the proper fix would be to get
the new drivers/media/i2c/mt9m114 standard v4l2 driver to work and
atomisp does support standard v4l2 drivers for a while now.

I plan to take a shot at this as time permits, hopefully sometime
within a month but no promises.

Regards,

Hans


Hans de Goede (4):
  media: atomisp: Fix WARN_ON() in vb2_start_streaming() triggering
  media: atomisp: mt9m114: Disable V4L2_CID_3A_LOCK control
  media: atomisp: mt9m114: Add missing mutex_init() call
  media: atomisp: mt9m114: Fix fmt->code not getting set on try_fmt

 drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c | 13 +++++++++----
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c   |  5 ++++-
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.47.0

[PATCH 1/4] media: atomisp: Fix WARN_ON() in vb2_start_streaming() triggering

[Hans de Goede]

If atomisp fails to start the stream the buffers must be returned to
the VB2_BUF_STATE_QUEUED state before returning an error from
atomisp_start_streaming().

This fixes the following WARN_ON():
[  250.313554] WARNING: CPU: 3 PID: 2178 at drivers/media/common/videobuf2/videobuf2-core.c:1801 vb2_start_streaming+0xcb/0x150 [videobuf2_common]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index d7e8a9871522..47d8a17379b0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -881,8 +881,10 @@ int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
 	mutex_lock(&isp->mutex);
 
 	ret = atomisp_pipe_check(pipe, false);
-	if (ret)
+	if (ret) {
+		atomisp_flush_video_pipe(pipe, VB2_BUF_STATE_QUEUED, true);
 		goto out_unlock;
+	}
 
 	/*
 	 * When running a classic v4l2 app after a media-controller aware
@@ -895,6 +897,7 @@ int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
 	mutex_unlock(&isp->media_dev.graph_mutex);
 	if (ret) {
 		dev_err(isp->dev, "Error starting mc pipeline: %d\n", ret);
+		atomisp_flush_video_pipe(pipe, VB2_BUF_STATE_QUEUED, true);
 		goto out_unlock;
 	}
 
-- 
2.47.0

Re: [PATCH 1/4] media: atomisp: Fix WARN_ON() in vb2_start_streaming() triggering

[Andy Shevchenko]

On Sun, Oct 13, 2024 at 05:40:53PM +0200, Hans de Goede wrote:
> If atomisp fails to start the stream the buffers must be returned to
> the VB2_BUF_STATE_QUEUED state before returning an error from
> atomisp_start_streaming().
> 
> This fixes the following WARN_ON():
> [  250.313554] WARNING: CPU: 3 PID: 2178 at drivers/media/common/videobuf2/videobuf2-core.c:1801 vb2_start_streaming+0xcb/0x150 [videobuf2_common]

...

>  	if (ret) {
>  		dev_err(isp->dev, "Error starting mc pipeline: %d\n", ret);
> +		atomisp_flush_video_pipe(pipe, VB2_BUF_STATE_QUEUED, true);
>  		goto out_unlock;
>  	}

Seems like error message does not depend on the video pipe. Hence I would
first flush the pipe and then print a message.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/4] media: atomisp: mt9m114: Disable V4L2_CID_3A_LOCK control

[Hans de Goede]

The V4L2_CID_3A_LOCK control is causing v4l2_ctrl_new_custom()
to fail with -ERANGE.

It would be better to fix this control but this entire driver is going to
be replaced with the drivers/media/i2c/mt9m114 standard v4l2 driver. This
quick fix gets things going for now until the driver is replaced.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
index 918ea4fa9f6b..b0b740dd3ca3 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
@@ -1224,6 +1224,7 @@ static struct v4l2_ctrl_config mt9m114_controls[] = {
 		.def = 0,
 		.flags = 0,
 	},
+#if 0 /* Causes v4l2_ctrl_new_custom() to fail with -ERANGE, disable for now */
 	{
 		.ops = &ctrl_ops,
 		.id = V4L2_CID_3A_LOCK,
@@ -1235,6 +1236,7 @@ static struct v4l2_ctrl_config mt9m114_controls[] = {
 		.def = 0,
 		.flags = 0,
 	},
+#endif
 };
 
 static int mt9m114_detect(struct mt9m114_device *dev, struct i2c_client *client)
-- 
2.47.0

[PATCH 3/4] media: atomisp: mt9m114: Add missing mutex_init() call

[Hans de Goede]

The input_lock was not being initialized, fix this.

Also switch to devm_kzalloc() for the main driver data struct, so that
devm_mutex_init() can be used for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
index b0b740dd3ca3..1a67f93a53d7 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
@@ -30,6 +30,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/acpi.h>
+#include <linux/mutex.h>
 #include "../include/linux/atomisp_gmin_platform.h"
 #include <media/v4l2-device.h>
 
@@ -1527,7 +1528,6 @@ static void mt9m114_remove(struct i2c_client *client)
 	v4l2_device_unregister_subdev(sd);
 	media_entity_cleanup(&dev->sd.entity);
 	v4l2_ctrl_handler_free(&dev->ctrl_handler);
-	kfree(dev);
 }
 
 static int mt9m114_probe(struct i2c_client *client)
@@ -1538,10 +1538,14 @@ static int mt9m114_probe(struct i2c_client *client)
 	void *pdata;
 
 	/* Setup sensor configuration structure */
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 
+	ret = devm_mutex_init(&client->dev, &dev->input_lock);
+	if (ret)
+		return ret;
+
 	v4l2_i2c_subdev_init(&dev->sd, client, &mt9m114_ops);
 	pdata = gmin_camera_platform_data(&dev->sd,
 					  ATOMISP_INPUT_FORMAT_RAW_10,
@@ -1550,14 +1554,12 @@ static int mt9m114_probe(struct i2c_client *client)
 		ret = mt9m114_s_config(&dev->sd, client->irq, pdata);
 	if (!pdata || ret) {
 		v4l2_device_unregister_subdev(&dev->sd);
-		kfree(dev);
 		return ret;
 	}
 
 	ret = atomisp_register_i2c_module(&dev->sd, pdata);
 	if (ret) {
 		v4l2_device_unregister_subdev(&dev->sd);
-		kfree(dev);
 		/* Coverity CID 298095 - return on error */
 		return ret;
 	}
-- 
2.47.0

Re: [PATCH 3/4] media: atomisp: mt9m114: Add missing mutex_init() call

[Andy Shevchenko]

On Sun, Oct 13, 2024 at 05:40:55PM +0200, Hans de Goede wrote:
> The input_lock was not being initialized, fix this.
> 
> Also switch to devm_kzalloc() for the main driver data struct, so that
> devm_mutex_init() can be used for this.

...

>  		ret = mt9m114_s_config(&dev->sd, client->irq, pdata);
>  	if (!pdata || ret) {

Hmm... What is the ret value when no pdata is provided?

>  		v4l2_device_unregister_subdev(&dev->sd);
> -		kfree(dev);
>  		return ret;
>  	}

...

>  	ret = atomisp_register_i2c_module(&dev->sd, pdata);
>  	if (ret) {
>  		v4l2_device_unregister_subdev(&dev->sd);
> -		kfree(dev);
>  		/* Coverity CID 298095 - return on error */

This comment is useless. But it seems we tend to drop this code completely in
the future.

>  		return ret;
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] media: atomisp: mt9m114: Add missing mutex_init() call

[Hans de Goede]

Hi,

On 14-Oct-24 1:31 PM, Andy Shevchenko wrote:
> On Sun, Oct 13, 2024 at 05:40:55PM +0200, Hans de Goede wrote:
>> The input_lock was not being initialized, fix this.
>>
>> Also switch to devm_kzalloc() for the main driver data struct, so that
>> devm_mutex_init() can be used for this.
> 
> ...
> 
>>  		ret = mt9m114_s_config(&dev->sd, client->irq, pdata);
>>  	if (!pdata || ret) {
> 
> Hmm... What is the ret value when no pdata is provided?

0, so this error-exit leaves the driver bound (pre-existing problem)

The error handling in the probe function is quite foobar in general,
but I just wanted to get the driver working as a POC / to have
a known working setup before focusing on using the other driver
since that is the way forward.

Regards,

Hans




> 
>>  		v4l2_device_unregister_subdev(&dev->sd);
>> -		kfree(dev);
>>  		return ret;
>>  	}
> 
> ...
> 
>>  	ret = atomisp_register_i2c_module(&dev->sd, pdata);
>>  	if (ret) {
>>  		v4l2_device_unregister_subdev(&dev->sd);
>> -		kfree(dev);
>>  		/* Coverity CID 298095 - return on error */
> 
> This comment is useless. But it seems we tend to drop this code completely in
> the future.
> 
>>  		return ret;
>>  	}
> 

[PATCH 4/4] media: atomisp: mt9m114: Fix fmt->code not getting set on try_fmt

[Hans de Goede]

Fix mt9m114_set_fmt() not setting format.format.code when called with
format.which == V4L2_SUBDEV_FORMAT_TRY.

This fixes atomisp failing to start streaming with the mt9m114 on
an Asus T100TA because of __media_pipeline_start() returning -EPIPE.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
index 1a67f93a53d7..7cf543986b91 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
@@ -665,6 +665,7 @@ static int mt9m114_set_fmt(struct v4l2_subdev *sd,
 
 	fmt->width = res->width;
 	fmt->height = res->height;
+	fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		*v4l2_subdev_state_get_format(sd_state, 0) = *fmt;
-- 
2.47.0

Re: [PATCH 0/4] media: atomisp: mt9m114: Make it work on Asus T100TA

[Andy Shevchenko]

On Sun, Oct 13, 2024 at 05:40:52PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Since there is some interest in getting the mt9m114 camera to work on
> the Asus T100TA:
> 
> https://github.com/jfwells/linux-asus-t100ta/issues/4
> https://lore.kernel.org/platform-driver-x86/e0c8c98f-64ec-4297-bbc8-de489414515c@zonnet.nl/
> 
> I have spend some time this weekend poking at this resulting in this
> series which makes things work somewhat.
> 
> Note that for now this will only work if you modify the BIOS settings
> to change the ISP to be listed as a PCI device in lspci:
> 
> 00:03.0 Multimedia controller: Intel Corporation Atom Processor Z36xxx/Z37xxx Series Camera ISP (rev 09)
> 
> If you do not have this in lspci then things will not work. The BIOS
> does not allow changing this setting from its menu, so we need to
> use the grub setup_var_3 command form a patched grub.
> 
> Note this requires your Asus T100TA to be at the latest v314 BIOS version,
> this has not been tested with other versions!
> 
> Download grubia32.efi from here:
> 
> https://fedorapeople.org/~jwrdegoede/grub-efi-setup_var/grubia32.efi
> 
> Boot this once and then run:
> 
> setup_var_3 6f
> 
> If the ISP is not visible in lspci this should show 01 as value
> for the 0x6f offset.
> 
> Now change this to 2:
> 
> setup_var_3 6f 02
> 
> and then reboot. After this the ISP should be visible in lspci.
> 
> For more details on how this works see:
> https://hansdegoede.dreamwidth.org/25589.html
> 
> Note this series is just a quick hack, the proper fix would be to get
> the new drivers/media/i2c/mt9m114 standard v4l2 driver to work and
> atomisp does support standard v4l2 drivers for a while now.
> 
> I plan to take a shot at this as time permits, hopefully sometime
> within a month but no promises.

LGTM, some nit-picks in the individual replies.
Reviewed-by: Andy Shevchenko <andy@kernel.org>

P.S. Can you consider my patch to apply (that removes GPL text
     in the top comments)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] media: atomisp: mt9m114: Make it work on Asus T100TA

[Hans de Goede]

Hi,

On 14-Oct-24 1:32 PM, Andy Shevchenko wrote:
> On Sun, Oct 13, 2024 at 05:40:52PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Since there is some interest in getting the mt9m114 camera to work on
>> the Asus T100TA:
>>
>> https://github.com/jfwells/linux-asus-t100ta/issues/4
>> https://lore.kernel.org/platform-driver-x86/e0c8c98f-64ec-4297-bbc8-de489414515c@zonnet.nl/
>>
>> I have spend some time this weekend poking at this resulting in this
>> series which makes things work somewhat.
>>
>> Note that for now this will only work if you modify the BIOS settings
>> to change the ISP to be listed as a PCI device in lspci:
>>
>> 00:03.0 Multimedia controller: Intel Corporation Atom Processor Z36xxx/Z37xxx Series Camera ISP (rev 09)
>>
>> If you do not have this in lspci then things will not work. The BIOS
>> does not allow changing this setting from its menu, so we need to
>> use the grub setup_var_3 command form a patched grub.
>>
>> Note this requires your Asus T100TA to be at the latest v314 BIOS version,
>> this has not been tested with other versions!
>>
>> Download grubia32.efi from here:
>>
>> https://fedorapeople.org/~jwrdegoede/grub-efi-setup_var/grubia32.efi
>>
>> Boot this once and then run:
>>
>> setup_var_3 6f
>>
>> If the ISP is not visible in lspci this should show 01 as value
>> for the 0x6f offset.
>>
>> Now change this to 2:
>>
>> setup_var_3 6f 02
>>
>> and then reboot. After this the ISP should be visible in lspci.
>>
>> For more details on how this works see:
>> https://hansdegoede.dreamwidth.org/25589.html
>>
>> Note this series is just a quick hack, the proper fix would be to get
>> the new drivers/media/i2c/mt9m114 standard v4l2 driver to work and
>> atomisp does support standard v4l2 drivers for a while now.
>>
>> I plan to take a shot at this as time permits, hopefully sometime
>> within a month but no promises.
> 
> LGTM, some nit-picks in the individual replies.
> Reviewed-by: Andy Shevchenko <andy@kernel.org>

Thank you.

> P.S. Can you consider my patch to apply (that removes GPL text
>      in the top comments)?

Yes I'll pick this up together with other atomisp patches
the next time I do a round of atomisp patch merging.

Regards,

Hans

Re: [PATCH 0/4] media: atomisp: mt9m114: Make it work on Asus T100TA

[Hans de Goede]

Hi,

On 13-Oct-24 5:40 PM, Hans de Goede wrote:
> Hi All,
> 
> Since there is some interest in getting the mt9m114 camera to work on
> the Asus T100TA:
> 
> https://github.com/jfwells/linux-asus-t100ta/issues/4
> https://lore.kernel.org/platform-driver-x86/e0c8c98f-64ec-4297-bbc8-de489414515c@zonnet.nl/
> 
> I have spend some time this weekend poking at this resulting in this
> series which makes things work somewhat.
> 
> Note that for now this will only work if you modify the BIOS settings
> to change the ISP to be listed as a PCI device in lspci:
> 
> 00:03.0 Multimedia controller: Intel Corporation Atom Processor Z36xxx/Z37xxx Series Camera ISP (rev 09)
> 
> If you do not have this in lspci then things will not work. The BIOS
> does not allow changing this setting from its menu, so we need to
> use the grub setup_var_3 command form a patched grub.
> 
> Note this requires your Asus T100TA to be at the latest v314 BIOS version,
> this has not been tested with other versions!
> 
> Download grubia32.efi from here:
> 
> https://fedorapeople.org/~jwrdegoede/grub-efi-setup_var/grubia32.efi
> 
> Boot this once and then run:
> 
> setup_var_3 6f
> 
> If the ISP is not visible in lspci this should show 01 as value
> for the 0x6f offset.
> 
> Now change this to 2:
> 
> setup_var_3 6f 02
> 
> and then reboot. After this the ISP should be visible in lspci.
> 
> For more details on how this works see:
> https://hansdegoede.dreamwidth.org/25589.html
> 
> Note this series is just a quick hack, the proper fix would be to get
> the new drivers/media/i2c/mt9m114 standard v4l2 driver to work and
> atomisp does support standard v4l2 drivers for a while now.
> 
> I plan to take a shot at this as time permits, hopefully sometime
> within a month but no promises.

Thank you for your patch(es).

I have merged this/these in my media-atomisp branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

And this/these will be included in my next pull-request to
Mauro (to media subsystem maintainer)

Regards,

Hans