| Message ID | 1425547288-16131-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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...
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
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
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 --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;
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)