NULL pointer dereference in imx-mipi-csis driver when starting stream

Thu Feb 16 00:18:14 PST 2023

On 15.02.23 19:19, Laurent Pinchart wrote:
> On Wed, Feb 15, 2023 at 04:30:49PM +0100, Frieder Schrempf wrote:
>> On 15.02.23 15:40, Frieder Schrempf wrote:
>>> On 15.02.23 15:20, Frieder Schrempf wrote:
>>>> Hi Laurent,
>>>>
>>>> On 15.02.23 13:05, Laurent Pinchart wrote:
>>>>> On Wed, Feb 15, 2023 at 12:53:56PM +0100, Frieder Schrempf wrote:
>>>>>> On 14.02.23 17:47, Frieder Schrempf wrote:
>>>>>>> Hi everyone,
>>>>>>>
>>>>>>> after solving the previous devicetree and driver issues with the media
>>>>>>> pipeline on i.MX8MM using a RPi v2.1 camera module (imx219) as sensor, I
>>>>>>> now try to get an image from the sensor and run into the next problem.
>>>>>>>
>>>>>>> Below you can find the commands I use and the output I'm getting. Maybe
>>>>>>> someone can see straight away what's wrong or at least can make a guess
>>>>>>> before I start diving into the code. ;)
>>>>>>>
>>>>>>> By the way: This happens on v6.1.11 and 6.2-rc8.
>>>>>>
>>>>>> So it looks like there are several problems (again):
>>>>>>
>>>>>> First I missed to enable the link between the imx219 and the imx-mipi-csis:
>>>>>>
>>>>>> media-ctl -l "'imx219 1-0010':0 -> 'csis-32e30000.mipi-csi':0[1]"
>>>>>>
>>>>>> And the imx-mipi-csis driver is missing a check for the missing source
>>>>>> link which caused the exception. I currently have this applied and will
>>>>>> send this as formal patch later:
>>>>>>
>>>>>> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
>>>>>> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
>>>>>> @@ -596,6 +596,11 @@ static int mipi_csis_calculate_params(struct
>>>>>> mipi_csis_device *csis,
>>>>>>         s64 link_freq;
>>>>>>         u32 lane_rate;
>>>>>>
>>>>>> +       if (!csis->src_sd) {
>>>>>> +               dev_err(csis->dev, "Missing source link\n");
>>>>>> +               return -EINVAL;
>>>>>> +       }
>>>>>> +
>>>>>>         /* Calculate the line rate from the pixel rate. */
>>>>>>         link_freq = v4l2_get_link_freq(csis->src_sd->ctrl_handler,
>>>>>>                                        csis_fmt->width,
>>>>>
>>>>> The pipeline is not correctly configured, and that should have been
>>>>> caught earlier as both pads are created with the
>>>>> MEDIA_PAD_FL_MUST_CONNECT flag. The __media_pipeline_start() function
>>>>> should have return an error. Could you try to check why that didn't
>>>>> happen ?
>>>>
>>>> Thanks for the pointer. I looked at __media_pipeline_start() and to me
>>>> it looks like there's something wrong. During validation of the links,
>>>> there is no code to handle the case where all links are skipped before
>>>> link_validate() is called on them. The loop is left with has_link = true
>>>> and has_enabled_link = true and validation of the pipeline succeeds even
>>>> though there is a missing link.
>>>>
>>>> Does this look like a valid fix to you:
>>>>
>>>> --- a/drivers/media/mc/mc-entity.c
>>>> +++ b/drivers/media/mc/mc-entity.c
>>>> @@ -744,6 +744,7 @@ __must_check int __media_pipeline_start(struct
>>>> media_pad *pad,
>>>>                 struct media_pad *pad = ppad->pad;
>>>>                 struct media_entity *entity = pad->entity;
>>>>                 bool has_enabled_link = false;
>>>> +               bool has_valid_link = false;
>>>>                 bool has_link = false;
>>>>                 struct media_link *link;
>>>>
>>>> @@ -806,6 +807,15 @@ __must_check int __media_pipeline_start(struct
>>>> media_pad *pad,
>>>>                                 link->source->index,
>>>>                                 link->sink->entity->name,
>>>>                                 link->sink->index);
>>>> +
>>>> +                       has_valid_link = true;
>>>> +                       break;
>>>> +               }
>>>> +
>>>> +               if (!has_valid_link) {
>>>> +                       dev_dbg(mdev->dev, "No valid link found");
>>>> +                       ret = -ENOLINK;
>>>> +                       goto error;
>>>>                 }
>>>>
>>>>
>>>
>>> On second thought, I see that this is probably not a correct fix. But I
>>> still think the current code has a flaw. Or maybe I'm missing something
>>> important again. ;)
>>
>> Looks like the pipeline validation is only run for the pads of the links
>> that are enabled. As the following output shows, the pad
>> 'csis-32e30000.mipi-csi':0 is not part of the pipeline and the link
>> 'csis-32e30000.mipi-csi':0 -> 'imx219 1-0010':0 is therefore not part of
>> the validation in __media_pipeline_start().
>>
>> [   36.069274] imx7-csi 32e20000.csi: media pipeline populated, found pads:
>> [   36.080901] imx7-csi 32e20000.csi: - 'csi capture':0
>> [   36.085926] imx7-csi 32e20000.csi: - 'csi':1
>> [   36.090222] imx7-csi 32e20000.csi: - 'csi':0
>> [   36.094524] imx7-csi 32e20000.csi: - 'csis-32e30000.mipi-csi':1
>>
>> So the first time the disabled link is detected is in the driver in
>> mipi_csis_calculate_params() which leads to the crash.
> 
> Of course ! That's what I was missing. Indeed, we have an issue there.
> I'll try to cook up a patch.

Great! Thanks!

> 
>>>>>> Now with this resolved, I get:
>>>>>>
>>>>>> v4l2-ctl -d /dev/video0
>>>>>> --set-fmt-video=width=640,height=480,pixelformat=RG10 --stream-mmap
>>>>>> [  574.758110] imx7-csi 32e20000.csi: pipeline start failed with -32
>>>>>>                 VIDIOC_STREAMON returned -1 (Broken pipe)
>>>>>>
>>>>>> So still not there, but a bit closer ;)
>>>>>> Probably I'm doing something wrong when setting up the format, etc.
>>>>>
>>>>> Quite likely :-) Have you configured formats on all subdevs through the
>>>>> pipeline with media-ctl ?
>>>>>
>>>>
>>>> I'm doing the following:
>>>>
>>>> media-ctl -l "'imx219 1-0010':0 -> 'csis-32e30000.mipi-csi':0[1]"
>>>> media-ctl -d /dev/media0 -V '"imx219 1-0010":0[fmt:SBGGR10_1X10/640x480
>>>> field:none]'
>>>> media-ctl -d /dev/media0 -V
>>>> '"csis-32e30000.mipi-csi":0[fmt:SBGGR10_1X10/640x480 field:none]'
>>>> media-ctl -d /dev/media0 -V '"csi":0[fmt:SBGGR10_1X10/640x480 field:none]'
>>>>
>>>> Is there more I need to do? Sorry, I still lack a lot of understanding
>>>> and experience on how to use the media framework.
>>>>
>>>> But I guess in some way it's also good, as I can provide some testing
>>>> for the error handling, that you would probably miss otherwise as you
>>>> know how to setup things properly. ;)
>>
>> So, I found out that I used SBGGR10_1X10 but the sensor only supports
>> SRGGB10_1X10. Now the pipeline seems to work.
> 
> Great !
> 
> On a side note, if you don't want to deal with the complexity of
> configuring the pipeline, libcamera (https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flibcamera.org%2F&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ce522d5e5757e4a6d58b608db0f813049%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638120819750552954%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GaL%2Bjlm0pcKs2Ijy23UQ6PzWuDVxpTTkjbhuiBASoJw%3D&reserved=0) can do it
> for you :-)

Awesome, thanks for the note! I will give libcamera a try.