Re: [PATCH 2/5] drm/omap: check that plane is inside crtc

From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/omap: check that plane is inside crtc
Date: Tue, 23 Jun 2015 11:01:24 +0300	[thread overview]
Message-ID: <55891254.6060404@ti.com> (raw)
In-Reply-To: <3022545.7m5qkGr2vj@avalon>

[-- Attachment #1.1: Type: text/plain, Size: 2443 bytes --]

On 18/06/15 17:45, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thursday 18 June 2015 13:10:36 Tomi Valkeinen wrote:
>> DRM allows planes to be partially off-screen, but DSS hardware does not.
>> This patch adds the necessary check to reject plane configs if the plane
>> is not fully inside the crtc.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_plane.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
>> b/drivers/gpu/drm/omapdrm/omap_plane.c index cfa8276c4deb..53594a3b4e98
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -17,6 +17,7 @@
>>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>>   */
>>
>> +#include <drm/drm_atomic.h>
>>  #include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_plane_helper.h>
>>
>> @@ -153,9 +154,34 @@ static void omap_plane_atomic_disable(struct drm_plane
>> *plane, dispc_ovl_enable(omap_plane->id, false);
>>  }
>>
>> +static int omap_plane_atomic_check(struct drm_plane *plane,
>> +				   struct drm_plane_state *state)
>> +{
>> +	struct drm_crtc_state *crtc_state;
>> +
>> +	if (!state->crtc)
>> +		return 0;
>> +
>> +	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
>> +	if (!crtc_state)
>> +		return 0;
> 
> drm_atomic_get_crtc_state() returns an ERR_PTR on error. You should then 
> propagate the error to the caller:
> 
> 	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> 	if (IS_ERR(crtc_state))
> 		return PTR_ERR(crtc_state);

Thanks, I missed that. I've made the change.

>> +	if (state->crtc_x < 0 || state->crtc_y < 0)
>> +		return -EINVAL;
>> +
>> +	if (state->crtc_x + state->crtc_w > crtc_state->adjusted_mode.hdisplay)
>> +		return -EINVAL;
>> +
>> +	if (state->crtc_y + state->crtc_h > crtc_state->adjusted_mode.vdisplay)
>> +		return -EINVAL;
> 
> I wonder whether we couldn't clip the plane in software instead of failing. 
> This patch is fine though (except for the problem above), clipping can be 
> implemented in a separate patch.

I had the same thought, but I didn't want to go that way yet. I have a
feeling that it could have corner cases that need to be taken care of,
and I just wanted to fix the current behavior.

 Tomi

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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