diff mbox

[1/2] drm/i915/skl: Allow universal planes to position

Message ID 1427704497-28344-1-git-send-email-sonika.jindal@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sonika.jindal@intel.com March 30, 2015, 8:34 a.m. UTC
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Matt Roper April 1, 2015, 6:21 p.m. UTC | #1
On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote:
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

It looks like this is dependent on Ville's patch

  [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane()

to actually let us do something sensible with the destination rectangle
at the hardware level.  Looks like that patch has a r-b, but hasn't made
it into di-nightly yet. 

Assuming Ville's patch lands first, this is
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ceb2e61..f0bbc22 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	bool can_position = false;
>  	int ret;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	if (INTEL_INFO(dev)->gen >= 9)
> +		can_position = true;
> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> -					    false, true, &state->visible);
> +					    can_position, true,
> +					    &state->visible);
>  	if (ret)
>  		return ret;
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
sonika.jindal@intel.com April 2, 2015, 4:38 a.m. UTC | #2
On 4/1/2015 11:51 PM, Matt Roper wrote:
> On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote:
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>
> It looks like this is dependent on Ville's patch
>
>    [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane()
>
> to actually let us do something sensible with the destination rectangle
> at the hardware level.  Looks like that patch has a r-b, but hasn't made
> it into di-nightly yet.
>
Right now, can_position is used to check for the scenarios where the 
primary plane is not covering the complete crtc. This could be due to 
positioning or a smaller fb on primary plane.
With Ville's patch, we would be able to allow positioning to happen.
But I need it here, to create a smaller fb for 90/270 rotation.

I agree that, until Ville's patch is there, we won't be entertaining any 
positioning requests on the primary plane and we will not be throwing 
any error also.

But for the 90/270 testcase in kms_rotation_crc to go through, we would 
need this to create a smaller fb so that we can rotate it.

> Assuming Ville's patch lands first, this is
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
>
> Matt
>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ceb2e61..f0bbc22 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
>>   	struct drm_rect *dest = &state->dst;
>>   	struct drm_rect *src = &state->src;
>>   	const struct drm_rect *clip = &state->clip;
>> +	bool can_position = false;
>>   	int ret;
>>
>>   	crtc = crtc ? crtc : plane->crtc;
>>   	intel_crtc = to_intel_crtc(crtc);
>>
>> +	if (INTEL_INFO(dev)->gen >= 9)
>> +		can_position = true;
>> +
>>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>>   					    src, dest, clip,
>>   					    DRM_PLANE_HELPER_NO_SCALING,
>>   					    DRM_PLANE_HELPER_NO_SCALING,
>> -					    false, true, &state->visible);
>> +					    can_position, true,
>> +					    &state->visible);
>>   	if (ret)
>>   		return ret;
>>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Matt Roper April 2, 2015, 3:48 p.m. UTC | #3
On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/1/2015 11:51 PM, Matt Roper wrote:
> >On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote:
> >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >
> >It looks like this is dependent on Ville's patch
> >
> >   [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane()
> >
> >to actually let us do something sensible with the destination rectangle
> >at the hardware level.  Looks like that patch has a r-b, but hasn't made
> >it into di-nightly yet.
> >
> Right now, can_position is used to check for the scenarios where the
> primary plane is not covering the complete crtc. This could be due
> to positioning or a smaller fb on primary plane.
> With Ville's patch, we would be able to allow positioning to happen.
> But I need it here, to create a smaller fb for 90/270 rotation.
> 
> I agree that, until Ville's patch is there, we won't be entertaining
> any positioning requests on the primary plane and we will not be
> throwing any error also.

Right...and I think failing to throw an error would be seen as a bug,
which is why I think Ville's patch needs to go in first.  Since it's
already been reviewed, I'm not aware of anything holding that up from
happening.

> But for the 90/270 testcase in kms_rotation_crc to go through, we
> would need this to create a smaller fb so that we can rotate it.

So is your worry here that drm_plane_helper_check_update() doesn't
understand rotation and winds up mixing up width/height?  If so, I think
the proper course of action is to write a patch for the helper function
that makes it rotation-aware.


Matt

> 
> >Assuming Ville's patch lands first, this is
> >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >
> >
> >Matt
> >
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index ceb2e61..f0bbc22 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
> >>  	struct drm_rect *dest = &state->dst;
> >>  	struct drm_rect *src = &state->src;
> >>  	const struct drm_rect *clip = &state->clip;
> >>+	bool can_position = false;
> >>  	int ret;
> >>
> >>  	crtc = crtc ? crtc : plane->crtc;
> >>  	intel_crtc = to_intel_crtc(crtc);
> >>
> >>+	if (INTEL_INFO(dev)->gen >= 9)
> >>+		can_position = true;
> >>+
> >>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> >>  					    src, dest, clip,
> >>  					    DRM_PLANE_HELPER_NO_SCALING,
> >>  					    DRM_PLANE_HELPER_NO_SCALING,
> >>-					    false, true, &state->visible);
> >>+					    can_position, true,
> >>+					    &state->visible);
> >>  	if (ret)
> >>  		return ret;
> >>
> >>--
> >>1.7.10.4
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
sonika.jindal@intel.com April 6, 2015, 5:20 a.m. UTC | #4
On 4/2/2015 9:18 PM, Matt Roper wrote:
> On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote:
>>
>>
>> On 4/1/2015 11:51 PM, Matt Roper wrote:
>>> On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote:
>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>
>>> It looks like this is dependent on Ville's patch
>>>
>>>    [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane()
>>>
>>> to actually let us do something sensible with the destination rectangle
>>> at the hardware level.  Looks like that patch has a r-b, but hasn't made
>>> it into di-nightly yet.
>>>
>> Right now, can_position is used to check for the scenarios where the
>> primary plane is not covering the complete crtc. This could be due
>> to positioning or a smaller fb on primary plane.
>> With Ville's patch, we would be able to allow positioning to happen.
>> But I need it here, to create a smaller fb for 90/270 rotation.
>>
>> I agree that, until Ville's patch is there, we won't be entertaining
>> any positioning requests on the primary plane and we will not be
>> throwing any error also.
>
> Right...and I think failing to throw an error would be seen as a bug,
> which is why I think Ville's patch needs to go in first.  Since it's
> already been reviewed, I'm not aware of anything holding that up from
> happening.
>
Agree, will check with Daniel on this.

>> But for the 90/270 testcase in kms_rotation_crc to go through, we
>> would need this to create a smaller fb so that we can rotate it.
>
> So is your worry here that drm_plane_helper_check_update() doesn't
> understand rotation and winds up mixing up width/height?  If so, I think
> the proper course of action is to write a patch for the helper function
> that makes it rotation-aware.
>
No, the worry is that it rejects a smaller fb for primary plane for all 
the platforms. I mentioned 90/270 rotation, because I create a smaller 
fb (rather than the full screen fb), so that the rotated plane fits into 
the screen. If it is lets say 1920x1080, and the pipe is set at 
1920x1080, after rotation the plane becomes 1080x1920 and the height of 
the plane surpasses that of crtc.
For gen >=9 , we can have smaller fb for primary plane which might not 
cover the entire crtc.

Regards,
Sonika
>
> Matt
>
>>
>>> Assuming Ville's patch lands first, this is
>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>
>>>
>>> Matt
>>>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index ceb2e61..f0bbc22 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
>>>>   	struct drm_rect *dest = &state->dst;
>>>>   	struct drm_rect *src = &state->src;
>>>>   	const struct drm_rect *clip = &state->clip;
>>>> +	bool can_position = false;
>>>>   	int ret;
>>>>
>>>>   	crtc = crtc ? crtc : plane->crtc;
>>>>   	intel_crtc = to_intel_crtc(crtc);
>>>>
>>>> +	if (INTEL_INFO(dev)->gen >= 9)
>>>> +		can_position = true;
>>>> +
>>>>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>>>>   					    src, dest, clip,
>>>>   					    DRM_PLANE_HELPER_NO_SCALING,
>>>>   					    DRM_PLANE_HELPER_NO_SCALING,
>>>> -					    false, true, &state->visible);
>>>> +					    can_position, true,
>>>> +					    &state->visible);
>>>>   	if (ret)
>>>>   		return ret;
>>>>
>>>> --
>>>> 1.7.10.4
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>
Daniel Vetter April 7, 2015, 8:16 a.m. UTC | #5
On Mon, Apr 06, 2015 at 10:50:51AM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/2/2015 9:18 PM, Matt Roper wrote:
> >On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote:
> >>
> >>
> >>On 4/1/2015 11:51 PM, Matt Roper wrote:
> >>>On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote:
> >>>>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>>
> >>>It looks like this is dependent on Ville's patch
> >>>
> >>>   [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane()
> >>>
> >>>to actually let us do something sensible with the destination rectangle
> >>>at the hardware level.  Looks like that patch has a r-b, but hasn't made
> >>>it into di-nightly yet.
> >>>
> >>Right now, can_position is used to check for the scenarios where the
> >>primary plane is not covering the complete crtc. This could be due
> >>to positioning or a smaller fb on primary plane.
> >>With Ville's patch, we would be able to allow positioning to happen.
> >>But I need it here, to create a smaller fb for 90/270 rotation.
> >>
> >>I agree that, until Ville's patch is there, we won't be entertaining
> >>any positioning requests on the primary plane and we will not be
> >>throwing any error also.
> >
> >Right...and I think failing to throw an error would be seen as a bug,
> >which is why I think Ville's patch needs to go in first.  Since it's
> >already been reviewed, I'm not aware of anything holding that up from
> >happening.
> >
> Agree, will check with Daniel on this.
> 
> >>But for the 90/270 testcase in kms_rotation_crc to go through, we
> >>would need this to create a smaller fb so that we can rotate it.
> >
> >So is your worry here that drm_plane_helper_check_update() doesn't
> >understand rotation and winds up mixing up width/height?  If so, I think
> >the proper course of action is to write a patch for the helper function
> >that makes it rotation-aware.
> >
> No, the worry is that it rejects a smaller fb for primary plane for all the
> platforms. I mentioned 90/270 rotation, because I create a smaller fb
> (rather than the full screen fb), so that the rotated plane fits into the
> screen. If it is lets say 1920x1080, and the pipe is set at 1920x1080, after
> rotation the plane becomes 1080x1920 and the height of the plane surpasses
> that of crtc.
> For gen >=9 , we can have smaller fb for primary plane which might not cover
> the entire crtc.

That sounds like a bug in the helper though if it rejects such a
framebuffer.
-Daniel
sonika.jindal@intel.com April 7, 2015, 8:21 a.m. UTC | #6
On 4/7/2015 1:46 PM, Daniel Vetter wrote:
> On Mon, Apr 06, 2015 at 10:50:51AM +0530, Jindal, Sonika wrote:
>>
>>
>> On 4/2/2015 9:18 PM, Matt Roper wrote:
>>> On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote:
>>>>
>>>>
>>>> On 4/1/2015 11:51 PM, Matt Roper wrote:
>>>>> On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote:
>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>>>
>>>>> It looks like this is dependent on Ville's patch
>>>>>
>>>>>    [PATCH v2 6/9] drm/i915: Pass the primary plane position to .update_primary_plane()
>>>>>
>>>>> to actually let us do something sensible with the destination rectangle
>>>>> at the hardware level.  Looks like that patch has a r-b, but hasn't made
>>>>> it into di-nightly yet.
>>>>>
>>>> Right now, can_position is used to check for the scenarios where the
>>>> primary plane is not covering the complete crtc. This could be due
>>>> to positioning or a smaller fb on primary plane.
>>>> With Ville's patch, we would be able to allow positioning to happen.
>>>> But I need it here, to create a smaller fb for 90/270 rotation.
>>>>
>>>> I agree that, until Ville's patch is there, we won't be entertaining
>>>> any positioning requests on the primary plane and we will not be
>>>> throwing any error also.
>>>
>>> Right...and I think failing to throw an error would be seen as a bug,
>>> which is why I think Ville's patch needs to go in first.  Since it's
>>> already been reviewed, I'm not aware of anything holding that up from
>>> happening.
>>>
>> Agree, will check with Daniel on this.
>>
>>>> But for the 90/270 testcase in kms_rotation_crc to go through, we
>>>> would need this to create a smaller fb so that we can rotate it.
>>>
>>> So is your worry here that drm_plane_helper_check_update() doesn't
>>> understand rotation and winds up mixing up width/height?  If so, I think
>>> the proper course of action is to write a patch for the helper function
>>> that makes it rotation-aware.
>>>
>> No, the worry is that it rejects a smaller fb for primary plane for all the
>> platforms. I mentioned 90/270 rotation, because I create a smaller fb
>> (rather than the full screen fb), so that the rotated plane fits into the
>> screen. If it is lets say 1920x1080, and the pipe is set at 1920x1080, after
>> rotation the plane becomes 1080x1920 and the height of the plane surpasses
>> that of crtc.
>> For gen >=9 , we can have smaller fb for primary plane which might not cover
>> the entire crtc.
>
> That sounds like a bug in the helper though if it rejects such a
> framebuffer.
> -Daniel
Till now we used to have primary plane covering the crtc, so its not a 
bug. For gen > 9 we can have smaller primary planes (or with other 
platforms as well?)
Anyways, we need to add this patch to get past that restriction.

Regards,
Sonika
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ceb2e61..f0bbc22 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12150,16 +12150,21 @@  intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	bool can_position = false;
 	int ret;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	if (INTEL_INFO(dev)->gen >= 9)
+		can_position = true;
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, true, &state->visible);
+					    can_position, true,
+					    &state->visible);
 	if (ret)
 		return ret;