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

[Lepton Wu]

Thanks for reviewing, just back from vacation and send out V3 for review.
I split it to 2 patches now and I removed lazy unmap since it's unnecessary.

On Wed, Feb 21, 2018 at 7:42 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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