| Message ID | 20171101182920.14386-4-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 01, 2017 at 08:29:18PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Try to fix the code to actually clip the plane to the crtc bounds > instead of the user provided crtc coordinates (which would be a no-op > since those are exactly the coordinates before clipping). > > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > Cc: Sinclair Yeh <syeh@vmware.com> > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I kinda wonder whether we shouldn't push this down into the helper ... Either way, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index 515b67783a41..efa41c086198 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -441,20 +441,23 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane, > int vmw_du_primary_plane_atomic_check(struct drm_plane *plane, > struct drm_plane_state *state) > { > + struct drm_crtc_state *crtc_state = NULL; > struct drm_framebuffer *new_fb = state->fb; > - struct drm_rect clip = { > - .x1 = state->crtc_x, > - .y1 = state->crtc_y, > - .x2 = state->crtc_x + state->crtc_w, > - .y2 = state->crtc_y + state->crtc_h, > - }; > + struct drm_rect clip = {}; > int ret; > > - ret = drm_plane_helper_check_state(state, &clip, > - DRM_PLANE_HELPER_NO_SCALING, > - DRM_PLANE_HELPER_NO_SCALING, > - false, true); > + if (state->crtc) > + crtc_state = drm_atomic_get_new_crtc_state(state->state, state->crtc); > > + if (crtc_state && crtc_state->enable) { > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > + } > + > + ret = drm_plane_helper_check_state(state, &clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + false, true); > > if (!ret && new_fb) { > struct drm_crtc *crtc = state->crtc; > -- > 2.13.6 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 02, 2017 at 11:12:09AM +0100, Daniel Vetter wrote: > On Wed, Nov 01, 2017 at 08:29:18PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Try to fix the code to actually clip the plane to the crtc bounds > > instead of the user provided crtc coordinates (which would be a no-op > > since those are exactly the coordinates before clipping). > > > > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > > Cc: Sinclair Yeh <syeh@vmware.com> > > Cc: Thomas Hellstrom <thellstrom@vmware.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > I kinda wonder whether we shouldn't push this down into the helper ... Would require quite going through all drivers making sure they are happy with using the adjusted_mode.crtc_ timings. I think most drivers use the other adjusted_mode timings currently, and some even use the user mode timings (which could be an actual bug). > > Either way, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 +++++++++++++---------- > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > index 515b67783a41..efa41c086198 100644 > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > @@ -441,20 +441,23 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane, > > int vmw_du_primary_plane_atomic_check(struct drm_plane *plane, > > struct drm_plane_state *state) > > { > > + struct drm_crtc_state *crtc_state = NULL; > > struct drm_framebuffer *new_fb = state->fb; > > - struct drm_rect clip = { > > - .x1 = state->crtc_x, > > - .y1 = state->crtc_y, > > - .x2 = state->crtc_x + state->crtc_w, > > - .y2 = state->crtc_y + state->crtc_h, > > - }; > > + struct drm_rect clip = {}; > > int ret; > > > > - ret = drm_plane_helper_check_state(state, &clip, > > - DRM_PLANE_HELPER_NO_SCALING, > > - DRM_PLANE_HELPER_NO_SCALING, > > - false, true); > > + if (state->crtc) > > + crtc_state = drm_atomic_get_new_crtc_state(state->state, state->crtc); > > > > + if (crtc_state && crtc_state->enable) { > > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > > + } > > + > > + ret = drm_plane_helper_check_state(state, &clip, > > + DRM_PLANE_HELPER_NO_SCALING, > > + DRM_PLANE_HELPER_NO_SCALING, > > + false, true); > > > > if (!ret && new_fb) { > > struct drm_crtc *crtc = state->crtc; > > -- > > 2.13.6 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Nov 02, 2017 at 03:19:30PM +0200, Ville Syrjälä wrote: > On Thu, Nov 02, 2017 at 11:12:09AM +0100, Daniel Vetter wrote: > > On Wed, Nov 01, 2017 at 08:29:18PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Try to fix the code to actually clip the plane to the crtc bounds > > > instead of the user provided crtc coordinates (which would be a no-op > > > since those are exactly the coordinates before clipping). > > > > > > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > > > Cc: Sinclair Yeh <syeh@vmware.com> > > > Cc: Thomas Hellstrom <thellstrom@vmware.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > I kinda wonder whether we shouldn't push this down into the helper ... > > Would require quite going through all drivers making sure they are > happy with using the adjusted_mode.crtc_ timings. I think most drivers > use the other adjusted_mode timings currently, and some even use the > user mode timings (which could be an actual bug). Let me take that back. What we want is to clip to the user mode timings. Stereo 3D needs the special treatment from drm_mode_get_hv_timing(). I'm getting confused by all these different timings we have all over the place. I think for i915 all we would need is to change the double wide/dual link adjustent for pipe_src_w to simply reject odd widths instead. That would guarantee that the user mode timings match the pipe_src_w/h 100%. For the other driver we'd need to figure out why they're using adjusted_mode timings for clipping. I guess that's just a mistake, which I repeated here with vmwgfx after getting confused by looking at the other drivers. I guess I just volunteered myself to do this. Just needs plenty of acks from driver maintainers I suppose. > > > > > Either way, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > --- > > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 +++++++++++++---------- > > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > > index 515b67783a41..efa41c086198 100644 > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > > @@ -441,20 +441,23 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane, > > > int vmw_du_primary_plane_atomic_check(struct drm_plane *plane, > > > struct drm_plane_state *state) > > > { > > > + struct drm_crtc_state *crtc_state = NULL; > > > struct drm_framebuffer *new_fb = state->fb; > > > - struct drm_rect clip = { > > > - .x1 = state->crtc_x, > > > - .y1 = state->crtc_y, > > > - .x2 = state->crtc_x + state->crtc_w, > > > - .y2 = state->crtc_y + state->crtc_h, > > > - }; > > > + struct drm_rect clip = {}; > > > int ret; > > > > > > - ret = drm_plane_helper_check_state(state, &clip, > > > - DRM_PLANE_HELPER_NO_SCALING, > > > - DRM_PLANE_HELPER_NO_SCALING, > > > - false, true); > > > + if (state->crtc) > > > + crtc_state = drm_atomic_get_new_crtc_state(state->state, state->crtc); > > > > > > + if (crtc_state && crtc_state->enable) { > > > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > > > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > > > + } > > > + > > > + ret = drm_plane_helper_check_state(state, &clip, > > > + DRM_PLANE_HELPER_NO_SCALING, > > > + DRM_PLANE_HELPER_NO_SCALING, > > > + false, true); > > > > > > if (!ret && new_fb) { > > > struct drm_crtc *crtc = state->crtc; > > > -- > > > 2.13.6 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 06, 2017 at 08:04:38PM +0200, Ville Syrjälä wrote: > On Thu, Nov 02, 2017 at 03:19:30PM +0200, Ville Syrjälä wrote: > > On Thu, Nov 02, 2017 at 11:12:09AM +0100, Daniel Vetter wrote: > > > On Wed, Nov 01, 2017 at 08:29:18PM +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Try to fix the code to actually clip the plane to the crtc bounds > > > > instead of the user provided crtc coordinates (which would be a no-op > > > > since those are exactly the coordinates before clipping). > > > > > > > > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > > > > Cc: Sinclair Yeh <syeh@vmware.com> > > > > Cc: Thomas Hellstrom <thellstrom@vmware.com> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > I kinda wonder whether we shouldn't push this down into the helper ... > > > > Would require quite going through all drivers making sure they are > > happy with using the adjusted_mode.crtc_ timings. I think most drivers > > use the other adjusted_mode timings currently, and some even use the > > user mode timings (which could be an actual bug). > > Let me take that back. What we want is to clip to the user mode > timings. Stereo 3D needs the special treatment from > drm_mode_get_hv_timing(). I'm getting confused by all these > different timings we have all over the place. > > I think for i915 all we would need is to change the double wide/dual > link adjustent for pipe_src_w to simply reject odd widths instead. > That would guarantee that the user mode timings match the pipe_src_w/h > 100%. > > For the other driver we'd need to figure out why they're using > adjusted_mode timings for clipping. I guess that's just a mistake, > which I repeated here with vmwgfx after getting confused by looking > at the other drivers. > > I guess I just volunteered myself to do this. Just needs plenty of > acks from driver maintainers I suppose. Yeah consistently using drm_mode_get_hv_timing from crtc_state->requested_mode seems like the right thing to do. Otherwise there's a driver bug lurking somewhere ... -Daniel > > > > > > > > > Either way, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > --- > > > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 +++++++++++++---------- > > > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > > > index 515b67783a41..efa41c086198 100644 > > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > > > @@ -441,20 +441,23 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane, > > > > int vmw_du_primary_plane_atomic_check(struct drm_plane *plane, > > > > struct drm_plane_state *state) > > > > { > > > > + struct drm_crtc_state *crtc_state = NULL; > > > > struct drm_framebuffer *new_fb = state->fb; > > > > - struct drm_rect clip = { > > > > - .x1 = state->crtc_x, > > > > - .y1 = state->crtc_y, > > > > - .x2 = state->crtc_x + state->crtc_w, > > > > - .y2 = state->crtc_y + state->crtc_h, > > > > - }; > > > > + struct drm_rect clip = {}; > > > > int ret; > > > > > > > > - ret = drm_plane_helper_check_state(state, &clip, > > > > - DRM_PLANE_HELPER_NO_SCALING, > > > > - DRM_PLANE_HELPER_NO_SCALING, > > > > - false, true); > > > > + if (state->crtc) > > > > + crtc_state = drm_atomic_get_new_crtc_state(state->state, state->crtc); > > > > > > > > + if (crtc_state && crtc_state->enable) { > > > > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > > > > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > > > > + } > > > > + > > > > + ret = drm_plane_helper_check_state(state, &clip, > > > > + DRM_PLANE_HELPER_NO_SCALING, > > > > + DRM_PLANE_HELPER_NO_SCALING, > > > > + false, true); > > > > > > > > if (!ret && new_fb) { > > > > struct drm_crtc *crtc = state->crtc; > > > > -- > > > > 2.13.6 > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
Hi Ville, On Monday, 6 November 2017 20:04:38 EET Ville Syrjälä wrote: > On Thu, Nov 02, 2017 at 03:19:30PM +0200, Ville Syrjälä wrote: > > On Thu, Nov 02, 2017 at 11:12:09AM +0100, Daniel Vetter wrote: > >> On Wed, Nov 01, 2017 at 08:29:18PM +0200, Ville Syrjala wrote: > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> > >>> Try to fix the code to actually clip the plane to the crtc bounds > >>> instead of the user provided crtc coordinates (which would be a no-op > >>> since those are exactly the coordinates before clipping). > >>> > >>> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > >>> Cc: Sinclair Yeh <syeh@vmware.com> > >>> Cc: Thomas Hellstrom <thellstrom@vmware.com> > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > >> I kinda wonder whether we shouldn't push this down into the helper ... I think that would be nice. > > Would require quite going through all drivers making sure they are > > happy with using the adjusted_mode.crtc_ timings. I think most drivers > > use the other adjusted_mode timings currently, and some even use the > > user mode timings (which could be an actual bug). > > Let me take that back. What we want is to clip to the user mode > timings. Stereo 3D needs the special treatment from > drm_mode_get_hv_timing(). I'm getting confused by all these > different timings we have all over the place. > > I think for i915 all we would need is to change the double wide/dual > link adjustent for pipe_src_w to simply reject odd widths instead. > That would guarantee that the user mode timings match the pipe_src_w/h > 100%. > > For the other driver we'd need to figure out why they're using > adjusted_mode timings for clipping. I guess that's just a mistake, > which I repeated here with vmwgfx after getting confused by looking > at the other drivers. I suppose it's a mix of cargo-cult and the fact that adjusted_mode hints that it contains the timings that should be applied to the hardware. That's why I use it in rcar-du. Are drivers allowed to adjust the hdisplay and vdisplay values though ? I thought unsupported values there had to be rejected with an error at atomic check time, not adjusted. > I guess I just volunteered myself to do this. Just needs plenty of > acks from driver maintainers I suppose. I'll review and test the rcar-du patch :-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 515b67783a41..efa41c086198 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -441,20 +441,23 @@ vmw_du_cursor_plane_atomic_update(struct drm_plane *plane, int vmw_du_primary_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state) { + struct drm_crtc_state *crtc_state = NULL; struct drm_framebuffer *new_fb = state->fb; - struct drm_rect clip = { - .x1 = state->crtc_x, - .y1 = state->crtc_y, - .x2 = state->crtc_x + state->crtc_w, - .y2 = state->crtc_y + state->crtc_h, - }; + struct drm_rect clip = {}; int ret; - ret = drm_plane_helper_check_state(state, &clip, - DRM_PLANE_HELPER_NO_SCALING, - DRM_PLANE_HELPER_NO_SCALING, - false, true); + if (state->crtc) + crtc_state = drm_atomic_get_new_crtc_state(state->state, state->crtc); + if (crtc_state && crtc_state->enable) { + clip.x2 = crtc_state->adjusted_mode.hdisplay; + clip.y2 = crtc_state->adjusted_mode.vdisplay; + } + + ret = drm_plane_helper_check_state(state, &clip, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + false, true); if (!ret && new_fb) { struct drm_crtc *crtc = state->crtc;