Hello Marek,
first of all thanks for checking this!
Marek Szyprowski wrote:
> Hi Tobias,
>
> On 2017-08-22 16:19, Tobias Jakobi wrote:
>> In some of subdrivers we compute something like 'pitch / cpp' at some
>> point, silently assuming that the pitch (which is in bytes) is
>> divisible by the buffer's cpp. This must not be true, in particular
>> it depends on the underlying hardware.
>>
>> If userspace should request such a setup, we should communicate this.
>>
>> Introduce a new cap which indicates that the hardware supports a
>> pitch with 'byte-granularity'. If the cap is not set, assume that
>> we need a pitch aligned to cpp.
>>
>> We set this cap in a later patch for the drivers/planes which
>> support it.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
>
> I briefly checked the patch and it looks fine, but I really wonder whether
> drivers should support such strange formats, when pitches are not multiple
> of pixel size in bytes. I cannot find any sane use case for such formats.
Apparantly the hw can do it, so why not support it? A potential use case I see
would be an application that uses a single buffer with multiple pixelformats.
Let buffer-size = width * height * sizeof(uint16_t), with width odd. Then the
buffer pitch is not divisible by 4. In case the hw supports it, we can still use
e.g. XRGB8888 with this setup.
> Maybe it would make sense to add a check for it in DRM core? I also didn't
> notice a check for that in any other drivers, but some of them also
> compute 'pitch / cpp' values.
I thought about some check in DRM core, but I didn't see a clean way to add it,
since the behaviour very much depends on the hw. Well, except if you just want
to enforce 'pitch % cpp == 0' everywhere. Not sure how the rest of the DRM folks
thinks about it.
With best wishes,
Tobias
>> ---
>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 +
>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 10 ++++++++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> index 43afab4bebc3..ec32632485d2 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> @@ -92,6 +92,7 @@ struct exynos_drm_plane {
>> #define EXYNOS_DRM_PLANE_CAP_SCALE (1 << 1)
>> #define EXYNOS_DRM_PLANE_CAP_ZPOS (1 << 2)
>> #define EXYNOS_DRM_PLANE_CAP_TILE (1 << 3)
>> +#define EXYNOS_DRM_PLANE_CAP_BYTE_PITCH (1 << 4)
>> /*
>> * Exynos DRM plane configuration structure.
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index 133af72f5c90..17e47b8f0d6a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -185,6 +185,16 @@ exynos_drm_plane_check_format(const struct
>> exynos_drm_plane_config *config,
>> {
>> struct drm_framebuffer *fb = state->base.fb;
>> + /*
>> + * Some blocks only allow to specify a buffer pitch in terms
>> + * of pixels. In these cases, we need to ensure that the pitch
>> + * provided by userspace is divisible by the cpp.
>> + */
>> + if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_BYTE_PITCH)) {
>> + if (fb->pitches[0] % fb->format->cpp[0])
>> + return -ENOTSUPP;
>> + }
>> +
>> switch (fb->modifier) {
>> case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
>> if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_TILE))
>
> Best regards
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
