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