On Thu, Jan 25, 2018 at 01:27:27PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 25, 2018 at 10:17:21AM +0100, Maarten Lankhorst wrote:
> > Op 24-01-18 om 22:47 schreef Ville Syrjala:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > We use 32bit bitmasks to track planes/crtcs/encoders/connectors.
> > > Naturally we can only do that if the index of those objects stays
> > > below 32. Issue a warning whenever we exceed that limit, hopefully
> > > prompting someone to fix the problem.
> > >
> > > For connectors the issue is a bit more complicated as they can
> > > be created/destroyed at runtime due to MST. So the problem is no
> > > longer a purely theoretical programmer error. As the connector
> > > indexes are allocated via ida, we can simply limit the maximum
> > > value the ida is allowed to hand out. The error handling is already
> > > in place.
> > >
> > > v2: Return an error to the caller (Harry)
> > >
> > > Cc: Harry Wentland <harry.wentland@xxxxxxx>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/drm_connector.c | 3 ++-
> > > drivers/gpu/drm/drm_crtc.c | 4 ++++
> > > drivers/gpu/drm/drm_encoder.c | 4 ++++
> > > drivers/gpu/drm/drm_plane.c | 4 ++++
> > > 4 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index b85a7749709d..f4a689ab990a 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -205,7 +205,8 @@ int drm_connector_init(struct drm_device *dev,
> > > connector->dev = dev;
> > > connector->funcs = funcs;
> > >
> > > - ret = ida_simple_get(&config->connector_ida, 0, 0, GFP_KERNEL);
> > > + /* connector index is used with 32bit bitmasks */
> > > + ret = ida_simple_get(&config->connector_ida, 0, 32, GFP_KERNEL);
> > > if (ret < 0)
> > > goto out_put;
> > > connector->index = ret;
> > Could we also put a DRM_DEBUG_KMS in here if allocation fails? Since it's more likely to happen now..
>
> I was actually pondering using DRM_ERROR(), or maybe even WARN(). It
> shouldn't be directly user triggerable so I think we want to get the
> bugreport if we do run out of bits.
OTOH we are likely to get the "my screen don't work" bugreport anyway,
so maybe a debug print is enough.
>
> >
> > Otherwise looks good so with that change:
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index f0556e654116..5b4be382a1d7 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -282,6 +282,10 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> > > WARN_ON(primary && primary->type != DRM_PLANE_TYPE_PRIMARY);
> > > WARN_ON(cursor && cursor->type != DRM_PLANE_TYPE_CURSOR);
> > >
> > > + /* crtc index is used with 32bit bitmasks */
> > > + if (WARN_ON(config->num_crtc >= 32))
> > > + return -EINVAL;
> > > +
> > > crtc->dev = dev;
> > > crtc->funcs = funcs;
> > >
> > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > > index 59e0ebe733f8..273e1c59c54a 100644
> > > --- a/drivers/gpu/drm/drm_encoder.c
> > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > @@ -110,6 +110,10 @@ int drm_encoder_init(struct drm_device *dev,
> > > {
> > > int ret;
> > >
> > > + /* encoder index is used with 32bit bitmasks */
> > > + if (WARN_ON(dev->mode_config.num_encoder >= 32))
> > > + return -EINVAL;
> > > +
> > > ret = drm_mode_object_add(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
> > > if (ret)
> > > return ret;
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 2c90519576a3..22b54663b6e7 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -173,6 +173,10 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> > > unsigned int format_modifier_count = 0;
> > > int ret;
> > >
> > > + /* plane index is used with 32bit bitmasks */
> > > + if (WARN_ON(config->num_total_plane >= 32))
> > > + return -EINVAL;
> > > +
> > > ret = drm_mode_object_add(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> > > if (ret)
> > > return ret;
> >
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
