Re: [PATCH] drm: Add rotation_property to mode_config and creating it

["Jindal, Sonika" <sonika.jindal@intel.com>]

On 8/4/2014 5:24 PM, Ville Syrjälä wrote:
> On Mon, Aug 04, 2014 at 03:59:23PM +0530, Jindal, Sonika wrote:
>>
>>
>> On 7/31/2014 9:39 AM, Jindal, Sonika wrote:
>>>
>>>
>>> On 7/29/2014 4:00 PM, Daniel Vetter wrote:
>>>> On Tue, Jul 29, 2014 at 12:40:29PM +0300, Ville Syrjälä wrote:
>>>>> On Mon, Jul 28, 2014 at 08:47:22PM +0200, Daniel Vetter wrote:
>>>>>> On Mon, Jul 28, 2014 at 06:29:41PM +0300, Ville Syrjälä wrote:
>>>>>>> On Tue, Jul 15, 2014 at 05:43:37PM +0530, sonika.jindal@intel.com wrote:
>>>>>>>> From: Sonika Jindal <sonika.jindal@intel.com>
>>>>>>>>
>>>>>>>> v2: Adding creation of rotation_property here.
>>>>>>>>
>>>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/drm_crtc.c |    3 ++-
>>>>>>>>     include/drm/drm_crtc.h     |    1 +
>>>>>>>>     2 files changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>>>>>> index 787631e..49c0747 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>>>>> @@ -1299,7 +1299,8 @@ static int drm_mode_create_standard_plane_properties(struct drm_device *dev)
>>>>>>>>     					"type", drm_plane_type_enum_list,
>>>>>>>>     					ARRAY_SIZE(drm_plane_type_enum_list));
>>>>>>>>     	dev->mode_config.plane_type_property = type;
>>>>>>>> -
>>>>>>>> +	dev->mode_config.rotation_property = drm_mode_create_rotation_property(dev,
>>>>>>>> +			BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180));
>>>>>>>
>>>>>>> This might not make sense for other (!i915) hardware. And that's the
>>>>>>> reason why I had the driver create the property in the first place.
>>>>>>>
>>>>>>> I think Daniel was thinking that we might want to expose all the bits
>>>>>>> regardless of what the hardware supports, but I don't like that idea.
>>>>>>> There are other properties (eg. alpha blending, csc stuff, etc.) that
>>>>>>> have the same problem of hardware supporting only a (potentially small)
>>>>>>> subset of the possible values. I'd rather we didn't make life harder
>>>>>>> for userspace when the kernel can already report that certain values
>>>>>>> will never work.
>>>>>>
>>>>>> Well I'd like the property to be in some generic place so that fbcon can
>>>>>> unroate and that with the atomic stuff we can have rotation support in the
>>>>>> core structures. Which should help with argument checking.
>>>>>>
>>>>>> But for rotation I don't think we should set it up in generic code, but in
>>>>>> i915. So the place where we keep it would be generic, the values would be
>>>>>> the sames, but the allowed set would differ depending upon platform or
>>>>>> driver.
>>>>>
>>>>> That would still fail if all the planes on the same device don't support
>>>>> the same rotation flags. Eg. on i915 we would have this problem if we
>>>>> exposed the old video overlay as a drm plane. And it wouldn't be the
>>>>> first piece of hardware where I've seen this kind of thing.
>>>>
>>>> Problem is still that I'd like to have a somewhat generic internal
>>>> representation available. We could punt this to atomic though by adding a
>>>> rotation field to the drm_plane_state structure. Which is more-or-less my
>>>> plan, but wouldn't work with Rob's approach.
>>>>
>>>> Or we keep the property link only in drm_plane (and give drivers the
>>>> freedom to set up the underlying enum however they want to), but I'm not
>>>> sure whether our interfaces can cope with that.
>>>> -Daniel
>>>>
>>> Daniel, Ville
>>>
>>> So what is the suggestion for this property? Should I be moving it to
>>> somewhere else?
>>>
>>> -Sonika
>> Hi Daniel/Ville,
>>
>> Please let me know if I need to move this property somewhere else.
>
> Well we at least need to move the crationg of the property to the
> driver. I guess we could leave the property itself in place in
> mode_config so that fbdev can get at it easily. If we come across
> a real need for separate per-plane rotation properties we can always
> move it to drm_plane later.
>
Ok, I will move it back to plane_create function.