Re: [PATCH 1/2] drm/exynos: mixer: cleanup pixelformat handling

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

On 2015-04-27 08:33, Joonyoung Shim wrote:
> Hi Tobias,
> 
> On 04/24/2015 05:10 PM, Tobias Jakobi wrote:
>> On 2015-04-24 03:48, Joonyoung Shim wrote:
>>> Hi Tobias,
>>> 
>>> On 04/23/2015 09:12 PM, Tobias Jakobi wrote:
>>>> Move the defines for the pixelformats that the mixer supports out
>>>> of mixer_graph_buffer() to the top of the source.
>>>> Then select the mixer pixelformat (pf) in mixer_graph_buffer() based 
>>>> on
>>>> the plane's pf (and not bpp).
>>>> Also add handling of RGB565 and XRGB1555 to the switch statement and
>>>> exit early if the plane has an unsupported pf.
>>>> 
>>>> Partially based on 'drm/exynos: enable/disable blend based on pixel
>>>> format' by Gustavo Padovan <gustavo.padovan@collabora.co.uk>.
>>>> 
>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 32 
>>>> ++++++++++++++++++++++----------
>>>>  1 file changed, 22 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
>>>> b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> index fbec750..1bd23ee 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> @@ -44,6 +44,12 @@
>>>>  #define MIXER_WIN_NR        3
>>>>  #define MIXER_DEFAULT_WIN    0
>>>> 
>>>> +/* The pixelformats that are natively supported by the mixer. */
>>>> +#define MIXER_PIXELFORMAT_RGB565 4
>>>> +#define MIXER_PIXELFORMAT_ARGB1555 5
>>>> +#define MIXER_PIXELFORMAT_ARGB4444 6
>>>> +#define MIXER_PIXELFORMAT_ARGB8888 7
>>>> +
>>> 
>>> Seems be long, how about s/PIXELFORMAT/FORMAT or just MIXER_RGB565 
>>> ... ?
>>> Please use tab between define name and value.
>> How about MXR_FORMAT_XYZ, to stay consistent with the regs-mixer 
>> header?
>> 
>> 
>>>>  struct mixer_resources {
>>>>      int            irq;
>>>>      void __iomem        *mixer_regs;
>>>> @@ -531,20 +537,26 @@ static void mixer_graph_buffer(struct 
>>>> mixer_context *ctx, int win)
>>>> 
>>>>      plane = &ctx->planes[win];
>>>> 
>>>> -    #define RGB565 4
>>>> -    #define ARGB1555 5
>>>> -    #define ARGB4444 6
>>>> -    #define ARGB8888 7
>>>> +    switch (plane->pixel_format) {
>>>> +    case DRM_FORMAT_XRGB4444:
>>>> +        fmt = MIXER_PIXELFORMAT_ARGB4444;
>>>> +        break;
>>>> +
>>>> +    case DRM_FORMAT_XRGB1555:
>>>> +        fmt = MIXER_PIXELFORMAT_ARGB1555;
>>>> +        break;
>>>> 
>>>> -    switch (plane->bpp) {
>>>> -    case 16:
>>>> -        fmt = ARGB4444;
>>>> +    case DRM_FORMAT_RGB565:
>>>> +        fmt = MIXER_PIXELFORMAT_RGB565;
>>>>          break;
>>>> -    case 32:
>>>> -        fmt = ARGB8888;
>>>> +
>>>> +    case DRM_FORMAT_XRGB8888:
>>>> +        fmt = MIXER_PIXELFORMAT_ARGB8888;
>>>>          break;
>>>> +
>>>>      default:
>>>> -        fmt = ARGB8888;
>>>> +        DRM_DEBUG_KMS("pixelformat unsupported by mixer\n");
>>>> +        return;
>>>>      }
>>>> 
>>>>      /* check if mixer supports requested scaling setup */
>>>> 
>>> 
>>> Hmm missing formats having alpha?
>>> DRM_FORMAT_ARGB4444
>>> DRM_FORMAT_ARGB1555
>>> DRM_FORMAT_ARGB8888
>> Should be obvious from my other mail. Blending/alpha is currently 
>> screwed up, so we shouldn't add these formats at this point. At least 
>> not before figuring out how to properly solve the issue.
>> 
> 
> It shouldn't mean to remove current supported feature(alpha pixel 
> format
> plane support of mixer driver).
Ah, I see what you mean! Going to send an updated version later this 
day.


With best wishes,
Tobias


> 
> Thanks.