On Mon, 2017-04-24 at 13:37 +0200, Peter Rosin wrote:
[...]
> Ok, so the difference is probably that the rwsem locking primitive
> don't have any lockdep checking hooked up. Because the rwsem was
> definitely held in the same way in v13 as the mutex is now held in
> v14, so there's no fundamental difference.
>
> So, yes, we can use some kind of refcount scheme to not hold an actual
> mutex for the duration of the mux select/deselect, but that doesn't
> really change anything. Userspace is still locking something, and that
> seems dangerous. How do you make sure that mux_control_deselect is
> called as it should?
My current video_mux link setup implementation looks like this (it is
called from userspace via the MEDIA_IOC_SETUP_LINK ioctl):
----------8<----------
static int video_mux_link_setup(struct media_entity *entity,
const struct media_pad *local,
const struct media_pad *remote, u32 flags)
{
struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
struct video_mux *video_mux = v4l2_subdev_to_video_mux(sd);
int ret;
/*
* The mux state is determined by the enabled sink pad link.
* Enabling or disabling the source pad link has no effect.
*/
if (!is_sink_pad(video_mux, local->index))
return 0;
dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]", remote->entity->name,
remote->index, local->entity->name, local->index,
flags & MEDIA_LNK_FL_ENABLED);
if (flags & MEDIA_LNK_FL_ENABLED) {
if (video_mux->active == local->index)
return 0;
if (video_mux->active >= 0)
return -EBUSY;
dev_dbg(sd->dev, "setting %d active\n", local->index);
ret = mux_control_try_select(video_mux->mux, local->index);
if (ret < 0)
return ret;
video_mux->active = local->index;
} else {
if (video_mux->active != local->index)
return 0;
dev_dbg(sd->dev, "going inactive\n");
mux_control_deselect(video_mux->mux);
video_mux->active = -1;
}
return 0;
}
---------->8----------
If a mux state was already selected, this should return -EBUSY, until a
call to disable the active link deselects the mux.
> What I don't like about abandoning the lock is that there is still a
> need to support the multi-consumer case and then you need some way
> of keeping track of waiters. A lock does this, and any attempt to open
> code that will get messy.
>
> What might be better is to support some kind of exclusive mux, i.e. a
> mux that only allows one consumer per mux controller. Then the mux core
> could trust that exclusive consumer to not fuck things up for itself and
> thus have no lock at all for select/deselect for the exclusive case. Or
> perhaps keep a refcount (as you suggested) for the exclusive case so
> that mux_control_try_select still makes sense, because you still want
> that, right?
In the case of an exclusive mux without lock, I don't see any need for a
try_select call.
> The question then becomes how to best tell the mux core that you want
> an exclusive mux. I see two options. Either you declare a mux controller
> as exclusive up front somehow (in the device tree presumably), or you
> add a mux_control_get_exclusive call that makes further calls to
> mux_control_get{,_exclusive} fail with -EBUSY. I think I like the
> latter better, if that can be made to work...
There is a precedent for the latter in the reset controller framework
(reset_control_get_shared and reset_control_get_exclusive variants).
Since this distinction is a matter of usage, and not a hardware property
of the mux/reset controller itself, I'd also prefer that.
regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
