diff mbox

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

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

Commit Message

sonika.jindal@intel.com March 5, 2015, 9:21 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

Daniel Vetter March 5, 2015, 12:54 p.m. UTC | #1
On Thu, Mar 05, 2015 at 02:51:26PM +0530, Sonika Jindal wrote:
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Imo this needs a little more commit message, and more important it needs
igt test coverage. Best approach there is probably to take the plane test
we have already and extend it to the primary plane.
-Daniel

> ---
>  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 437a679..e1b0c4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12183,16 +12183,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 March 9, 2015, 3:03 a.m. UTC | #2
On Thursday 05 March 2015 06:24 PM, Daniel Vetter wrote:
> On Thu, Mar 05, 2015 at 02:51:26PM +0530, Sonika Jindal wrote:
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> Imo this needs a little more commit message, and more important it needs
> igt test coverage. Best approach there is probably to take the plane test
> we have already and extend it to the primary plane.
> -Daniel
This is just to take care of the case when the size of the fb is smaller 
than the crtc.
I have extended the rotation test (yet to be posted), to create a 
smaller primary plane fb to be used for 90/270 rotation.

Since we still set position to 0 for primary plane, I did not add any 
test case for positioning of primary plane.
That can be added as a separate activity when positioning support is added.
Right now this is just to allow smaller fb for primary plane which is 
possible with universal planes gen >=9.

Regards,
Sonika
>> ---
>>   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 437a679..e1b0c4d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12183,16 +12183,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 March 9, 2015, 8:46 a.m. UTC | #3
On Mon, Mar 09, 2015 at 08:33:27AM +0530, sonika wrote:
> 
> On Thursday 05 March 2015 06:24 PM, Daniel Vetter wrote:
> >On Thu, Mar 05, 2015 at 02:51:26PM +0530, Sonika Jindal wrote:
> >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >Imo this needs a little more commit message, and more important it needs
> >igt test coverage. Best approach there is probably to take the plane test
> >we have already and extend it to the primary plane.
> >-Daniel
> This is just to take care of the case when the size of the fb is smaller
> than the crtc.
> I have extended the rotation test (yet to be posted), to create a smaller
> primary plane fb to be used for 90/270 rotation.
> 
> Since we still set position to 0 for primary plane, I did not add any test
> case for positioning of primary plane.
> That can be added as a separate activity when positioning support is added.
> Right now this is just to allow smaller fb for primary plane which is
> possible with universal planes gen >=9.

Through universal planes it's already possible to position any plane
anywhere, and this code is all that makes sure this doesn't happen for the
primary plane. Since you've just changed that I think this needs a
testcase in igt.

Or maybe I missed something and it's indeed not yet possible to do this?
-Daniel
Ville Syrjälä March 9, 2015, 3:20 p.m. UTC | #4
On Mon, Mar 09, 2015 at 09:46:27AM +0100, Daniel Vetter wrote:
> On Mon, Mar 09, 2015 at 08:33:27AM +0530, sonika wrote:
> > 
> > On Thursday 05 March 2015 06:24 PM, Daniel Vetter wrote:
> > >On Thu, Mar 05, 2015 at 02:51:26PM +0530, Sonika Jindal wrote:
> > >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > >Imo this needs a little more commit message, and more important it needs
> > >igt test coverage. Best approach there is probably to take the plane test
> > >we have already and extend it to the primary plane.
> > >-Daniel
> > This is just to take care of the case when the size of the fb is smaller
> > than the crtc.
> > I have extended the rotation test (yet to be posted), to create a smaller
> > primary plane fb to be used for 90/270 rotation.
> > 
> > Since we still set position to 0 for primary plane, I did not add any test
> > case for positioning of primary plane.
> > That can be added as a separate activity when positioning support is added.
> > Right now this is just to allow smaller fb for primary plane which is
> > possible with universal planes gen >=9.
> 
> Through universal planes it's already possible to position any plane
> anywhere, and this code is all that makes sure this doesn't happen for the
> primary plane. Since you've just changed that I think this needs a
> testcase in igt.
> 
> Or maybe I missed something and it's indeed not yet possible to do this?

We're not ready to enable this just yet. First we need to change all the
plane code to use the correct derived state (eg. clipped coordinates as
opposed to the user requested ones). And to do that we must make sure
the derived state is up to date at .crtc_enable() time.

This was one of the things I had on mind when Gustavo was reworking the
primary/cursor plane code, but then Gustavo was pulled away and this
task fell through the cracks. I've now cooked up something that should
make this happen, but I haven't tested it yet. I'll give it a spin today
and post the patch if all goes well...
Daniel Vetter March 9, 2015, 3:31 p.m. UTC | #5
On Mon, Mar 09, 2015 at 05:20:39PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 09, 2015 at 09:46:27AM +0100, Daniel Vetter wrote:
> > On Mon, Mar 09, 2015 at 08:33:27AM +0530, sonika wrote:
> > > 
> > > On Thursday 05 March 2015 06:24 PM, Daniel Vetter wrote:
> > > >On Thu, Mar 05, 2015 at 02:51:26PM +0530, Sonika Jindal wrote:
> > > >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > >Imo this needs a little more commit message, and more important it needs
> > > >igt test coverage. Best approach there is probably to take the plane test
> > > >we have already and extend it to the primary plane.
> > > >-Daniel
> > > This is just to take care of the case when the size of the fb is smaller
> > > than the crtc.
> > > I have extended the rotation test (yet to be posted), to create a smaller
> > > primary plane fb to be used for 90/270 rotation.
> > > 
> > > Since we still set position to 0 for primary plane, I did not add any test
> > > case for positioning of primary plane.
> > > That can be added as a separate activity when positioning support is added.
> > > Right now this is just to allow smaller fb for primary plane which is
> > > possible with universal planes gen >=9.
> > 
> > Through universal planes it's already possible to position any plane
> > anywhere, and this code is all that makes sure this doesn't happen for the
> > primary plane. Since you've just changed that I think this needs a
> > testcase in igt.
> > 
> > Or maybe I missed something and it's indeed not yet possible to do this?
> 
> We're not ready to enable this just yet. First we need to change all the
> plane code to use the correct derived state (eg. clipped coordinates as
> opposed to the user requested ones). And to do that we must make sure
> the derived state is up to date at .crtc_enable() time.
> 
> This was one of the things I had on mind when Gustavo was reworking the
> primary/cursor plane code, but then Gustavo was pulled away and this
> task fell through the cracks. I've now cooked up something that should
> make this happen, but I haven't tested it yet. I'll give it a spin today
> and post the patch if all goes well...

Hm, is there a lot of derived state around (restricting just to skl+) once
we have the wm code converted over to always look at plane_state structs
instead of the add-hoc wm struct we currently use?
-Daniel
sonika.jindal@intel.com March 10, 2015, 3:53 a.m. UTC | #6
On Monday 09 March 2015 02:16 PM, Daniel Vetter wrote:
> On Mon, Mar 09, 2015 at 08:33:27AM +0530, sonika wrote:
>> On Thursday 05 March 2015 06:24 PM, Daniel Vetter wrote:
>>> On Thu, Mar 05, 2015 at 02:51:26PM +0530, Sonika Jindal wrote:
>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>> Imo this needs a little more commit message, and more important it needs
>>> igt test coverage. Best approach there is probably to take the plane test
>>> we have already and extend it to the primary plane.
>>> -Daniel
>> This is just to take care of the case when the size of the fb is smaller
>> than the crtc.
>> I have extended the rotation test (yet to be posted), to create a smaller
>> primary plane fb to be used for 90/270 rotation.
>>
>> Since we still set position to 0 for primary plane, I did not add any test
>> case for positioning of primary plane.
>> That can be added as a separate activity when positioning support is added.
>> Right now this is just to allow smaller fb for primary plane which is
>> possible with universal planes gen >=9.
> Through universal planes it's already possible to position any plane
> anywhere, and this code is all that makes sure this doesn't happen for the
> primary plane. Since you've just changed that I think this needs a
> testcase in igt.
>
> Or maybe I missed something and it's indeed not yet possible to do this?
> -Daniel
Yes, it isn't possible yet. We set the position to 0 in 
skylake_update_primary_plane.
-Sonika
Daniel Vetter March 10, 2015, 10:10 a.m. UTC | #7
On Tue, Mar 10, 2015 at 09:23:54AM +0530, sonika wrote:
> 
> On Monday 09 March 2015 02:16 PM, Daniel Vetter wrote:
> >On Mon, Mar 09, 2015 at 08:33:27AM +0530, sonika wrote:
> >>On Thursday 05 March 2015 06:24 PM, Daniel Vetter wrote:
> >>>On Thu, Mar 05, 2015 at 02:51:26PM +0530, Sonika Jindal wrote:
> >>>>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>>Imo this needs a little more commit message, and more important it needs
> >>>igt test coverage. Best approach there is probably to take the plane test
> >>>we have already and extend it to the primary plane.
> >>>-Daniel
> >>This is just to take care of the case when the size of the fb is smaller
> >>than the crtc.
> >>I have extended the rotation test (yet to be posted), to create a smaller
> >>primary plane fb to be used for 90/270 rotation.
> >>
> >>Since we still set position to 0 for primary plane, I did not add any test
> >>case for positioning of primary plane.
> >>That can be added as a separate activity when positioning support is added.
> >>Right now this is just to allow smaller fb for primary plane which is
> >>possible with universal planes gen >=9.
> >Through universal planes it's already possible to position any plane
> >anywhere, and this code is all that makes sure this doesn't happen for the
> >primary plane. Since you've just changed that I think this needs a
> >testcase in igt.
> >
> >Or maybe I missed something and it's indeed not yet possible to do this?
> >-Daniel
> Yes, it isn't possible yet. We set the position to 0 in
> skylake_update_primary_plane.

Hm indeed. But with your patch we no longer reject plane updates for the
primary plane which aren't positioned at 0,0. So there's now a bug. I
guess either go the full way towards implementing universal plane support
on skl (and fix up the hw programming) or we need to wait with this patch
a bit more.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 437a679..e1b0c4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12183,16 +12183,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;