From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jindal, Sonika" Subject: Re: [PATCH] drm: Add rotation_property to mode_config and creating it Date: Mon, 04 Aug 2014 17:34:31 +0530 Message-ID: <53DF76CF.2000102@intel.com> References: <1405413629-4266-4-git-send-email-sonika.jindal@intel.com> <1405426417-18616-1-git-send-email-sonika.jindal@intel.com> <20140728152941.GL27580@intel.com> <20140728184722.GH4747@phenom.ffwll.local> <20140729094029.GP27580@intel.com> <20140729103049.GM4747@phenom.ffwll.local> <53D9C182.3010207@intel.com> <53DF6083.5090708@intel.com> <20140804115439.GD4193@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 77F9B6E3AE for ; Mon, 4 Aug 2014 05:04:35 -0700 (PDT) In-Reply-To: <20140804115439.GD4193@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On 8/4/2014 5:24 PM, Ville Syrj=E4l=E4 wrote: > On Mon, Aug 04, 2014 at 03:59:23PM +0530, Jindal, Sonika wrote: >> >> >> On 7/31/2014 9:39 AM, Jindal, Sonika wrote: >>> >>> >>> On 7/29/2014 4:00 PM, Daniel Vetter wrote: >>>> On Tue, Jul 29, 2014 at 12:40:29PM +0300, Ville Syrj=E4l=E4 wrote: >>>>> On Mon, Jul 28, 2014 at 08:47:22PM +0200, Daniel Vetter wrote: >>>>>> On Mon, Jul 28, 2014 at 06:29:41PM +0300, Ville Syrj=E4l=E4 wrote: >>>>>>> On Tue, Jul 15, 2014 at 05:43:37PM +0530, sonika.jindal@intel.com w= rote: >>>>>>>> From: Sonika Jindal >>>>>>>> >>>>>>>> v2: Adding creation of rotation_property here. >>>>>>>> >>>>>>>> Signed-off-by: Sonika Jindal >>>>>>>> --- >>>>>>>> drivers/gpu/drm/drm_crtc.c | 3 ++- >>>>>>>> include/drm/drm_crtc.h | 1 + >>>>>>>> 2 files changed, 3 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc= .c >>>>>>>> index 787631e..49c0747 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c >>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>>>>>> @@ -1299,7 +1299,8 @@ static int drm_mode_create_standard_plane_pr= operties(struct drm_device *dev) >>>>>>>> "type", drm_plane_type_enum_list, >>>>>>>> ARRAY_SIZE(drm_plane_type_enum_list)); >>>>>>>> dev->mode_config.plane_type_property =3D type; >>>>>>>> - >>>>>>>> + dev->mode_config.rotation_property =3D drm_mode_create_rotation_= property(dev, >>>>>>>> + BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180)); >>>>>>> >>>>>>> This might not make sense for other (!i915) hardware. And that's the >>>>>>> reason why I had the driver create the property in the first place. >>>>>>> >>>>>>> I think Daniel was thinking that we might want to expose all the bi= ts >>>>>>> regardless of what the hardware supports, but I don't like that ide= a. >>>>>>> There are other properties (eg. alpha blending, csc stuff, etc.) th= at >>>>>>> have the same problem of hardware supporting only a (potentially sm= all) >>>>>>> subset of the possible values. I'd rather we didn't make life harder >>>>>>> for userspace when the kernel can already report that certain values >>>>>>> will never work. >>>>>> >>>>>> Well I'd like the property to be in some generic place so that fbcon= can >>>>>> unroate and that with the atomic stuff we can have rotation support = in the >>>>>> core structures. Which should help with argument checking. >>>>>> >>>>>> But for rotation I don't think we should set it up in generic code, = but in >>>>>> i915. So the place where we keep it would be generic, the values wou= ld be >>>>>> the sames, but the allowed set would differ depending upon platform = or >>>>>> driver. >>>>> >>>>> That would still fail if all the planes on the same device don't supp= ort >>>>> the same rotation flags. Eg. on i915 we would have this problem if we >>>>> exposed the old video overlay as a drm plane. And it wouldn't be the >>>>> first piece of hardware where I've seen this kind of thing. >>>> >>>> Problem is still that I'd like to have a somewhat generic internal >>>> representation available. We could punt this to atomic though by addin= g a >>>> rotation field to the drm_plane_state structure. Which is more-or-less= my >>>> plan, but wouldn't work with Rob's approach. >>>> >>>> Or we keep the property link only in drm_plane (and give drivers the >>>> freedom to set up the underlying enum however they want to), but I'm n= ot >>>> sure whether our interfaces can cope with that. >>>> -Daniel >>>> >>> Daniel, Ville >>> >>> So what is the suggestion for this property? Should I be moving it to >>> somewhere else? >>> >>> -Sonika >> Hi Daniel/Ville, >> >> Please let me know if I need to move this property somewhere else. > > Well we at least need to move the crationg of the property to the > driver. I guess we could leave the property itself in place in > mode_config so that fbdev can get at it easily. If we come across > a real need for separate per-plane rotation properties we can always > move it to drm_plane later. > Ok, I will move it back to plane_create function.