| Message ID | 1479469214-24714-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 18-11-16 om 12:40 schreef Tvrtko Ursulin: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > It just says "plane X disabled" which does not seem very useful. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Could we remove plane dumping altogether? It's not safe to do so in the way done by this function, and we could add missing things in intel_plane_atomic_calc_changes. ~Maarten
On Fri, Nov 18, 2016 at 03:13:14PM +0100, Maarten Lankhorst wrote: > Op 18-11-16 om 12:40 schreef Tvrtko Ursulin: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > It just says "plane X disabled" which does not seem very useful. > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Could we remove plane dumping altogether? It's not safe to do so in the way done by this function, and we could add missing things in intel_plane_atomic_calc_changes. Yeah, might make sense to dump that stuff only from some plane code. And we should probably take a good look at Rob Clark's new state dumping stuff for that, so that we'll get it in some decently standardized format. I must admit that I didn't take a very good look at Rob's stuff, but maybe he already added the dumps to some useful places, and all we'd have to do is expand the dumps with our own derived plane state...
On 18/11/2016 14:19, Ville Syrjälä wrote: > On Fri, Nov 18, 2016 at 03:13:14PM +0100, Maarten Lankhorst wrote: >> Op 18-11-16 om 12:40 schreef Tvrtko Ursulin: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> It just says "plane X disabled" which does not seem very useful. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Could we remove plane dumping altogether? It's not safe to do so in the way done by this function, and we could add missing things in intel_plane_atomic_calc_changes. > > Yeah, might make sense to dump that stuff only from some plane code. And > we should probably take a good look at Rob Clark's new state dumping stuff > for that, so that we'll get it in some decently standardized format. > > I must admit that I didn't take a very good look at Rob's stuff, but > maybe he already added the dumps to some useful places, and all we'd > have to do is expand the dumps with our own derived plane state... Shall I leave it with you guys then? The most I would feel confident in this area is to submit a patch which removes the plane debug from intel_dump_pipe_config if that is broken. But not sure how useful would that be without this other work you are discussing. Regards, Tvrtko
Op 21-11-16 om 11:50 schreef Tvrtko Ursulin: > > On 18/11/2016 14:19, Ville Syrjälä wrote: >> On Fri, Nov 18, 2016 at 03:13:14PM +0100, Maarten Lankhorst wrote: >>> Op 18-11-16 om 12:40 schreef Tvrtko Ursulin: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> It just says "plane X disabled" which does not seem very useful. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Could we remove plane dumping altogether? It's not safe to do so in the way done by this function, and we could add missing things in intel_plane_atomic_calc_changes. >> >> Yeah, might make sense to dump that stuff only from some plane code. And >> we should probably take a good look at Rob Clark's new state dumping stuff >> for that, so that we'll get it in some decently standardized format. >> >> I must admit that I didn't take a very good look at Rob's stuff, but >> maybe he already added the dumps to some useful places, and all we'd >> have to do is expand the dumps with our own derived plane state... > > Shall I leave it with you guys then? The most I would feel confident in this area is to submit a patch which removes the plane debug from intel_dump_pipe_config if that is broken. But not sure how useful would that be without this other work you are discussing. Intel plane dumping is broken regardless, so feel I think it's good to submit a patch to nuke it. :)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 155910c93896..d35ffb40ca67 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12732,9 +12732,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = to_i915(dev); struct drm_plane *plane; - struct intel_plane *intel_plane; - struct intel_plane_state *state; - struct drm_framebuffer *fb; + bool planes = false; DRM_DEBUG_KMS("[CRTC:%d:%s]%s\n", crtc->base.base.id, crtc->base.name, context); @@ -12822,19 +12820,19 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, pipe_config->dpll_hw_state.fp1); } - DRM_DEBUG_KMS("planes on this crtc\n"); list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + struct intel_plane *intel_plane = to_intel_plane(plane); + struct intel_plane_state *state = + to_intel_plane_state(plane->state); + struct drm_framebuffer *fb = state->base.fb; struct drm_format_name_buf format_name; - intel_plane = to_intel_plane(plane); - if (intel_plane->pipe != crtc->pipe) - continue; - state = to_intel_plane_state(plane->state); - fb = state->base.fb; - if (!fb) { - DRM_DEBUG_KMS("[PLANE:%d:%s] disabled, scaler_id = %d\n", - plane->base.id, plane->name, state->scaler_id); + if (intel_plane->pipe != crtc->pipe || !fb) continue; + + if (!planes) { + DRM_DEBUG_KMS("Planes on this crtc:\n"); + planes = true; } DRM_DEBUG_KMS("[PLANE:%d:%s] FB:%d, fb = %ux%u format = %s\n",