Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757104AbbBFXcV (ORCPT ); Fri, 6 Feb 2015 18:32:21 -0500 Received: from mail-ig0-f181.google.com ([209.85.213.181]:36538 "EHLO mail-ig0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753800AbbBFXcU (ORCPT ); Fri, 6 Feb 2015 18:32:20 -0500 MIME-Version: 1.0 In-Reply-To: <20150206211147.GF14009@phenom.ffwll.local> References: <1423236662-6782-1-git-send-email-boris.brezillon@free-electrons.com> <20150206211147.GF14009@phenom.ffwll.local> Date: Fri, 6 Feb 2015 18:32:19 -0500 Message-ID: Subject: Re: [PATCH] drm: atmel-hlcdc: add discard area support From: Rob Clark To: Boris Brezillon , David Airlie , "dri-devel@lists.freedesktop.org" , Nicolas Ferre , Jean-Christophe Plagniol-Villard , Alexandre Belloni , "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List , Rob Clark , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= Cc: Daniel Vetter Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9514 Lines: 247 On Fri, Feb 6, 2015 at 4:11 PM, Daniel Vetter wrote: > On Fri, Feb 06, 2015 at 04:31:02PM +0100, Boris Brezillon wrote: >> The HLCDC IP provides a way to discard a specific area on the primary >> plane (in case at least one of the overlay is activated and alpha >> blending is disabled). >> Doing this will reduce the amount of data to transfer from the main >> memory to the Display Controller, and thus alleviate the load on the >> memory bus (since this link is quite limited on such hardware, >> this kind of optimization is really important). >> >> Signed-off-by: Boris Brezillon >> --- >> Hi Daniel, >> >> Can you tell me if that's what you had in mind for the discard area feature >> we talked about yersterday. >> >> This patch depends on the Atmel HLCDC Atomic mode-setting conversion >> work [1]. >> >> Best Regards, >> >> Boris >> >> [1]https://lkml.org/lkml/2015/2/4/658 >> >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 2 +- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 2 + >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 108 ++++++++++++++++++++++++ >> 3 files changed, 111 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> index de6973d..4fd1683 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> @@ -201,7 +201,7 @@ static int atmel_hlcdc_crtc_atomic_check(struct drm_crtc *c, >> if (atmel_hlcdc_dc_mode_valid(crtc->dc, &s->adjusted_mode) != MODE_OK) >> return -EINVAL; >> >> - return 0; >> + return atmel_hlcdc_plane_prepare_disc_area(s); >> } >> >> static void atmel_hlcdc_crtc_atomic_begin(struct drm_crtc *c) >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h >> index 015c3f1..1ea9c2c 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h >> @@ -148,6 +148,8 @@ int atmel_hlcdc_dc_mode_valid(struct atmel_hlcdc_dc *dc, >> struct atmel_hlcdc_planes * >> atmel_hlcdc_create_planes(struct drm_device *dev); >> >> +int atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state *c_state); >> + >> void atmel_hlcdc_crtc_irq(struct drm_crtc *c); >> >> void atmel_hlcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> index 6c6fcae..dbf97d9 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> @@ -51,6 +51,13 @@ struct atmel_hlcdc_plane_state { >> >> u8 alpha; >> >> + bool disc_updated; >> + >> + int disc_x; >> + int disc_y; >> + int disc_w; >> + int disc_h; >> + >> /* These fields are private and should not be touched */ >> int bpp[ATMEL_HLCDC_MAX_PLANES]; >> unsigned int offsets[ATMEL_HLCDC_MAX_PLANES]; >> @@ -428,6 +435,104 @@ static void atmel_hlcdc_plane_update_buffers(struct atmel_hlcdc_plane *plane, >> } >> } >> >> +int >> +atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state *c_state) >> +{ >> + int disc_x = 0, disc_y = 0, disc_w = 0, disc_h = 0; >> + const struct atmel_hlcdc_layer_cfg_layout *layout; >> + struct atmel_hlcdc_plane_state *primary_state; >> + struct drm_plane_state *primary_s; >> + struct atmel_hlcdc_plane *primary; >> + struct drm_plane *ovl; >> + >> + primary = drm_plane_to_atmel_hlcdc_plane(c_state->crtc->primary); >> + layout = &primary->layer.desc->layout; >> + if (!layout->disc_pos || !layout->disc_size) >> + return 0; >> + >> + primary_s = drm_atomic_get_plane_state(c_state->state, >> + &primary->base); >> + if (IS_ERR(primary_s)) >> + return PTR_ERR(primary_s); >> + >> + primary_state = drm_plane_state_to_atmel_hlcdc_plane_state(primary_s); >> + >> + drm_atomic_crtc_state_for_each_plane(ovl, c_state) { >> + struct atmel_hlcdc_plane_state *ovl_state; >> + struct drm_plane_state *ovl_s; >> + >> + if (ovl == c_state->crtc->primary) >> + continue; >> + >> + ovl_s = drm_atomic_get_plane_state(c_state->state, ovl); >> + if (IS_ERR(ovl_s)) >> + return PTR_ERR(ovl_s); >> + >> + ovl_state = drm_plane_state_to_atmel_hlcdc_plane_state(ovl_s); > > Note that right now the atomic helpers do not no-op out null plane state > updates. The assumption is that such plane states aren't requested. > Usually it's not harmful to add more planes by default (just a bit of > busy-work really), but if you have more than 1 crtc then suddenly every > pageflip will be converted into an update spanning more than 1 crtc, and > you most definitely don't want that. This is because we always must the > state for the crtc each plane is connected to. This is also the reason why > my speudo-code started from the planes and not the crtc, so that > unecessary plane states can be avoided. > so maybe a: const struct drm_xyz_state * drm_atomic_get_xyz_state_ro(...); which does not create new state, if there is not already one, but instead just returns the current state for planes that are not being changed? might need to be a bit careful about state state pointers if someone subsequently does drm_atomic_get_xyz_state().. but that shouldn't normally be the case in the check phase. If needed there could be some sanity checking to WARN() on drm_atomic_get_xyz_state() after drm_atomic_get_xyz_state_ro() BR, -R > If that's a problem I can try to look into some way to remove a plane > state from an atomic update. > > Assuming this isn't an issue for your hw the overall logic here looks > sound, so > > Acked-by: Daniel Vetter > >> + >> + if (!ovl_s->fb || >> + atmel_hlcdc_format_embeds_alpha(ovl_s->fb->pixel_format) || >> + ovl_state->alpha != 255) >> + continue; >> + >> + /* TODO: implement a smarter hidden area detection */ >> + if (ovl_state->crtc_h * ovl_state->crtc_w < disc_h * disc_w) >> + continue; >> + >> + disc_x = ovl_state->crtc_x; >> + disc_y = ovl_state->crtc_y; >> + disc_h = ovl_state->crtc_h; >> + disc_w = ovl_state->crtc_w; >> + } >> + >> + if (disc_x == primary_state->disc_x && >> + disc_y == primary_state->disc_y && >> + disc_w == primary_state->disc_w && >> + disc_h == primary_state->disc_h) >> + return 0; >> + >> + >> + primary_state->disc_x = disc_x; >> + primary_state->disc_y = disc_y; >> + primary_state->disc_w = disc_w; >> + primary_state->disc_h = disc_h; >> + primary_state->disc_updated = true; >> + >> + return 0; >> +} >> + >> +static void >> +atmel_hlcdc_plane_update_disc_area(struct atmel_hlcdc_plane *plane, >> + struct atmel_hlcdc_plane_state *state) >> +{ >> + const struct atmel_hlcdc_layer_cfg_layout *layout = >> + &plane->layer.desc->layout; >> + int disc_surface = 0; >> + >> + if (!state->disc_updated) >> + return; >> + >> + disc_surface = state->disc_h * state->disc_w; >> + >> + atmel_hlcdc_layer_update_cfg(&plane->layer, layout->general_config, >> + ATMEL_HLCDC_LAYER_DISCEN, >> + disc_surface ? ATMEL_HLCDC_LAYER_DISCEN : 0); >> + >> + if (!disc_surface) >> + return; >> + >> + atmel_hlcdc_layer_update_cfg(&plane->layer, >> + layout->disc_pos, >> + 0xffffffff, >> + state->disc_x | (state->disc_y << 16)); >> + >> + atmel_hlcdc_layer_update_cfg(&plane->layer, >> + layout->disc_size, >> + 0xffffffff, >> + (state->disc_w - 1) | >> + ((state->disc_h - 1) << 16)); >> +} >> + >> static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, >> struct drm_plane_state *s) >> { >> @@ -628,6 +733,7 @@ static void atmel_hlcdc_plane_atomic_update(struct drm_plane *p, >> atmel_hlcdc_plane_update_general_settings(plane, state); >> atmel_hlcdc_plane_update_format(plane, state); >> atmel_hlcdc_plane_update_buffers(plane, state); >> + atmel_hlcdc_plane_update_disc_area(plane, state); >> >> atmel_hlcdc_layer_update_commit(&plane->layer); >> } >> @@ -773,6 +879,8 @@ atmel_hlcdc_plane_atomic_duplicate_state(struct drm_plane *p) >> if (!copy) >> return NULL; >> >> + copy->disc_updated = false; >> + >> if (copy->base.fb) >> drm_framebuffer_reference(copy->base.fb); >> >> -- >> 1.9.1 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/