| Message ID | 1370020027-27595-4-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 31, 2013 at 08:07:03PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > plane->enabled is never set, so this code didn't do anything. > > Fix the code for sprites by calling the new drm_plane_force_disable() > function. That means the plane will remain off until someone explicitly > turns it back on. > > And do the same for cursors and the old video overlays, since we only > want to see the primary plane for fbdev. > > v2: Disable sprites/cursors until explicitly re-enabled > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_fb.c | 9 ++------- > 3 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 944b6d5..1d1a3fd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9842,3 +9842,19 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m, > } > } > #endif > + > +void intel_disable_cursors_and_sprites(struct drm_device *dev) > +{ > + struct drm_crtc *crtc; > + struct drm_plane *plane; > + > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + intel_crtc_dpms_overlay(to_intel_crtc(crtc), false); > + intel_crtc_cursor_set(crtc, NULL, 0, 0, 0); > + } > + > + list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > + if (plane->fb) > + drm_plane_force_disable(plane); This would be neater if drm_plane_force_disable did the check for plane-fb itself. -Chris
On Fri, May 31, 2013 at 08:07:03PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > plane->enabled is never set, so this code didn't do anything. > > Fix the code for sprites by calling the new drm_plane_force_disable() > function. That means the plane will remain off until someone explicitly > turns it back on. > > And do the same for cursors and the old video overlays, since we only > want to see the primary plane for fbdev. > > v2: Disable sprites/cursors until explicitly re-enabled > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_fb.c | 9 ++------- > 3 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 944b6d5..1d1a3fd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9842,3 +9842,19 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m, > } > } > #endif > + > +void intel_disable_cursors_and_sprites(struct drm_device *dev) > +{ > + struct drm_crtc *crtc; > + struct drm_plane *plane; > + > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + intel_crtc_dpms_overlay(to_intel_crtc(crtc), false); > + intel_crtc_cursor_set(crtc, NULL, 0, 0, 0); > + } I still think we should just move this into drm_fb_helper_restore_fbdev_mode: - Cursors can be killed with crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0); Or at least we should be able to do so, I've done a quick audit of all drivers and none really cares about file_priv if the handle is 0. Of course we should put that into a drm_cursor_force_disable helper to prepare for a better world (atomic modeset and all ...). vmwgfx needs to be fixed up slightly since it derefences file_priv outside of a handle != 0 check, but that one can be trivially fixed for both cursor_set callbacks by moving the driver_private dereference into the if block. - Legacy overlay stuff has been broken like that forever, and at least on i8xx those are used much more often. Imo whoever's offended by gen2 being a bit broken can write a drm plane wrapper for the overlay stuff (or better, move the code to drm planes and convert the ioctl to be a shim around the real drm plane interfaces). I don't think we should add hacks for that. Cheers, Daniel > + > + list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > + if (plane->fb) > + drm_plane_force_disable(plane); > + } > +} > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d0a8c1a..f8e76cf 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -800,5 +800,6 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, > extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev, > enum transcoder pch_transcoder, > bool enable); > +extern void intel_disable_cursors_and_sprites(struct drm_device *dev); > > #endif /* __INTEL_DRV_H__ */ > diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c > index 5fe525a..b34ccf3 100644 > --- a/drivers/gpu/drm/i915/intel_fb.c > +++ b/drivers/gpu/drm/i915/intel_fb.c > @@ -293,22 +293,17 @@ void intel_fb_restore_mode(struct drm_device *dev) > { > int ret; > drm_i915_private_t *dev_priv = dev->dev_private; > - struct drm_mode_config *config = &dev->mode_config; > - struct drm_plane *plane; > > if (INTEL_INFO(dev)->num_pipes == 0) > return; > > drm_modeset_lock_all(dev); > > + intel_disable_cursors_and_sprites(dev); > + > ret = drm_fb_helper_restore_fbdev_mode(&dev_priv->fbdev->helper); > if (ret) > DRM_DEBUG("failed to restore crtc mode\n"); > > - /* Be sure to shut off any planes that may be active */ > - list_for_each_entry(plane, &config->plane_list, head) > - if (plane->enabled) > - plane->funcs->disable_plane(plane); > - > drm_modeset_unlock_all(dev); > } > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 03, 2013 at 02:00:00PM +0200, Daniel Vetter wrote: > On Fri, May 31, 2013 at 08:07:03PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > plane->enabled is never set, so this code didn't do anything. > > > > Fix the code for sprites by calling the new drm_plane_force_disable() > > function. That means the plane will remain off until someone explicitly > > turns it back on. > > > > And do the same for cursors and the old video overlays, since we only > > want to see the primary plane for fbdev. > > > > v2: Disable sprites/cursors until explicitly re-enabled > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_fb.c | 9 ++------- > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 944b6d5..1d1a3fd 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9842,3 +9842,19 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m, > > } > > } > > #endif > > + > > +void intel_disable_cursors_and_sprites(struct drm_device *dev) > > +{ > > + struct drm_crtc *crtc; > > + struct drm_plane *plane; > > + > > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > > + intel_crtc_dpms_overlay(to_intel_crtc(crtc), false); > > + intel_crtc_cursor_set(crtc, NULL, 0, 0, 0); > > + } > > I still think we should just move this into drm_fb_helper_restore_fbdev_mode: > - Cursors can be killed with crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0); > Or at least we should be able to do so, I've done a quick audit of all > drivers and none really cares about file_priv if the handle is 0. Of > course we should put that into a drm_cursor_force_disable helper to > prepare for a better world (atomic modeset and all ...). > > vmwgfx needs to be fixed up slightly since it derefences file_priv > outside of a handle != 0 check, but that one can be trivially fixed for > both cursor_set callbacks by moving the driver_private dereference into > the if block. > > - Legacy overlay stuff has been broken like that forever, and at least on > i8xx those are used much more often. Imo whoever's offended by gen2 > being a bit broken can write a drm plane wrapper for the overlay stuff > (or better, move the code to drm planes and convert the ioctl to be a > shim around the real drm plane interfaces). I don't think we should add > hacks for that. OK I give up. I did a quick check of the private plane stuff, and it turns out they're not on plane_list, so I think just walking the list and disabling everything there should be OK. Since you're happy w/ leaving the video overlay out in the cold for now, I'll shovel it all into drm_fb_helper...
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 944b6d5..1d1a3fd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9842,3 +9842,19 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m, } } #endif + +void intel_disable_cursors_and_sprites(struct drm_device *dev) +{ + struct drm_crtc *crtc; + struct drm_plane *plane; + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + intel_crtc_dpms_overlay(to_intel_crtc(crtc), false); + intel_crtc_cursor_set(crtc, NULL, 0, 0, 0); + } + + list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + if (plane->fb) + drm_plane_force_disable(plane); + } +} diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d0a8c1a..f8e76cf 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -800,5 +800,6 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev, enum transcoder pch_transcoder, bool enable); +extern void intel_disable_cursors_and_sprites(struct drm_device *dev); #endif /* __INTEL_DRV_H__ */ diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c index 5fe525a..b34ccf3 100644 --- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c @@ -293,22 +293,17 @@ void intel_fb_restore_mode(struct drm_device *dev) { int ret; drm_i915_private_t *dev_priv = dev->dev_private; - struct drm_mode_config *config = &dev->mode_config; - struct drm_plane *plane; if (INTEL_INFO(dev)->num_pipes == 0) return; drm_modeset_lock_all(dev); + intel_disable_cursors_and_sprites(dev); + ret = drm_fb_helper_restore_fbdev_mode(&dev_priv->fbdev->helper); if (ret) DRM_DEBUG("failed to restore crtc mode\n"); - /* Be sure to shut off any planes that may be active */ - list_for_each_entry(plane, &config->plane_list, head) - if (plane->enabled) - plane->funcs->disable_plane(plane); - drm_modeset_unlock_all(dev); }