Re: [PATCH 06/21] drm/omap: check CRTC color format earlier

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

On 27/02/15 14:07, Daniel Vetter wrote:
> On Thu, Feb 26, 2015 at 03:20:14PM +0200, Tomi Valkeinen wrote:
>> When setting a color format to a DRM plane, the DRM core checks whether
>> the format is supported by the HW. However, it seems that when setting
>> the color format of a CRTC (i.e. a root plane), there's no checking
>> done. This causes omapdrm to configure omapdss with the bad color
>> format, which omapdss then rejects.
>>
>> While the above is ok, the error message is a bit confusing, and the
>> configuring of omapdss happens asynchronously from the ioctl that does
>> the color format change.
>>
>> This patch adds a color format check to omap_plane_mode_set() which
>> causes the ioctl to return an error for invalid color format. This means
>> that the userspace will get to know of the wrong setting, instead of the
>> current behavior where the error is not seen by the userspace.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index 1f4f2b866379..bedd6f7af0f1 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane,
>>  {
>>  	struct omap_plane *omap_plane = to_omap_plane(plane);
>>  	struct omap_drm_window *win = &omap_plane->win;
>> +	int i;
>> +
>> +	/*
>> +	 * Check whether this plane supports the fb pixel format.
>> +	 * I don't think this should really be needed, but it looks like the
>> +	 * drm core only checks the format for planes, not for the crtc. So
>> +	 * when setting the format for crtc, without this check we would
>> +	 * get an error later when trying to program the color format into the
>> +	 * HW.
>> +	 */
> 
> I think we should add a format check to the setcrtc ioctl if crtc->primary
> is set. Atm the code is in __setplane_internal but could easily be shared
> - there's already a copy in drm_atomi.c.
> 
> Omapdrm wouldn't benefit from this yet since it doesn't support universal
> planes. But adding universal planes should be on your todo anyway ;-)

Laurent is working on universal planes and atomic modesetting for
omapdrm. In fact, I think universal planes is already done in his WIP
branch.

So, if this check is already done with universal planes (if I understood
correctly), I'm fine with dropping this patch.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]