Re: [PATCH 3/5] drm: introduce pipe color correction properties

[Emil Velikov <emil.l.velikov@gmail.com>]

On 26 February 2016 at 15:43, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
> On 26/02/16 00:36, Emil Velikov wrote:
>>
>> Hi Lionel,
>>
>> A bunch of suggestions - feel free to take or ignore them :-)
>>
>> On 25 February 2016 at 10:58, Lionel Landwerlin
>> <lionel.g.landwerlin@intel.com> wrote:

> I'm not sure it matters as the drm_crtc_state you're set properties on will
> be discarded if there is an error.
> The current drm_crtc_state that has been applied onto the hardware should be
> untouched.
>
That's the thing - the current drm_crts_state (mode_blob) is being
discarded, as opposed to the newly setup one. Although I could have
misunderstood something.


>
> This is because we accept more than one size of degamma/gamma LUT (legacy ->
> 256 elements, new LUT -> (de)gamma_lut_size elements).
> It's up for the driver the check the size and raise an error in its
> atomic_check() vfunc.
>
Ahh yes the legacy vs atomic difference.


>
> Moving the if (blob == NULL) right after the blob allocation to make it
> simpler.
>
> What about completeness? Is there something inherently wrong here?
The suggestion to reorder things is mostly to keep the setup/teardown
order in reverse order - create_blob, create_state and drop_state,
drop_blob. As you've noticed, I'm kind of a sucker for those :-) There
are no inter-dependencies that would require it here so it's not
required.

>>
>>
>>> +       state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
>>> +retry:
>>> +       crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>> +       if (IS_ERR(crtc_state)) {
>>> +               ret = PTR_ERR(crtc_state);
>>> +               goto fail;
>>> +       }
>>> +
>>> +       /* Reset DEGAMMA_LUT and CTM properties. */
>>> +       ret = drm_atomic_crtc_set_property(crtc, crtc_state,
>>> +                       config->degamma_lut_property, 0);
>>> +       if (ret)
>>> +               goto fail;
>>
>> Add new blank line please.
>
>
> Sure.
>>
>>
>>> +       ret = drm_atomic_crtc_set_property(crtc, crtc_state,
>>> +                       config->ctm_property, 0);
>>> +       if (ret)
>>> +               goto fail;
>>> +
>>> +       /* Set GAMMA_LUT with legacy values. */
>>> +       if (blob == NULL) {
>>> +               ret = -ENOMEM;
>>> +               goto fail;
>>> +       }
>>> +
>>> +       blob_data = (struct drm_color_lut *) blob->data;
>>> +       for (i = 0; i < size; i++) {
>>> +               blob_data[i].red = red[i];
>>> +               blob_data[i].green = green[i];
>>> +               blob_data[i].blue = blue[i];
>>> +       }
>>> +
>>
>> Move this loop after create_blob()
>
> Thanks, indeed no need to refill it in case of retry.
>
>>
>>> +       ret = drm_atomic_crtc_set_property(crtc, crtc_state,
>>> +                       config->gamma_lut_property, blob->base.id);
>>> +       if (ret)
>>> +               goto fail;
>>> +
>>> +       ret = drm_atomic_commit(state);
>>> +       if (ret != 0)
>>
>> Please check in a consistent way. Currently we have ret != 0 vs ret
>> and foo == NULL vs !foo.
>
>
> Sure.
>
>>
>>> +               goto fail;
>>> +
>>> +       drm_property_unreference_blob(blob);
>>> +
>>> +       /* Driver takes ownership of state on successful commit. */
>>
>> Move the comment before unreference_blob(), so that it's closer to
>> atomic_commit() ?
>
>
> Sure.
>>
>>
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -1554,6 +1554,41 @@ static int
>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>>                  return -ENOMEM;
>>>          dev->mode_config.prop_mode_id = prop;
>>>
>>> +       prop = drm_property_create(dev,
>>> +                       DRM_MODE_PROP_BLOB,
>>> +                       "DEGAMMA_LUT", 0);
>>
>> Just wondering -  don't we want this and the remaining properties to
>> be atomic only ? I doubt we have userspace that [will be updated to]
>> handle these, yet lacks atomic.
>
> This was pointed out by Matt already. Here is Daniel Stone's response :
> https://lists.freedesktop.org/archives/intel-gfx/2016-January/086120.html
>
> I think it's fine to have these properties not atomic because it's not
> really something you update very often (maybe just when starting your UI).
> That's actually how we would like to use them in ChromiumOS as a first step,
> until eventually ChromiumOS switches to atomic.
>
It wasn't a question of "can it be used" but more of "would it make
sense to not switch to atomics once we're here". As is, userspace will
need to have two slightly different code paths. Put the question "how
to deal with if compositor crashes and (re)applying gamma/etc.
multiple times" by Daniel Vettel, on top of it all and things get
extra messy. In both usespace in kernel.

My line of thought is - if there is a high demand for
non-atomic(legacy) degamma/etc. one can easily add it. On the other
hand, once it lands one cannot remove the code from the kernel, ever.

It's up-to you guys really. Just thought I mentioned it.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel