| Message ID | 1469549224-1860-9-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 26, 2016 at 12:07 PM, <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Replace the use of drm_plane_helper_check_update() with > drm_plane_helper_check_state() since we have a plane state. > > This also eliminates the double clipping the driver was doing > in both check and commit phases). And it should fix src coordinate > addr adjustement. Previously the driver was expecting negative dst > coordinates after clipping, which is not going happen, so any clipping > induced addr adjustment simply didn't happen. Neither did the driver > respect any user configured src coordinates, so panning and such would > have been totally broken. It should be all good now. > > Cc: CK Hu <ck.hu@mediatek.com> > Cc: linux-mediatek@lists.infradead.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 72 +++++++++----------------------- > 1 file changed, 20 insertions(+), 52 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index 3995765a90dc..5f2516fca079 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -30,15 +30,20 @@ static const u32 formats[] = { > DRM_FORMAT_RGB565, > }; > > -static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, > - dma_addr_t addr, struct drm_rect *dest) > +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, > + dma_addr_t addr) > { > struct drm_plane *plane = &mtk_plane->base; > struct mtk_plane_state *state = to_mtk_plane_state(plane->state); > unsigned int pitch, format; > - int x, y; > + bool enable; > > - if (WARN_ON(!plane->state || (enable && !plane->state->fb))) > + if (WARN_ON(!plane->state)) > + return; > + > + enable = state->base.visible; > + > + if (WARN_ON(enable && !plane->state->fb)) > return; > > if (plane->state->fb) { > @@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, > format = DRM_FORMAT_RGBA8888; > } > > - x = plane->state->crtc_x; > - y = plane->state->crtc_y; > - > - if (x < 0) { > - addr -= x * 4; > - x = 0; > - } > - > - if (y < 0) { > - addr -= y * pitch; > - y = 0; > - } > + addr += (state->base.src.x1 >> 16) * 4; > + addr += (state->base.src.y1 >> 16) * pitch; > > state->pending.enable = enable; > state->pending.pitch = pitch; > state->pending.format = format; > state->pending.addr = addr; > - state->pending.x = x; > - state->pending.y = y; > - state->pending.width = dest->x2 - dest->x1; > - state->pending.height = dest->y2 - dest->y1; > + state->pending.x = state->base.dst.x1; > + state->pending.y = state->base.dst.y1; > + state->pending.width = drm_rect_width(&state->base.dst); > + state->pending.height = drm_rect_height(&state->base.dst); > wmb(); /* Make sure the above parameters are set before update */ > state->pending.dirty = true; > } > @@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > { > struct drm_framebuffer *fb = state->fb; > struct drm_crtc_state *crtc_state; > - bool visible; > - struct drm_rect dest = { > - .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 src = { > - /* 16.16 fixed point */ > - .x1 = state->src_x, > - .y1 = state->src_y, > - .x2 = state->src_x + state->src_w, > - .y2 = state->src_y + state->src_h, > - }; > struct drm_rect clip = { 0, }; > > if (!fb) > @@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > clip.x2 = crtc_state->mode.hdisplay; > clip.y2 = crtc_state->mode.vdisplay; > > - return drm_plane_helper_check_update(plane, state->crtc, fb, > - &src, &dest, &clip, > - state->rotation, > - DRM_PLANE_HELPER_NO_SCALING, > - DRM_PLANE_HELPER_NO_SCALING, > - true, true, &visible); > + return drm_plane_helper_check_state(state, &clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + true, true); > } > > static void mtk_plane_atomic_update(struct drm_plane *plane, > @@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane, > struct drm_gem_object *gem; > struct mtk_drm_gem_obj *mtk_gem; > struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane); > - struct drm_rect dest = { > - .x1 = state->base.crtc_x, > - .y1 = state->base.crtc_y, > - .x2 = state->base.crtc_x + state->base.crtc_w, > - .y2 = state->base.crtc_y + state->base.crtc_h, > - }; > - struct drm_rect clip = { 0, }; > > if (!crtc) > return; > > - clip.x2 = state->base.crtc->state->mode.hdisplay; > - clip.y2 = state->base.crtc->state->mode.vdisplay; > - drm_rect_intersect(&dest, &clip); > - > gem = mtk_fb_get_gem_obj(state->base.fb); > mtk_gem = to_mtk_gem_obj(gem); > - mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest); > + mtk_plane_enable(mtk_plane, mtk_gem->dma_addr); > } > > static void mtk_plane_atomic_disable(struct drm_plane *plane, > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2016-07-26 at 19:07 +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Replace the use of drm_plane_helper_check_update() with > drm_plane_helper_check_state() since we have a plane state. > > This also eliminates the double clipping the driver was doing > in both check and commit phases). And it should fix src coordinate > addr adjustement. Previously the driver was expecting negative dst > coordinates after clipping, which is not going happen, so any clipping > induced addr adjustment simply didn't happen. Neither did the driver > respect any user configured src coordinates, so panning and such would > have been totally broken. It should be all good now. > > Cc: CK Hu <ck.hu@mediatek.com> > Cc: linux-mediatek@lists.infradead.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> This patch looks fine to me, thanks for your patch. Reviewed-by: Bibby Hsieh <bibby.hsieh@mediatek.com> Tested-by: Bibby Hsieh <bibby.hsieh@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 72 +++++++++----------------------- > 1 file changed, 20 insertions(+), 52 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index 3995765a90dc..5f2516fca079 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -30,15 +30,20 @@ static const u32 formats[] = { > DRM_FORMAT_RGB565, > }; > > -static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, > - dma_addr_t addr, struct drm_rect *dest) > +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, > + dma_addr_t addr) > { > struct drm_plane *plane = &mtk_plane->base; > struct mtk_plane_state *state = to_mtk_plane_state(plane->state); > unsigned int pitch, format; > - int x, y; > + bool enable; > > - if (WARN_ON(!plane->state || (enable && !plane->state->fb))) > + if (WARN_ON(!plane->state)) > + return; > + > + enable = state->base.visible; > + > + if (WARN_ON(enable && !plane->state->fb)) > return; > > if (plane->state->fb) { > @@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, > format = DRM_FORMAT_RGBA8888; > } > > - x = plane->state->crtc_x; > - y = plane->state->crtc_y; > - > - if (x < 0) { > - addr -= x * 4; > - x = 0; > - } > - > - if (y < 0) { > - addr -= y * pitch; > - y = 0; > - } > + addr += (state->base.src.x1 >> 16) * 4; > + addr += (state->base.src.y1 >> 16) * pitch; > > state->pending.enable = enable; > state->pending.pitch = pitch; > state->pending.format = format; > state->pending.addr = addr; > - state->pending.x = x; > - state->pending.y = y; > - state->pending.width = dest->x2 - dest->x1; > - state->pending.height = dest->y2 - dest->y1; > + state->pending.x = state->base.dst.x1; > + state->pending.y = state->base.dst.y1; > + state->pending.width = drm_rect_width(&state->base.dst); > + state->pending.height = drm_rect_height(&state->base.dst); > wmb(); /* Make sure the above parameters are set before update */ > state->pending.dirty = true; > } > @@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > { > struct drm_framebuffer *fb = state->fb; > struct drm_crtc_state *crtc_state; > - bool visible; > - struct drm_rect dest = { > - .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 src = { > - /* 16.16 fixed point */ > - .x1 = state->src_x, > - .y1 = state->src_y, > - .x2 = state->src_x + state->src_w, > - .y2 = state->src_y + state->src_h, > - }; > struct drm_rect clip = { 0, }; > > if (!fb) > @@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > clip.x2 = crtc_state->mode.hdisplay; > clip.y2 = crtc_state->mode.vdisplay; > > - return drm_plane_helper_check_update(plane, state->crtc, fb, > - &src, &dest, &clip, > - state->rotation, > - DRM_PLANE_HELPER_NO_SCALING, > - DRM_PLANE_HELPER_NO_SCALING, > - true, true, &visible); > + return drm_plane_helper_check_state(state, &clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + true, true); > } > > static void mtk_plane_atomic_update(struct drm_plane *plane, > @@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane, > struct drm_gem_object *gem; > struct mtk_drm_gem_obj *mtk_gem; > struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane); > - struct drm_rect dest = { > - .x1 = state->base.crtc_x, > - .y1 = state->base.crtc_y, > - .x2 = state->base.crtc_x + state->base.crtc_w, > - .y2 = state->base.crtc_y + state->base.crtc_h, > - }; > - struct drm_rect clip = { 0, }; > > if (!crtc) > return; > > - clip.x2 = state->base.crtc->state->mode.hdisplay; > - clip.y2 = state->base.crtc->state->mode.vdisplay; > - drm_rect_intersect(&dest, &clip); > - > gem = mtk_fb_get_gem_obj(state->base.fb); > mtk_gem = to_mtk_gem_obj(gem); > - mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest); > + mtk_plane_enable(mtk_plane, mtk_gem->dma_addr); > } > > static void mtk_plane_atomic_disable(struct drm_plane *plane,
On Tue, 2016-07-26 at 19:07 +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Replace the use of drm_plane_helper_check_update() with > drm_plane_helper_check_state() since we have a plane state. > > This also eliminates the double clipping the driver was doing > in both check and commit phases). And it should fix src coordinate > addr adjustement. Previously the driver was expecting negative dst > coordinates after clipping, which is not going happen, so any clipping > induced addr adjustment simply didn't happen. Neither did the driver > respect any user configured src coordinates, so panning and such would > have been totally broken. It should be all good now. > > Cc: CK Hu <ck.hu@mediatek.com> > Cc: linux-mediatek@lists.infradead.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- Acked-by: CK Hu <ck.hu@mediatek.com> > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 72 +++++++++----------------------- > 1 file changed, 20 insertions(+), 52 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index 3995765a90dc..5f2516fca079 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -30,15 +30,20 @@ static const u32 formats[] = { > DRM_FORMAT_RGB565, > }; > > -static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, > - dma_addr_t addr, struct drm_rect *dest) > +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, > + dma_addr_t addr) > { > struct drm_plane *plane = &mtk_plane->base; > struct mtk_plane_state *state = to_mtk_plane_state(plane->state); > unsigned int pitch, format; > - int x, y; > + bool enable; > > - if (WARN_ON(!plane->state || (enable && !plane->state->fb))) > + if (WARN_ON(!plane->state)) > + return; > + > + enable = state->base.visible; > + > + if (WARN_ON(enable && !plane->state->fb)) > return; > > if (plane->state->fb) { > @@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, > format = DRM_FORMAT_RGBA8888; > } > > - x = plane->state->crtc_x; > - y = plane->state->crtc_y; > - > - if (x < 0) { > - addr -= x * 4; > - x = 0; > - } > - > - if (y < 0) { > - addr -= y * pitch; > - y = 0; > - } > + addr += (state->base.src.x1 >> 16) * 4; > + addr += (state->base.src.y1 >> 16) * pitch; > > state->pending.enable = enable; > state->pending.pitch = pitch; > state->pending.format = format; > state->pending.addr = addr; > - state->pending.x = x; > - state->pending.y = y; > - state->pending.width = dest->x2 - dest->x1; > - state->pending.height = dest->y2 - dest->y1; > + state->pending.x = state->base.dst.x1; > + state->pending.y = state->base.dst.y1; > + state->pending.width = drm_rect_width(&state->base.dst); > + state->pending.height = drm_rect_height(&state->base.dst); > wmb(); /* Make sure the above parameters are set before update */ > state->pending.dirty = true; > } > @@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > { > struct drm_framebuffer *fb = state->fb; > struct drm_crtc_state *crtc_state; > - bool visible; > - struct drm_rect dest = { > - .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 src = { > - /* 16.16 fixed point */ > - .x1 = state->src_x, > - .y1 = state->src_y, > - .x2 = state->src_x + state->src_w, > - .y2 = state->src_y + state->src_h, > - }; > struct drm_rect clip = { 0, }; > > if (!fb) > @@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > clip.x2 = crtc_state->mode.hdisplay; > clip.y2 = crtc_state->mode.vdisplay; > > - return drm_plane_helper_check_update(plane, state->crtc, fb, > - &src, &dest, &clip, > - state->rotation, > - DRM_PLANE_HELPER_NO_SCALING, > - DRM_PLANE_HELPER_NO_SCALING, > - true, true, &visible); > + return drm_plane_helper_check_state(state, &clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + true, true); > } > > static void mtk_plane_atomic_update(struct drm_plane *plane, > @@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane, > struct drm_gem_object *gem; > struct mtk_drm_gem_obj *mtk_gem; > struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane); > - struct drm_rect dest = { > - .x1 = state->base.crtc_x, > - .y1 = state->base.crtc_y, > - .x2 = state->base.crtc_x + state->base.crtc_w, > - .y2 = state->base.crtc_y + state->base.crtc_h, > - }; > - struct drm_rect clip = { 0, }; > > if (!crtc) > return; > > - clip.x2 = state->base.crtc->state->mode.hdisplay; > - clip.y2 = state->base.crtc->state->mode.vdisplay; > - drm_rect_intersect(&dest, &clip); > - > gem = mtk_fb_get_gem_obj(state->base.fb); > mtk_gem = to_mtk_gem_obj(gem); > - mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest); > + mtk_plane_enable(mtk_plane, mtk_gem->dma_addr); > } > > static void mtk_plane_atomic_disable(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c index 3995765a90dc..5f2516fca079 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c @@ -30,15 +30,20 @@ static const u32 formats[] = { DRM_FORMAT_RGB565, }; -static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, - dma_addr_t addr, struct drm_rect *dest) +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, + dma_addr_t addr) { struct drm_plane *plane = &mtk_plane->base; struct mtk_plane_state *state = to_mtk_plane_state(plane->state); unsigned int pitch, format; - int x, y; + bool enable; - if (WARN_ON(!plane->state || (enable && !plane->state->fb))) + if (WARN_ON(!plane->state)) + return; + + enable = state->base.visible; + + if (WARN_ON(enable && !plane->state->fb)) return; if (plane->state->fb) { @@ -49,27 +54,17 @@ static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable, format = DRM_FORMAT_RGBA8888; } - x = plane->state->crtc_x; - y = plane->state->crtc_y; - - if (x < 0) { - addr -= x * 4; - x = 0; - } - - if (y < 0) { - addr -= y * pitch; - y = 0; - } + addr += (state->base.src.x1 >> 16) * 4; + addr += (state->base.src.y1 >> 16) * pitch; state->pending.enable = enable; state->pending.pitch = pitch; state->pending.format = format; state->pending.addr = addr; - state->pending.x = x; - state->pending.y = y; - state->pending.width = dest->x2 - dest->x1; - state->pending.height = dest->y2 - dest->y1; + state->pending.x = state->base.dst.x1; + state->pending.y = state->base.dst.y1; + state->pending.width = drm_rect_width(&state->base.dst); + state->pending.height = drm_rect_height(&state->base.dst); wmb(); /* Make sure the above parameters are set before update */ state->pending.dirty = true; } @@ -134,20 +129,6 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, { struct drm_framebuffer *fb = state->fb; struct drm_crtc_state *crtc_state; - bool visible; - struct drm_rect dest = { - .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 src = { - /* 16.16 fixed point */ - .x1 = state->src_x, - .y1 = state->src_y, - .x2 = state->src_x + state->src_w, - .y2 = state->src_y + state->src_h, - }; struct drm_rect clip = { 0, }; if (!fb) @@ -168,12 +149,10 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, clip.x2 = crtc_state->mode.hdisplay; clip.y2 = crtc_state->mode.vdisplay; - return drm_plane_helper_check_update(plane, state->crtc, fb, - &src, &dest, &clip, - state->rotation, - DRM_PLANE_HELPER_NO_SCALING, - DRM_PLANE_HELPER_NO_SCALING, - true, true, &visible); + return drm_plane_helper_check_state(state, &clip, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + true, true); } static void mtk_plane_atomic_update(struct drm_plane *plane, @@ -184,24 +163,13 @@ static void mtk_plane_atomic_update(struct drm_plane *plane, struct drm_gem_object *gem; struct mtk_drm_gem_obj *mtk_gem; struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane); - struct drm_rect dest = { - .x1 = state->base.crtc_x, - .y1 = state->base.crtc_y, - .x2 = state->base.crtc_x + state->base.crtc_w, - .y2 = state->base.crtc_y + state->base.crtc_h, - }; - struct drm_rect clip = { 0, }; if (!crtc) return; - clip.x2 = state->base.crtc->state->mode.hdisplay; - clip.y2 = state->base.crtc->state->mode.vdisplay; - drm_rect_intersect(&dest, &clip); - gem = mtk_fb_get_gem_obj(state->base.fb); mtk_gem = to_mtk_gem_obj(gem); - mtk_plane_enable(mtk_plane, true, mtk_gem->dma_addr, &dest); + mtk_plane_enable(mtk_plane, mtk_gem->dma_addr); } static void mtk_plane_atomic_disable(struct drm_plane *plane,