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

[Frieder Schrempf]

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. ;)

>>
>>> 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. ;)
> 
> Thanks
> Frieder