diff mbox

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

Message ID 1395188579-17191-5-git-send-email-matthew.d.roper@intel.com
State Superseded
Headers show

Commit Message

Matt Roper March 19, 2014, 12:22 a.m. UTC
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(+)

Comments

Daniel Vetter March 19, 2014, 11:51 a.m. UTC | #1
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
Daniel Kurtz March 19, 2014, 2:26 p.m. UTC | #2
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
Daniel Vetter March 19, 2014, 7:31 p.m. UTC | #3
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
Daniel Kurtz March 20, 2014, 1:56 a.m. UTC | #4
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
Daniel Vetter March 20, 2014, 3:35 p.m. UTC | #5
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 mbox

Patch

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);
 	}