Re: [PATCH 07/15] drm/omap: Use per-plane rotation property

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Tomi Valkeinen <tomi.valkeinen@gmail.com>
Cc: intel-gfx@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 07/15] drm/omap: Use per-plane rotation property
Date: Fri, 12 Aug 2016 19:04:04 +0300	[thread overview]
Message-ID: <20160812160404.GD4329@intel.com> (raw)
In-Reply-To: <20160811133332.GL4329@intel.com>

On Thu, Aug 11, 2016 at 04:33:32PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 11, 2016 at 02:32:44PM +0300, Tomi Valkeinen wrote:
> > Hi,
> > 
> > On 22/07/16 16:43, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The global mode_config.rotation_property is going away, switch over to
> > > per-plane rotation_property.
> > > 
> > > Not sure I got the annoying crtc rotation_property handling right.
> > > Might work, or migth not.
> > 
> > I think something is funny with this patch or the series. I fetched your
> > branch, and with your series, it looks like the primary planes lose all
> > their props. modetest says:
> > 
> > could not get plane 26 properties: Invalid argument
> > could not get plane 30 properties: Invalid argument
> 
> Hmm. Weird. Is it really the get props ioctl that fails?
> 
> The first EINVAL I can spot there is
>         if (!obj->properties) {
>               ret = -EINVAL;
>               goto out_unref;
> 	}
> which definitely makes no sense since this is assigned
> as plane->base.properties = &plane->properties. So can't be that unless
> we manage to clear the pointer somehow after the init.
> 
> The only other direct EINVAL I see there is if
>  drm_object_property_get_value(obj->properties->properties[i])
> fails to find the passed prop in the properties array. Which clearly
> can't happen since we got it from the array in the first place. Also,
> clearly that code is rather inefficient, perhaps someone should rewrite
> it a bit.
> 
> Can't quite see how this could fail for the plane in other ways. But I
> might be blind.

I tried to think on this a bit more, and the only think I came up with was
that we end up doing the drm_plane_create_rotation_property() twice for the
primary planes. I tried that on i915 but it'd didn't result in anything bad
AFAICS. Would leak a bit, but so what :P

Dunno, I guess you could try something like:

--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -211,11 +211,12 @@ void omap_plane_install_properties(struct drm_plane *plane,
        struct omap_drm_private *priv = dev->dev_private;
 
        if (priv->has_dmm) {
-               drm_plane_create_rotation_property(plane,
-                                                  BIT(DRM_ROTATE_0),
-                                                  BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_90) |
-                                                  BIT(DRM_ROTATE_180) | BIT(DRM_ROTATE_270) |
-                                                  BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y));
+               if (!plane->rotation_property)
+                       drm_plane_create_rotation_property(plane,
+                                                          BIT(DRM_ROTATE_0),
+                                                          BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_90) |
+                                                          BIT(DRM_ROTATE_180) | BIT(DRM_ROTATE_270) |
+                                                          BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y));


> 
> > 
> > and
> > 
> > Planes:
> > id      crtc    fb      CRTC x,y        x,y     gamma size      possible
> > crtcs
> > 26      28      55      0,0             0,0     0               0x00000001
> >   formats: RG16 RX12 XR12 RA12 AR12 XR15 AR15 RG24 RX24 XR24 RA24 AR24
> >   no properties found
> > 30      0       0       0,0             0,0     0               0x00000002
> >   formats: RG16 RX12 XR12 RA12 AR12 XR15 AR15 RG24 RX24 XR24 RA24 AR24
> > NV12 YUYV UYVY
> >   no properties found
> > 
> > I didn't look closer yet.
> > 
> >  Tomi
> > 
> 
> 
> 
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
