| Message ID | 1398272641-4368-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote: > The DRM core setplane code should check that the plane is usable on the > specified CRTC before calling into the driver. > > Prior to this patch, a plane's possible_crtcs field was purely > informational for userspace and was never actually verified at the > kernel level (aside from the primary plane helper). > > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Do you have a nasty igt somewhere which tries to use a plane on the wrong crtc? Especially useful since our i915 code and hw relies on this. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 7 +++++++ > drivers/gpu/drm/drm_plane_helper.c | 6 ------ > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 461d19b..b6d6c04 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2140,6 +2140,13 @@ int drm_mode_setplane(struct drm_device *dev, void *data, > } > crtc = obj_to_crtc(obj); > > + /* Check whether this plane is usable on this CRTC */ > + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) { > + DRM_DEBUG_KMS("Invalid crtc for plane\n"); > + ret = -EINVAL; > + goto out; > + } > + > fb = drm_framebuffer_lookup(dev, plane_req->fb_id); > if (!fb) { > DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index b72736d..65c4a00 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, > return -EINVAL; > } > > - /* Primary planes are locked to their owning CRTC */ > - if (plane->possible_crtcs != drm_crtc_mask(crtc)) { > - DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); > - return -EINVAL; > - } > - > /* Disallow scaling */ > src_w >>= 16; > src_h >>= 16; > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 23, 2014 at 08:03:50PM +0200, Daniel Vetter wrote: > On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote: > > The DRM core setplane code should check that the plane is usable on the > > specified CRTC before calling into the driver. > > > > Prior to this patch, a plane's possible_crtcs field was purely > > informational for userspace and was never actually verified at the > > kernel level (aside from the primary plane helper). > > > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Do you have a nasty igt somewhere which tries to use a plane on the wrong > crtc? Especially useful since our i915 code and hw relies on this. > -Daniel Not yet; I'll add/modify a test to include that. I'm still working on some other i-g-t test updates for the primary plane stuff based on your previous feedback. Speaking of i-g-t testing, what is the expected behavior if someone issues a pageflip ioctl while the primary plane is disabled? I'd expect it to return an error, but the -EBUSY currently returned by the DRM core seems a bit confusing/misleading in my opinion. The comments indicate that the existing assumption is that crtc->primary->fb == NULL indicates a hotplug event that userspace hasn't noticed yet, although now we clearly have other ways to hit that error, so I'm not sure EBUSY is really the right response. Matt > > > --- > > drivers/gpu/drm/drm_crtc.c | 7 +++++++ > > drivers/gpu/drm/drm_plane_helper.c | 6 ------ > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 461d19b..b6d6c04 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -2140,6 +2140,13 @@ int drm_mode_setplane(struct drm_device *dev, void *data, > > } > > crtc = obj_to_crtc(obj); > > > > + /* Check whether this plane is usable on this CRTC */ > > + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) { > > + DRM_DEBUG_KMS("Invalid crtc for plane\n"); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > fb = drm_framebuffer_lookup(dev, plane_req->fb_id); > > if (!fb) { > > DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > > index b72736d..65c4a00 100644 > > --- a/drivers/gpu/drm/drm_plane_helper.c > > +++ b/drivers/gpu/drm/drm_plane_helper.c > > @@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, > > return -EINVAL; > > } > > > > - /* Primary planes are locked to their owning CRTC */ > > - if (plane->possible_crtcs != drm_crtc_mask(crtc)) { > > - DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); > > - return -EINVAL; > > - } > > - > > /* Disallow scaling */ > > src_w >>= 16; > > src_h >>= 16; > > -- > > 1.8.5.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Apr 23, 2014 at 11:26:04AM -0700, Matt Roper wrote: > On Wed, Apr 23, 2014 at 08:03:50PM +0200, Daniel Vetter wrote: > > On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote: > > > The DRM core setplane code should check that the plane is usable on the > > > specified CRTC before calling into the driver. > > > > > > Prior to this patch, a plane's possible_crtcs field was purely > > > informational for userspace and was never actually verified at the > > > kernel level (aside from the primary plane helper). > > > > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > > Do you have a nasty igt somewhere which tries to use a plane on the wrong > > crtc? Especially useful since our i915 code and hw relies on this. > > -Daniel > > Not yet; I'll add/modify a test to include that. I'm still working on > some other i-g-t test updates for the primary plane stuff based on your > previous feedback. > > Speaking of i-g-t testing, what is the expected behavior if someone > issues a pageflip ioctl while the primary plane is disabled? I'd expect > it to return an error, but the -EBUSY currently returned by the DRM core > seems a bit confusing/misleading in my opinion. The comments indicate > that the existing assumption is that crtc->primary->fb == NULL indicates > a hotplug event that userspace hasn't noticed yet, although now we > clearly have other ways to hit that error, so I'm not sure EBUSY is > really the right response. That comment is outdated since nowadays the kernel doesn't randomly kill a crtc if its outputs gets unplugged. I think a simple -EINVAL should be good. We'd need to update kms_flip with that new value though. A quick grep through the intel ddx shows that we don't really depend on this either way. -EBUSY for a disabled primary plane (whether the crtc is on or not doesn't matter imo) really makes no sense. -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 461d19b..b6d6c04 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2140,6 +2140,13 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } crtc = obj_to_crtc(obj); + /* Check whether this plane is usable on this CRTC */ + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) { + DRM_DEBUG_KMS("Invalid crtc for plane\n"); + ret = -EINVAL; + goto out; + } + fb = drm_framebuffer_lookup(dev, plane_req->fb_id); if (!fb) { DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index b72736d..65c4a00 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, return -EINVAL; } - /* Primary planes are locked to their owning CRTC */ - if (plane->possible_crtcs != drm_crtc_mask(crtc)) { - DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); - return -EINVAL; - } - /* Disallow scaling */ src_w >>= 16; src_h >>= 16;
The DRM core setplane code should check that the plane is usable on the specified CRTC before calling into the driver. Prior to this patch, a plane's possible_crtcs field was purely informational for userspace and was never actually verified at the kernel level (aside from the primary plane helper). Cc: dri-devel@lists.freedesktop.org Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/drm_crtc.c | 7 +++++++ drivers/gpu/drm/drm_plane_helper.c | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-)