| Message ID | 1395188579-17191-5-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Mar 18, 2014 at 05:22:49PM -0700, Matt Roper wrote: > Before we add additional types of planes to the DRM plane list, ensure > that existing loops over all planes continue to operate only on > "overlay" planes and ignore primary & cursor planes. > > Cc: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/exynos/exynos_drm_encoder.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > index 06f1b2a..2fa2685 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > @@ -127,6 +127,9 @@ static void disable_plane_to_crtc(struct drm_device *dev, > * (encoder->crtc) > */ > list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > + if (plane->type != DRM_PLANE_TYPE_OVERLAY) I think a drm_for_each_legacy_plane iteration helper would be neat for this one and the following i915 patch. -Daniel > + continue; > + > if (plane->crtc == old_crtc) { > /* > * do not change below call order. > @@ -247,6 +250,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder) > > /* all planes connected to this encoder should be also disabled. */ > list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > + if (plane->type != DRM_PLANE_TYPE_OVERLAY) > + continue; > + > if (plane->crtc == encoder->crtc) > plane->funcs->disable_plane(plane); > } > -- > 1.8.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Mar 19, 2014 at 7:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Mar 18, 2014 at 05:22:49PM -0700, Matt Roper wrote: >> Before we add additional types of planes to the DRM plane list, ensure >> that existing loops over all planes continue to operate only on >> "overlay" planes and ignore primary & cursor planes. >> >> Cc: Inki Dae <inki.dae@samsung.com> >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> index 06f1b2a..2fa2685 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> @@ -127,6 +127,9 @@ static void disable_plane_to_crtc(struct drm_device *dev, >> * (encoder->crtc) >> */ >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) > > I think a drm_for_each_legacy_plane iteration helper would be neat for > this one and the following i915 patch. > -Daniel > >> + continue; >> + >> if (plane->crtc == old_crtc) { >> /* >> * do not change below call order. >> @@ -247,6 +250,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder) >> >> /* all planes connected to this encoder should be also disabled. */ >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) >> + continue; >> + >> if (plane->crtc == encoder->crtc) >> plane->funcs->disable_plane(plane); >> } The original loop disables all planes attached to a crtc when disabling an encoder attached to the same crtc. It was added by: commit bcf4cef94294992f7cd11d5a90fa58b0eae6c795 Author: Inki Dae <inki.dae@samsung.com> Date: Fri Aug 24 10:54:12 2012 -0700 drm/exynos: Disable plane when released this patch ensures that each plane connected to encoder is disabled when released, by adding disable callback function of encoder helper we had faced with one issue that invalid memory is accessed by dma once drm is released and then the dma is turned on again. actually, in our case, page fault was incurred with iommu. the reason is that a gem buffer accessed by the dma is also released once drm is released. so this patch would fix this issue ensuring the dma is disabled when released. An encoder receives and encodes the mixed output of all of the planes/overlays. It would seem that whether the individual planes themselves are enabled or not should be completely independent of the status any encoder. However, I find the code in exynos_drm_encoder.c very difficult to follow, so perhaps there is some extra linkage between encoder/crtc/plane that is exynos specific. In any case, judging from the commit message, this disable loop should probably still iterate over all of the planes, not just the "DRM_PLANE_TYPE_OVERLAY" ones. So, I think this new patch is incorrect. >> -- >> 1.8.5.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Mar 19, 2014 at 10:26:13PM +0800, Daniel Kurtz wrote: > On Wed, Mar 19, 2014 at 7:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Mar 18, 2014 at 05:22:49PM -0700, Matt Roper wrote: > >> Before we add additional types of planes to the DRM plane list, ensure > >> that existing loops over all planes continue to operate only on > >> "overlay" planes and ignore primary & cursor planes. > >> > >> Cc: Inki Dae <inki.dae@samsung.com> > >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >> --- > >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >> index 06f1b2a..2fa2685 100644 > >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >> @@ -127,6 +127,9 @@ static void disable_plane_to_crtc(struct drm_device *dev, > >> * (encoder->crtc) > >> */ > >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) > > > > I think a drm_for_each_legacy_plane iteration helper would be neat for > > this one and the following i915 patch. > > -Daniel > > > >> + continue; > >> + > >> if (plane->crtc == old_crtc) { > >> /* > >> * do not change below call order. > >> @@ -247,6 +250,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder) > >> > >> /* all planes connected to this encoder should be also disabled. */ > >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) > >> + continue; > >> + > >> if (plane->crtc == encoder->crtc) > >> plane->funcs->disable_plane(plane); > >> } > > The original loop disables all planes attached to a crtc when > disabling an encoder attached to the same crtc. It was added by: > > commit bcf4cef94294992f7cd11d5a90fa58b0eae6c795 > Author: Inki Dae <inki.dae@samsung.com> > Date: Fri Aug 24 10:54:12 2012 -0700 > > drm/exynos: Disable plane when released > > this patch ensures that each plane connected to encoder is disabled > when released, by adding disable callback function of encoder helper > > we had faced with one issue that invalid memory is accessed by dma > once drm is released and then the dma is turned on again. actually, > in our case, page fault was incurred with iommu. the reason is that > a gem buffer accessed by the dma is also released once drm is released. > > so this patch would fix this issue ensuring the dma is disabled > when released. > > > An encoder receives and encodes the mixed output of all of the > planes/overlays. It would seem that whether the individual planes > themselves are enabled or not should be completely independent of the > status any encoder. However, I find the code in exynos_drm_encoder.c > very difficult to follow, so perhaps there is some extra linkage > between encoder/crtc/plane that is exynos specific. > > In any case, judging from the commit message, this disable loop should > probably still iterate over all of the planes, not just the > "DRM_PLANE_TYPE_OVERLAY" ones. So, I think this new patch is > incorrect. It keeps full backwars compatibility with existing semantics, which is the right thing to do in such a case. It could be that exynos simply has a bug, but imo that should be a separate patch outside of this series. -Daniel
On Thu, Mar 20, 2014 at 3:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Mar 19, 2014 at 10:26:13PM +0800, Daniel Kurtz wrote: >> On Wed, Mar 19, 2014 at 7:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Tue, Mar 18, 2014 at 05:22:49PM -0700, Matt Roper wrote: >> >> Before we add additional types of planes to the DRM plane list, ensure >> >> that existing loops over all planes continue to operate only on >> >> "overlay" planes and ignore primary & cursor planes. >> >> >> >> Cc: Inki Dae <inki.dae@samsung.com> >> >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> >> --- >> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 6 ++++++ >> >> 1 file changed, 6 insertions(+) >> >> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> >> index 06f1b2a..2fa2685 100644 >> >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> >> @@ -127,6 +127,9 @@ static void disable_plane_to_crtc(struct drm_device *dev, >> >> * (encoder->crtc) >> >> */ >> >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { >> >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) >> > >> > I think a drm_for_each_legacy_plane iteration helper would be neat for >> > this one and the following i915 patch. >> > -Daniel >> > >> >> + continue; >> >> + >> >> if (plane->crtc == old_crtc) { >> >> /* >> >> * do not change below call order. >> >> @@ -247,6 +250,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder) >> >> >> >> /* all planes connected to this encoder should be also disabled. */ >> >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { >> >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) >> >> + continue; >> >> + >> >> if (plane->crtc == encoder->crtc) >> >> plane->funcs->disable_plane(plane); >> >> } >> >> The original loop disables all planes attached to a crtc when >> disabling an encoder attached to the same crtc. It was added by: >> >> commit bcf4cef94294992f7cd11d5a90fa58b0eae6c795 >> Author: Inki Dae <inki.dae@samsung.com> >> Date: Fri Aug 24 10:54:12 2012 -0700 >> >> drm/exynos: Disable plane when released >> >> this patch ensures that each plane connected to encoder is disabled >> when released, by adding disable callback function of encoder helper >> >> we had faced with one issue that invalid memory is accessed by dma >> once drm is released and then the dma is turned on again. actually, >> in our case, page fault was incurred with iommu. the reason is that >> a gem buffer accessed by the dma is also released once drm is released. >> >> so this patch would fix this issue ensuring the dma is disabled >> when released. >> >> >> An encoder receives and encodes the mixed output of all of the >> planes/overlays. It would seem that whether the individual planes >> themselves are enabled or not should be completely independent of the >> status any encoder. However, I find the code in exynos_drm_encoder.c >> very difficult to follow, so perhaps there is some extra linkage >> between encoder/crtc/plane that is exynos specific. >> >> In any case, judging from the commit message, this disable loop should >> probably still iterate over all of the planes, not just the >> "DRM_PLANE_TYPE_OVERLAY" ones. So, I think this new patch is >> incorrect. > > It keeps full backwars compatibility with existing semantics, which is the > right thing to do in such a case. It could be that exynos simply has a > bug, but imo that should be a separate patch outside of this series. Indeed... I missed the fact that in the existing code, the "priv" (now primary) plane is not added to the plane_list, so it wouldn't actually be disabled in this loop here anyway. New question: are the planes that will become DRM_PLANE_TYPE_CURSOR formerly "priv", or not? If they were not, then I think the backwards- and forwards- compatible loop is: list_for_each_entry(plane, &dev->mode_config.plane_list, head) { if (plane->type == DRM_PLANE_TYPE_PRIMARY) continue; /* do something with legacy planes */ } > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Mar 20, 2014 at 09:56:24AM +0800, Daniel Kurtz wrote: > On Thu, Mar 20, 2014 at 3:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Mar 19, 2014 at 10:26:13PM +0800, Daniel Kurtz wrote: > >> On Wed, Mar 19, 2014 at 7:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > >> > On Tue, Mar 18, 2014 at 05:22:49PM -0700, Matt Roper wrote: > >> >> Before we add additional types of planes to the DRM plane list, ensure > >> >> that existing loops over all planes continue to operate only on > >> >> "overlay" planes and ignore primary & cursor planes. > >> >> > >> >> Cc: Inki Dae <inki.dae@samsung.com> > >> >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >> >> --- > >> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 6 ++++++ > >> >> 1 file changed, 6 insertions(+) > >> >> > >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >> >> index 06f1b2a..2fa2685 100644 > >> >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >> >> @@ -127,6 +127,9 @@ static void disable_plane_to_crtc(struct drm_device *dev, > >> >> * (encoder->crtc) > >> >> */ > >> >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > >> >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) > >> > > >> > I think a drm_for_each_legacy_plane iteration helper would be neat for > >> > this one and the following i915 patch. > >> > -Daniel > >> > > >> >> + continue; > >> >> + > >> >> if (plane->crtc == old_crtc) { > >> >> /* > >> >> * do not change below call order. > >> >> @@ -247,6 +250,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder) > >> >> > >> >> /* all planes connected to this encoder should be also disabled. */ > >> >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > >> >> + if (plane->type != DRM_PLANE_TYPE_OVERLAY) > >> >> + continue; > >> >> + > >> >> if (plane->crtc == encoder->crtc) > >> >> plane->funcs->disable_plane(plane); > >> >> } > >> > >> The original loop disables all planes attached to a crtc when > >> disabling an encoder attached to the same crtc. It was added by: > >> > >> commit bcf4cef94294992f7cd11d5a90fa58b0eae6c795 > >> Author: Inki Dae <inki.dae@samsung.com> > >> Date: Fri Aug 24 10:54:12 2012 -0700 > >> > >> drm/exynos: Disable plane when released > >> > >> this patch ensures that each plane connected to encoder is disabled > >> when released, by adding disable callback function of encoder helper > >> > >> we had faced with one issue that invalid memory is accessed by dma > >> once drm is released and then the dma is turned on again. actually, > >> in our case, page fault was incurred with iommu. the reason is that > >> a gem buffer accessed by the dma is also released once drm is released. > >> > >> so this patch would fix this issue ensuring the dma is disabled > >> when released. > >> > >> > >> An encoder receives and encodes the mixed output of all of the > >> planes/overlays. It would seem that whether the individual planes > >> themselves are enabled or not should be completely independent of the > >> status any encoder. However, I find the code in exynos_drm_encoder.c > >> very difficult to follow, so perhaps there is some extra linkage > >> between encoder/crtc/plane that is exynos specific. > >> > >> In any case, judging from the commit message, this disable loop should > >> probably still iterate over all of the planes, not just the > >> "DRM_PLANE_TYPE_OVERLAY" ones. So, I think this new patch is > >> incorrect. > > > > It keeps full backwars compatibility with existing semantics, which is the > > right thing to do in such a case. It could be that exynos simply has a > > bug, but imo that should be a separate patch outside of this series. > > Indeed... I missed the fact that in the existing code, the "priv" > (now primary) plane is not added to the plane_list, so it wouldn't > actually be disabled in this loop here anyway. > > New question: are the planes that will become DRM_PLANE_TYPE_CURSOR > formerly "priv", or not? > If they were not, then I think the backwards- and forwards- compatible loop is: > > list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > continue; > /* do something with legacy planes */ > } Cursor planes haven't been exposed at all as plane objects before, not even as private driver objects. So they'd need to be excluded here too. -Daniel
diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c index 06f1b2a..2fa2685 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c @@ -127,6 +127,9 @@ static void disable_plane_to_crtc(struct drm_device *dev, * (encoder->crtc) */ list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + if (plane->type != DRM_PLANE_TYPE_OVERLAY) + continue; + if (plane->crtc == old_crtc) { /* * do not change below call order. @@ -247,6 +250,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder) /* all planes connected to this encoder should be also disabled. */ list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + if (plane->type != DRM_PLANE_TYPE_OVERLAY) + continue; + if (plane->crtc == encoder->crtc) plane->funcs->disable_plane(plane); }
Before we add additional types of planes to the DRM plane list, ensure that existing loops over all planes continue to operate only on "overlay" planes and ignore primary & cursor planes. Cc: Inki Dae <inki.dae@samsung.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/exynos/exynos_drm_encoder.c | 6 ++++++ 1 file changed, 6 insertions(+)