On 02/02/2016 04:09 PM, Sakari Ailus wrote:
> Hi Shuah,
>
> On Tue, Feb 02, 2016 at 04:04:50PM -0700, Shuah Khan wrote:
>> On 02/02/2016 03:53 PM, Sakari Ailus wrote:
>>> Hi Shuah,
>>>
>>> On Sat, Jan 30, 2016 at 01:10:52PM -0700, Shuah Khan wrote:
>>>> Change media_entity_pipeline_stop() to not decrement
>>>> stream_count of an inactive media pipeline. Doing so,
>>>> results in preventing starting the pipeline.
>>>>
>>>> Signed-off-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
>>>> ---
>>>> drivers/media/media-entity.c | 18 ++++++++++++------
>>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>>> index e89d85a..f2e4360 100644
>>>> --- a/drivers/media/media-entity.c
>>>> +++ b/drivers/media/media-entity.c
>>>> @@ -452,9 +452,12 @@ error:
>>>> media_entity_graph_walk_start(graph, entity_err);
>>>>
>>>> while ((entity_err = media_entity_graph_walk_next(graph))) {
>>>> - entity_err->stream_count--;
>>>> - if (entity_err->stream_count == 0)
>>>> - entity_err->pipe = NULL;
>>>> + /* don't let the stream_count go negative */
>>>> + if (entity->stream_count > 0) {
>>>> + entity_err->stream_count--;
>>>> + if (entity_err->stream_count == 0)
>>>> + entity_err->pipe = NULL;
>>>> + }
>>>>
>>>> /*
>>>> * We haven't increased stream_count further than this
>>>> @@ -486,9 +489,12 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>>>> media_entity_graph_walk_start(graph, entity);
>>>>
>>>> while ((entity = media_entity_graph_walk_next(graph))) {
>>>> - entity->stream_count--;
>>>> - if (entity->stream_count == 0)
>>>> - entity->pipe = NULL;
>>>> + /* don't let the stream_count go negative */
>>>> + if (entity->stream_count > 0) {
>>>> + entity->stream_count--;
>>>> + if (entity->stream_count == 0)
>>>> + entity->pipe = NULL;
>>>> + }
>>>> }
>>>>
>>>> if (!--pipe->streaming_count)
>>>
>>> Have you seen issues with a certain driver, for instance?
>>>
>>> In the original design the streaming count is really a count --- streaming
>>> starts when count becomes non-zero, and stops when it reaches zero again.
>>>
>>> The calls to media_entity_pipeline_start() and media_entity_pipeline_stop()
>>> should thus always be balanced. I'm fine with the patch, but the framework
>>> should shout loud when the count would be decremented when it's zero.
>>>
>>> That was some four or more years ago. I have to say I really haven't been
>>> able to see good reasons for making this a count --- rather what's needed is
>>> to mark the entity as busy so that its link configuration isn't touched. The
>>> request API is a completely different matter then.
>>>
>>> Such change would require more discussion IMHO.
>>>
>>
>> Yes. I found problems with au0828 and ALSA
>> media controller use-case. It got into a state
>> where pipeline was essentially locked in a state.
>> It took some work to debug and find that the
>> stream_count was negative.
>>
>> Granted the start and stop should be balanced, however,
>> the media_entity_pipeline_stop() still needs to protect
>> the stream_count from going negative.
>
> Agreed; I'm fine with the patch, except I think it should complain when that
> happens rather than silently ignoring it.
>
> WARN_ON(1) perhaps? Or how about dev_err() loudly complaining about a driver
> bug?
>
Yes. I can add a WARN_ON(1). Looks like we are using
it in other places in media-entity.c
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
