diff mbox

[v2,1/6] drm/msm/mdp5: Use per-plane rotation property

Message ID ca0e99fa-9126-a4e9-baf7-6a65ad1a48e2@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Archit Taneja Oct. 24, 2016, 10:03 a.m. UTC 
Hi Ville,

On 10/22/2016 12:52 AM, 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.


I was trying to test this on msm/drm using modetest. The 180 rotation
works fine, but drm rejects reflect-x and reflect-y rotation prop
values. Is this expected?

I needed to make this modification to get reflect-x/y working too:




Otherwise, the patches look fine to me.

Thanks,
Archit

>
> v2: Drop the BIT()
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Jilai Wang <jilaiw@codeaurora.org>
> Cc: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index 951c002b05df..2653ad893ebc 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -75,15 +75,11 @@ static void mdp5_plane_install_rotation_property(struct drm_device *dev,
>  		!(mdp5_plane->caps & MDP_PIPE_CAP_VFLIP))
>  		return;
>
> -	if (!dev->mode_config.rotation_property)
> -		dev->mode_config.rotation_property =
> -			drm_mode_create_rotation_property(dev,
> -				DRM_ROTATE_0 | DRM_REFLECT_X | DRM_REFLECT_Y);
> -
> -	if (dev->mode_config.rotation_property)
> -		drm_object_attach_property(&plane->base,
> -			dev->mode_config.rotation_property,
> -			DRM_ROTATE_0);
> +	drm_plane_create_rotation_property(plane,
> +					   DRM_ROTATE_0,
> +					   DRM_ROTATE_0 |
> +					   DRM_REFLECT_X |
> +					   DRM_REFLECT_Y);
>  }
>
>  /* helper to install properties which are common to planes and crtcs */
>

Comments

Ville Syrjälä Oct. 24, 2016, 10:15 a.m. UTC | #1 
On Mon, Oct 24, 2016 at 03:33:18PM +0530, Archit Taneja wrote:
> Hi Ville,
> 
> On 10/22/2016 12:52 AM, 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.
> 
> 
> I was trying to test this on msm/drm using modetest. The 180 rotation
> works fine, but drm rejects reflect-x and reflect-y rotation prop
> values. Is this expected?
> 
> I needed to make this modification to get reflect-x/y working too:
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a747bb1..9fcc2c9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -711,7 +711,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   		state->src_h = val;
>   	} else if (property == config->rotation_property ||
>   		   property == plane->rotation_property) {
> -		if (!is_power_of_2(val & DRM_ROTATE_MASK))
> +		if (!is_power_of_2(val & (DRM_ROTATE_MASK | DRM_REFLECT_MASK)))

That makes no sense. You _must_ to pass one and only one rotation angle.
In *addition* you can pass any number of the reflection flags.

>   			return -EINVAL;
>   		state->rotation = val;
>   	} else if (property == plane->zpos_property) {
> 
> 
> 
> Otherwise, the patches look fine to me.
> 
> Thanks,
> Archit
> 
> >
> > v2: Drop the BIT()
> >
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Jilai Wang <jilaiw@codeaurora.org>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Rob Clark <robdclark@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > index 951c002b05df..2653ad893ebc 100644
> > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > @@ -75,15 +75,11 @@ static void mdp5_plane_install_rotation_property(struct drm_device *dev,
> >  		!(mdp5_plane->caps & MDP_PIPE_CAP_VFLIP))
> >  		return;
> >
> > -	if (!dev->mode_config.rotation_property)
> > -		dev->mode_config.rotation_property =
> > -			drm_mode_create_rotation_property(dev,
> > -				DRM_ROTATE_0 | DRM_REFLECT_X | DRM_REFLECT_Y);
> > -
> > -	if (dev->mode_config.rotation_property)
> > -		drm_object_attach_property(&plane->base,
> > -			dev->mode_config.rotation_property,
> > -			DRM_ROTATE_0);
> > +	drm_plane_create_rotation_property(plane,
> > +					   DRM_ROTATE_0,
> > +					   DRM_ROTATE_0 |
> > +					   DRM_REFLECT_X |
> > +					   DRM_REFLECT_Y);
> >  }
> >
> >  /* helper to install properties which are common to planes and crtcs */
> >
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Archit Taneja Oct. 24, 2016, 10:22 a.m. UTC | #2 
On 10/24/2016 03:45 PM, Ville Syrjälä wrote:
> On Mon, Oct 24, 2016 at 03:33:18PM +0530, Archit Taneja wrote:
>> Hi Ville,
>>
>> On 10/22/2016 12:52 AM, 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.
>>
>>
>> I was trying to test this on msm/drm using modetest. The 180 rotation
>> works fine, but drm rejects reflect-x and reflect-y rotation prop
>> values. Is this expected?
>>
>> I needed to make this modification to get reflect-x/y working too:
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index a747bb1..9fcc2c9 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -711,7 +711,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>   		state->src_h = val;
>>   	} else if (property == config->rotation_property ||
>>   		   property == plane->rotation_property) {
>> -		if (!is_power_of_2(val & DRM_ROTATE_MASK))
>> +		if (!is_power_of_2(val & (DRM_ROTATE_MASK | DRM_REFLECT_MASK)))
>
> That makes no sense. You _must_ to pass one and only one rotation angle.
> In *addition* you can pass any number of the reflection flags.

Okay. Does the rotation property also include reflection flags, though?

When I dump plane properties using modetest, I get:

31 rotation:
      flags: bitmask
      values: rotate-0=0x1 rotate-180=0x4 reflect-x=0x10 reflect-y=0x20
      value: 1

If I use modetest to set 0x10 or 0x20, it returns an error. I wanted to
know if this is expected behavior or not?

Thanks,
Archit

>
>>   			return -EINVAL;
>>   		state->rotation = val;
>>   	} else if (property == plane->zpos_property) {
>>
>>
>>
>> Otherwise, the patches look fine to me.
>>
>> Thanks,
>> Archit
>>
>>>
>>> v2: Drop the BIT()
>>>
>>> Cc: Rob Clark <robdclark@gmail.com>
>>> Cc: Jilai Wang <jilaiw@codeaurora.org>
>>> Cc: Archit Taneja <architt@codeaurora.org>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>> ---
>>>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 14 +++++---------
>>>  1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>>> index 951c002b05df..2653ad893ebc 100644
>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>>> @@ -75,15 +75,11 @@ static void mdp5_plane_install_rotation_property(struct drm_device *dev,
>>>  		!(mdp5_plane->caps & MDP_PIPE_CAP_VFLIP))
>>>  		return;
>>>
>>> -	if (!dev->mode_config.rotation_property)
>>> -		dev->mode_config.rotation_property =
>>> -			drm_mode_create_rotation_property(dev,
>>> -				DRM_ROTATE_0 | DRM_REFLECT_X | DRM_REFLECT_Y);
>>> -
>>> -	if (dev->mode_config.rotation_property)
>>> -		drm_object_attach_property(&plane->base,
>>> -			dev->mode_config.rotation_property,
>>> -			DRM_ROTATE_0);
>>> +	drm_plane_create_rotation_property(plane,
>>> +					   DRM_ROTATE_0,
>>> +					   DRM_ROTATE_0 |
>>> +					   DRM_REFLECT_X |
>>> +					   DRM_REFLECT_Y);
>>>  }
>>>
>>>  /* helper to install properties which are common to planes and crtcs */
>>>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>
Ville Syrjälä Oct. 24, 2016, 10:25 a.m. UTC | #3 
On Mon, Oct 24, 2016 at 03:52:09PM +0530, Archit Taneja wrote:
> 
> 
> On 10/24/2016 03:45 PM, Ville Syrjälä wrote:
> > On Mon, Oct 24, 2016 at 03:33:18PM +0530, Archit Taneja wrote:
> >> Hi Ville,
> >>
> >> On 10/22/2016 12:52 AM, 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.
> >>
> >>
> >> I was trying to test this on msm/drm using modetest. The 180 rotation
> >> works fine, but drm rejects reflect-x and reflect-y rotation prop
> >> values. Is this expected?
> >>
> >> I needed to make this modification to get reflect-x/y working too:
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index a747bb1..9fcc2c9 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -711,7 +711,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >>   		state->src_h = val;
> >>   	} else if (property == config->rotation_property ||
> >>   		   property == plane->rotation_property) {
> >> -		if (!is_power_of_2(val & DRM_ROTATE_MASK))
> >> +		if (!is_power_of_2(val & (DRM_ROTATE_MASK | DRM_REFLECT_MASK)))
> >
> > That makes no sense. You _must_ to pass one and only one rotation angle.
> > In *addition* you can pass any number of the reflection flags.
> 
> Okay. Does the rotation property also include reflection flags, though?

Yes.

> 
> When I dump plane properties using modetest, I get:
> 
> 31 rotation:
>       flags: bitmask
>       values: rotate-0=0x1 rotate-180=0x4 reflect-x=0x10 reflect-y=0x20
>       value: 1
> 
> If I use modetest to set 0x10 or 0x20, it returns an error. I wanted to
> know if this is expected behavior or not?

It is. If you want 0 degree rotation with X reflection you have to pass
0x11.

> 
> Thanks,
> Archit
> 
> >
> >>   			return -EINVAL;
> >>   		state->rotation = val;
> >>   	} else if (property == plane->zpos_property) {
> >>
> >>
> >>
> >> Otherwise, the patches look fine to me.
> >>
> >> Thanks,
> >> Archit
> >>
> >>>
> >>> v2: Drop the BIT()
> >>>
> >>> Cc: Rob Clark <robdclark@gmail.com>
> >>> Cc: Jilai Wang <jilaiw@codeaurora.org>
> >>> Cc: Archit Taneja <architt@codeaurora.org>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Reviewed-by: Rob Clark <robdclark@gmail.com>
> >>> ---
> >>>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 14 +++++---------
> >>>  1 file changed, 5 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >>> index 951c002b05df..2653ad893ebc 100644
> >>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >>> @@ -75,15 +75,11 @@ static void mdp5_plane_install_rotation_property(struct drm_device *dev,
> >>>  		!(mdp5_plane->caps & MDP_PIPE_CAP_VFLIP))
> >>>  		return;
> >>>
> >>> -	if (!dev->mode_config.rotation_property)
> >>> -		dev->mode_config.rotation_property =
> >>> -			drm_mode_create_rotation_property(dev,
> >>> -				DRM_ROTATE_0 | DRM_REFLECT_X | DRM_REFLECT_Y);
> >>> -
> >>> -	if (dev->mode_config.rotation_property)
> >>> -		drm_object_attach_property(&plane->base,
> >>> -			dev->mode_config.rotation_property,
> >>> -			DRM_ROTATE_0);
> >>> +	drm_plane_create_rotation_property(plane,
> >>> +					   DRM_ROTATE_0,
> >>> +					   DRM_ROTATE_0 |
> >>> +					   DRM_REFLECT_X |
> >>> +					   DRM_REFLECT_Y);
> >>>  }
> >>>
> >>>  /* helper to install properties which are common to planes and crtcs */
> >>>
> >>
> >> --
> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> >> a Linux Foundation Collaborative Project
> >
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Archit Taneja Oct. 24, 2016, 10:31 a.m. UTC | #4 
On 10/24/2016 03:55 PM, Ville Syrjälä wrote:
> On Mon, Oct 24, 2016 at 03:52:09PM +0530, Archit Taneja wrote:
>>
>>
>> On 10/24/2016 03:45 PM, Ville Syrjälä wrote:
>>> On Mon, Oct 24, 2016 at 03:33:18PM +0530, Archit Taneja wrote:
>>>> Hi Ville,
>>>>
>>>> On 10/22/2016 12:52 AM, 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.
>>>>
>>>>
>>>> I was trying to test this on msm/drm using modetest. The 180 rotation
>>>> works fine, but drm rejects reflect-x and reflect-y rotation prop
>>>> values. Is this expected?
>>>>
>>>> I needed to make this modification to get reflect-x/y working too:
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index a747bb1..9fcc2c9 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -711,7 +711,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>   		state->src_h = val;
>>>>   	} else if (property == config->rotation_property ||
>>>>   		   property == plane->rotation_property) {
>>>> -		if (!is_power_of_2(val & DRM_ROTATE_MASK))
>>>> +		if (!is_power_of_2(val & (DRM_ROTATE_MASK | DRM_REFLECT_MASK)))
>>>
>>> That makes no sense. You _must_ to pass one and only one rotation angle.
>>> In *addition* you can pass any number of the reflection flags.
>>
>> Okay. Does the rotation property also include reflection flags, though?
>
> Yes.
>
>>
>> When I dump plane properties using modetest, I get:
>>
>> 31 rotation:
>>       flags: bitmask
>>       values: rotate-0=0x1 rotate-180=0x4 reflect-x=0x10 reflect-y=0x20
>>       value: 1
>>
>> If I use modetest to set 0x10 or 0x20, it returns an error. I wanted to
>> know if this is expected behavior or not?
>
> It is. If you want 0 degree rotation with X reflection you have to pass
> 0x11.

Ah okay, didn't realize I had to put a 0 degree rotation along with the
reflect. Works fine with 0x11. Thanks for the clarification.

Archit

>
>>
>> Thanks,
>> Archit
>>
>>>
>>>>   			return -EINVAL;
>>>>   		state->rotation = val;
>>>>   	} else if (property == plane->zpos_property) {
>>>>
>>>>
>>>>
>>>> Otherwise, the patches look fine to me.
>>>>
>>>> Thanks,
>>>> Archit
>>>>
>>>>>
>>>>> v2: Drop the BIT()
>>>>>
>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>> Cc: Jilai Wang <jilaiw@codeaurora.org>
>>>>> Cc: Archit Taneja <architt@codeaurora.org>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>>> ---
>>>>>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 14 +++++---------
>>>>>  1 file changed, 5 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>>>>> index 951c002b05df..2653ad893ebc 100644
>>>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>>>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
>>>>> @@ -75,15 +75,11 @@ static void mdp5_plane_install_rotation_property(struct drm_device *dev,
>>>>>  		!(mdp5_plane->caps & MDP_PIPE_CAP_VFLIP))
>>>>>  		return;
>>>>>
>>>>> -	if (!dev->mode_config.rotation_property)
>>>>> -		dev->mode_config.rotation_property =
>>>>> -			drm_mode_create_rotation_property(dev,
>>>>> -				DRM_ROTATE_0 | DRM_REFLECT_X | DRM_REFLECT_Y);
>>>>> -
>>>>> -	if (dev->mode_config.rotation_property)
>>>>> -		drm_object_attach_property(&plane->base,
>>>>> -			dev->mode_config.rotation_property,
>>>>> -			DRM_ROTATE_0);
>>>>> +	drm_plane_create_rotation_property(plane,
>>>>> +					   DRM_ROTATE_0,
>>>>> +					   DRM_ROTATE_0 |
>>>>> +					   DRM_REFLECT_X |
>>>>> +					   DRM_REFLECT_Y);
>>>>>  }
>>>>>
>>>>>  /* helper to install properties which are common to planes and crtcs */
>>>>>
>>>>
>>>> --
>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>>>> a Linux Foundation Collaborative Project
>>>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a747bb1..9fcc2c9 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -711,7 +711,7 @@  int drm_atomic_plane_set_property(struct drm_plane *plane,
  		state->src_h = val;
  	} else if (property == config->rotation_property ||
  		   property == plane->rotation_property) {
-		if (!is_power_of_2(val & DRM_ROTATE_MASK))
+		if (!is_power_of_2(val & (DRM_ROTATE_MASK | DRM_REFLECT_MASK)))
  			return -EINVAL;
  		state->rotation = val;
  	} else if (property == plane->zpos_property) {