Re: [PATCH RESEND v2 12/32] drm/vkms: Introduce configfs for plane color encoding

[Louis Chauvet <louis.chauvet@bootlin.com>]

>>> +
>>> +    ret = kstrtouint(page, 10, &val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /* Should be a supported value */
>>> +    if (val & ~(BIT(DRM_COLOR_YCBCR_BT601) |
>>> +            BIT(DRM_COLOR_YCBCR_BT709) |
>>> +            BIT(DRM_COLOR_YCBCR_BT2020)))
>>> +        return -EINVAL;
>>> +    /* Should at least provide one color range */
>>> +    if ((val & (BIT(DRM_COLOR_YCBCR_BT601) |
>>> +            BIT(DRM_COLOR_YCBCR_BT709) |
>>> +            BIT(DRM_COLOR_YCBCR_BT2020))) == 0)
>>> +        return -EINVAL;
>>
>> Shouldn't you check that exactly one bit is set? As in patch 9.
> 
> Because this code is wrong... the default rotation should be 
> DRM_COLOR_YCBCR_BT601 / DRM_COLOR_YCBCR_BT709 / DRM_COLOR_YCBCR_BT2020
> not a bitfield...

And after fixing this, I think I will keep bitmask with only one bit set 
so supported_color_encodings and default_color_encoding will have 
exactly the same values. Same for color ranges. Thanks for the report!

> 
>>> +
>>> +    scoped_guard(mutex, &plane->dev->lock) {
>>> +        /* Ensures that the default rotation is included in 
>>> supported rotation */
>>> +        if (plane->dev->enabled)
>>> +            return -EINVAL;
>>
>> As before, wrong comment.
>>
>> Luca
>>
>> -- 
>> Luca Ceresoli, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
>