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

[Tomi Valkeinen]

On 12/08/16 19:04, Ville Syrjälä wrote:
> 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 at linux.intel.com wrote:
>>>> From: Ville Syrjälä <ville.syrjala at 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));

This fixes the problem... Obviously something gets confused if the
property is created twice. Perhaps the first one gets stored somewhere,
and gets used somehow, even if the latter property is the "real" one,
attached to the plane? Just a guess...

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160923/ed1ad629/attachment.sig>