Re: [PATCH] drm: atmel-hlcdc: add discard area support

[Rob Clark]

On Fri, Feb 6, 2015 at 4:11 PM, Daniel Vetter <daniel@xxxxxxxx> 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 <boris.brezillon@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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 <daniel.vetter@xxxxxxxx>
>
>> +
>> +             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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/