diff mbox

[PATCHv4,04/13] drm/shmobile: Restrict plane loops to only operate on legacy planes

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

Commit Message

Matt Roper March 28, 2014, 12:44 a.m. UTC
Ensure that existing driver loops over all planes do not change behavior
when we begin adding new types of planes (primary and cursor) to the DRM
plane list in future patches.

Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Laurent Pinchart March 28, 2014, 3:50 p.m. UTC | #1
Hi Matt,

Thank you for the patch.

On Thursday 27 March 2014 17:44:29 Matt Roper wrote:
> Ensure that existing driver loops over all planes do not change behavior
> when we begin adding new types of planes (primary and cursor) to the DRM
> plane list in future patches.
> 
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I have a question though. The patch set introduces three plane types, OVERLAY, 
PRIMARY and CURSOR. What should a driver that has no concept of primary plane 
do ? Expose all planes as OVERLAY planes only ?

> ---
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c index 0428076..ea543b4 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> @@ -247,7 +247,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc
> *scrtc) lcdc_write(sdev, LDDDSR, value);
> 
>  	/* Setup planes. */
> -	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> +	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>  		if (plane->crtc == crtc)
>  			shmob_drm_plane_setup(plane);
>  	}
Daniel Vetter March 28, 2014, 5:52 p.m. UTC | #2
On Fri, Mar 28, 2014 at 04:50:13PM +0100, Laurent Pinchart wrote:
> Hi Matt,
> 
> Thank you for the patch.
> 
> On Thursday 27 March 2014 17:44:29 Matt Roper wrote:
> > Ensure that existing driver loops over all planes do not change behavior
> > when we begin adding new types of planes (primary and cursor) to the DRM
> > plane list in future patches.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I have a question though. The patch set introduces three plane types, OVERLAY, 
> PRIMARY and CURSOR. What should a driver that has no concept of primary plane 
> do ? Expose all planes as OVERLAY planes only ?

It's a matter of backwards compat with old userspace. primary/cursor are
simply ways to tell the drm core which planes to use to forward the legacy
cursor crtc and which plane will be used for the framebuffer in setCrtc.

So until we have the new atomic interface ready your driver kinda needs to
expose at least a primary plane, otherwise there's no way to even switch
on the crtc.

But besides this backwards compat issue there's no difference and you can
specify whatever plane you want as primary/cursor (or none if you don't
care about old userspace).
-Daniel

> 
> > ---
> >  drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c index 0428076..ea543b4 100644
> > --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > @@ -247,7 +247,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc
> > *scrtc) lcdc_write(sdev, LDDDSR, value);
> > 
> >  	/* Setup planes. */
> > -	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > +	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> >  		if (plane->crtc == crtc)
> >  			shmob_drm_plane_setup(plane);
> >  	}
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter March 28, 2014, 5:53 p.m. UTC | #3
On Fri, Mar 28, 2014 at 06:52:50PM +0100, Daniel Vetter wrote:
> On Fri, Mar 28, 2014 at 04:50:13PM +0100, Laurent Pinchart wrote:
> > Hi Matt,
> > 
> > Thank you for the patch.
> > 
> > On Thursday 27 March 2014 17:44:29 Matt Roper wrote:
> > > Ensure that existing driver loops over all planes do not change behavior
> > > when we begin adding new types of planes (primary and cursor) to the DRM
> > > plane list in future patches.
> > > 
> > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I have a question though. The patch set introduces three plane types, OVERLAY, 
> > PRIMARY and CURSOR. What should a driver that has no concept of primary plane 
> > do ? Expose all planes as OVERLAY planes only ?
> 
> It's a matter of backwards compat with old userspace. primary/cursor are
> simply ways to tell the drm core which planes to use to forward the legacy
> cursor crtc and which plane will be used for the framebuffer in setCrtc.
> 
> So until we have the new atomic interface ready your driver kinda needs to
> expose at least a primary plane, otherwise there's no way to even switch
> on the crtc.
> 
> But besides this backwards compat issue there's no difference and you can
> specify whatever plane you want as primary/cursor (or none if you don't
> care about old userspace).

Well the NULL primary plane probably needs a bit of work on top of Matt's
patch series here ...
-Daniel
Laurent Pinchart April 1, 2014, 3:27 p.m. UTC | #4
Hi Daniel,

On Friday 28 March 2014 18:53:40 Daniel Vetter wrote:
> On Fri, Mar 28, 2014 at 06:52:50PM +0100, Daniel Vetter wrote:
> > On Fri, Mar 28, 2014 at 04:50:13PM +0100, Laurent Pinchart wrote:
> > > On Thursday 27 March 2014 17:44:29 Matt Roper wrote:
> > > > Ensure that existing driver loops over all planes do not change
> > > > behavior when we begin adding new types of planes (primary and cursor)
> > > > to the DRM plane list in future patches.
> > > > 
> > > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > 
> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > I have a question though. The patch set introduces three plane types,
> > > OVERLAY, PRIMARY and CURSOR. What should a driver that has no concept
> > > of primary plane do ? Expose all planes as OVERLAY planes only ?
> > 
> > It's a matter of backwards compat with old userspace. primary/cursor are
> > simply ways to tell the drm core which planes to use to forward the legacy
> > cursor crtc and which plane will be used for the framebuffer in setCrtc.
> > 
> > So until we have the new atomic interface ready your driver kinda needs to
> > expose at least a primary plane, otherwise there's no way to even switch
> > on the crtc.
> > 
> > But besides this backwards compat issue there's no difference and you can
> > specify whatever plane you want as primary/cursor (or none if you don't
> > care about old userspace).
> 
> Well the NULL primary plane probably needs a bit of work on top of Matt's
> patch series here ...

It's the NULL primary plane I'm interested about, to allow a "root" plane not 
to span the whole display. I have no urgent need for this though, I was just 
curious to see if that feature was planned.
Daniel Vetter April 1, 2014, 9:43 p.m. UTC | #5
On Tue, Apr 01, 2014 at 05:27:33PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Friday 28 March 2014 18:53:40 Daniel Vetter wrote:
> > On Fri, Mar 28, 2014 at 06:52:50PM +0100, Daniel Vetter wrote:
> > > On Fri, Mar 28, 2014 at 04:50:13PM +0100, Laurent Pinchart wrote:
> > > > On Thursday 27 March 2014 17:44:29 Matt Roper wrote:
> > > > > Ensure that existing driver loops over all planes do not change
> > > > > behavior when we begin adding new types of planes (primary and cursor)
> > > > > to the DRM plane list in future patches.
> > > > > 
> > > > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > 
> > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > I have a question though. The patch set introduces three plane types,
> > > > OVERLAY, PRIMARY and CURSOR. What should a driver that has no concept
> > > > of primary plane do ? Expose all planes as OVERLAY planes only ?
> > > 
> > > It's a matter of backwards compat with old userspace. primary/cursor are
> > > simply ways to tell the drm core which planes to use to forward the legacy
> > > cursor crtc and which plane will be used for the framebuffer in setCrtc.
> > > 
> > > So until we have the new atomic interface ready your driver kinda needs to
> > > expose at least a primary plane, otherwise there's no way to even switch
> > > on the crtc.
> > > 
> > > But besides this backwards compat issue there's no difference and you can
> > > specify whatever plane you want as primary/cursor (or none if you don't
> > > care about old userspace).
> > 
> > Well the NULL primary plane probably needs a bit of work on top of Matt's
> > patch series here ...
> 
> It's the NULL primary plane I'm interested about, to allow a "root" plane not 
> to span the whole display. I have no urgent need for this though, I was just 
> curious to see if that feature was planned.

That should work and is kinda one of the features we want ot enable with
this. At least on i915 with a bit of frobbing we should be able to allow a
root/primary fb not spanning the entire crtc (with black as background
color). Or entirely disable the primary plane and just display a (again
boxed) yuv plane for fullscreen video with aspect ratio mismatching.

When I talk that NULL primary plane needs work I mean a driver which
doesn't register _any_ primary plane at all and only supports
free-floating over planes. So wouldn't work with fbcon or any current
generic userspace at all, but would allow you to reassign all planes to a
single crtc (if your hw can do it) without pulling tricks with fake
primary planes.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
index 0428076..ea543b4 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
@@ -247,7 +247,7 @@  static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
 	lcdc_write(sdev, LDDDSR, value);
 
 	/* Setup planes. */
-	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
 		if (plane->crtc == crtc)
 			shmob_drm_plane_setup(plane);
 	}