Re: [RFCv3 04/14] drm/exynos: Restrict plane loops to only operate on overlay planes

From: Daniel Kurtz <djkurtz@chromium.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [RFCv3 04/14] drm/exynos: Restrict plane loops to only operate on overlay planes
Date: Thu, 20 Mar 2014 09:56:24 +0800	[thread overview]
Message-ID: <CAGS+omCRs41XkeXBOGW_o7mBgEbcj0xKt2yZ0VCyCGc2co-Fcw@mail.gmail.com> (raw)
In-Reply-To: <20140319193132.GK30571@phenom.ffwll.local>

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