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@xxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxxxxx>
> >> >> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >> >> ---
> >> >> 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@xxxxxxxxxxx>
> >> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
