Hi Laurent,
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.
Sorry for digging out this old thread. I just wanted to ask if you
managed to come up with a fix for the issue above?
Maybe it's already there and I just missed it?
If I remember correctly it's only about handling a misconfigured
pipeline so it won't be triggered if things are done properly.
But as I've managed to trigger it, someone in the future might too and
it would be nice to make the framework more robust against this.
Thanks
Frieder
