[Mesa-dev] [PATCH] gallium/winsys/kms: Add support for multi-planes (v2)

[Emil Velikov]

HI Lepton,

It seems that this has fallen through the cracks.

There's one important functionality change which should be split out
and documented.
The rest is just style nitpicks.

On 28 December 2017 at 07:35, Lepton Wu <lepton at chromium.org> wrote:
> v2: address comments from Tomasz Figa
>    a) Add more check for plane size.
>    b) Avoid duplicated mapping and leaked mapping.
>    c) Other minor changes.
Normally I omit saying "other minor changes" since it don't mean anything.


> @@ -105,6 +114,42 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
>     return TRUE;
>  }
>
> +static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
> +                                      enum pipe_format format,
> +                                      unsigned width, unsigned height,
> +                                      unsigned stride, unsigned offset) {
Opening bracket should be at the start of next line.

> +   struct kms_sw_plane * tmp, * plane = NULL;
Through the patch: no space between * and variable name - example: s/* tmp/*tmp/

Add empty line between declarations and code.

> +   if (offset + util_format_get_2d_size(format, stride, height) >
> +       kms_sw_dt->size) {
> +      DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
> +                  "offset: %d size:%d\n", format, stride, height, offset,
> +                  kms_sw_dt->size);
> +      return NULL;
> +   }
> +   LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
> +      if (tmp->offset == offset) {
> +         plane = tmp;
> +         break;
> +      }
> +   }
> +   if (plane) {
> +      assert(plane->width == width);
> +      assert(plane->height == height);
> +      assert(plane->stride == stride);
> +      assert(plane->dt == kms_sw_dt);
The only way to hit this if there's serious corruption happening. I'd
just drop it and "return tmp;" in the loop above.

> +   } else {
> +      plane = CALLOC_STRUCT(kms_sw_plane);
> +      if (plane == NULL) return NULL;
The return statement should be on separate line.

Thorough the patch: swap "foo == NULL" with "!foo"

> @@ -138,17 +182,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,

> -   *stride = kms_sw_dt->stride;
> -   return (struct sw_displaytarget *)kms_sw_dt;
> -
> +   *stride = create_req.pitch;
> +   return (struct sw_displaytarget *) plane;
Swap the cast with an inline wrapper analogous to the kms_sw_plane() one.


> @@ -163,13 +209,19 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>                               struct sw_displaytarget *dt)
>  {
>     struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
> -   struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
> +   struct kms_sw_plane *plane = kms_sw_plane(dt);
> +   struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
>     struct drm_mode_destroy_dumb destroy_req;
>
>     kms_sw_dt->ref_count --;
>     if (kms_sw_dt->ref_count > 0)
>        return;
>
> +   if (kms_sw_dt->ro_mapped)
> +     munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
> +   if (kms_sw_dt->mapped)
> +     munmap(kms_sw_dt->mapped, kms_sw_dt->size);
> +
This hunk moves the munmap from dt_unmap to dt_destroy.
It should be safe, although it is a subtle functionality change that
should be split out.
A commit message should explain why it's needed, etc.

Ideally, the ro_mapped introduction will be another patch.
Alternatively the commit message should mention it.


> @@ -198,16 +255,20 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
>        return NULL;
>
>     prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
> -   kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
> -                            kms_sw->fd, map_req.offset);
> -
> -   if (kms_sw_dt->mapped == MAP_FAILED)
> -      return NULL;
> +   void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
> +   if (*ptr == NULL) {
To prevent interment breakage, this NULL check must be part of the
delayer munmap patch.

HTH
Emil