| Message ID | 1470221788-30808-2-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma <shashank.sharma@intel.com> wrote: > This patch adds drm flag bits for aspect ratio information > > Currently drm flag bits don't have field for mode's picture > aspect ratio. This field will help the driver to pick mode with > right aspect ratio, and help in setting right VIC field in avi > infoframes. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > include/uapi/drm/drm_mode.h | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 49a7265..cd66a95 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -77,6 +77,19 @@ extern "C" { > #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14) > #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF (8<<14) > > +/* Picture aspect ratio options */ > +#define DRM_MODE_PICTURE_ASPECT_NONE 0 > +#define DRM_MODE_PICTURE_ASPECT_4_3 1 > +#define DRM_MODE_PICTURE_ASPECT_16_9 2 > + > +/* Aspect ratio flag bitmask (4 bits 22:19) */ > +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) > +#define DRM_MODE_FLAG_PARNONE \ > + (DRM_MODE_PICTURE_ASPECT_NONE << 19) While I prefer the spaces in between the << operator, it seems like convention in this file is to not have them. > +#define DRM_MODE_FLAG_PAR4_3 \ > + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) > +#define DRM_MODE_FLAG_PAR16_9 \ > + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) Not crazy about "PAR". Perhaps DRM_MODE_FLAG_ASPECT_BLAH would be more descriptive? > > /* DPMS flags */ > /* bit compatible with the xorg definitions. */ > @@ -92,11 +105,6 @@ extern "C" { > #define DRM_MODE_SCALE_CENTER 2 /* Centered, no scaling */ > #define DRM_MODE_SCALE_ASPECT 3 /* Full screen, preserve aspect */ > > -/* Picture aspect ratio options */ > -#define DRM_MODE_PICTURE_ASPECT_NONE 0 > -#define DRM_MODE_PICTURE_ASPECT_4_3 1 > -#define DRM_MODE_PICTURE_ASPECT_16_9 2 > - > /* Dithering mode options */ > #define DRM_MODE_DITHERING_OFF 0 > #define DRM_MODE_DITHERING_ON 1 > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Thanks for the review, Sean. My comments, inline. Regards Shashank On 8/3/2016 11:10 PM, Sean Paul wrote: > On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma > <shashank.sharma@intel.com> wrote: >> This patch adds drm flag bits for aspect ratio information >> >> Currently drm flag bits don't have field for mode's picture >> aspect ratio. This field will help the driver to pick mode with >> right aspect ratio, and help in setting right VIC field in avi >> infoframes. >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> include/uapi/drm/drm_mode.h | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >> index 49a7265..cd66a95 100644 >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -77,6 +77,19 @@ extern "C" { >> #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14) >> #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF (8<<14) >> >> +/* Picture aspect ratio options */ >> +#define DRM_MODE_PICTURE_ASPECT_NONE 0 >> +#define DRM_MODE_PICTURE_ASPECT_4_3 1 >> +#define DRM_MODE_PICTURE_ASPECT_16_9 2 >> + >> +/* Aspect ratio flag bitmask (4 bits 22:19) */ >> +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) >> +#define DRM_MODE_FLAG_PARNONE \ >> + (DRM_MODE_PICTURE_ASPECT_NONE << 19) > While I prefer the spaces in between the << operator, it seems like > convention in this file is to not have them. I agree, I will remove the spaces. > >> +#define DRM_MODE_FLAG_PAR4_3 \ >> + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) >> +#define DRM_MODE_FLAG_PAR16_9 \ >> + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) > Not crazy about "PAR". Perhaps DRM_MODE_FLAG_ASPECT_BLAH would be more > descriptive? Ok, let me come up with something which rhymes with BLAH :) Regards Shashank >> /* DPMS flags */ >> /* bit compatible with the xorg definitions. */ >> @@ -92,11 +105,6 @@ extern "C" { >> #define DRM_MODE_SCALE_CENTER 2 /* Centered, no scaling */ >> #define DRM_MODE_SCALE_ASPECT 3 /* Full screen, preserve aspect */ >> >> -/* Picture aspect ratio options */ >> -#define DRM_MODE_PICTURE_ASPECT_NONE 0 >> -#define DRM_MODE_PICTURE_ASPECT_4_3 1 >> -#define DRM_MODE_PICTURE_ASPECT_16_9 2 >> - >> /* Dithering mode options */ >> #define DRM_MODE_DITHERING_OFF 0 >> #define DRM_MODE_DITHERING_ON 1 >> -- >> 1.9.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote: > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > + > +/* Aspect ratio flag bitmask (4 bits 22:19) */ > +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) > +#define DRM_MODE_FLAG_PARNONE \ > + (DRM_MODE_PICTURE_ASPECT_NONE << 19) > +#define DRM_MODE_FLAG_PAR4_3 \ > + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) > +#define DRM_MODE_FLAG_PAR16_9 \ > + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) > Afaict all these flags are internal/implementation specific thus we shouldn't expose them to userspace. Right ? -Emil
Hello Emil, Thanks for your time. I have got mixed opinion on this. IMHO we should expose them to userspace too, as UI agents like Hardware composer/X/Wayland must know what does these flags means, so that they can display them on the end user screen (like settings menu) But few people even think thats its too complex to be exposed to userspace agents. So your suggestions are welcome. Regards Shashank On 8/4/2016 3:12 PM, Emil Velikov wrote: > On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote: > >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> + >> +/* Aspect ratio flag bitmask (4 bits 22:19) */ >> +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) >> +#define DRM_MODE_FLAG_PARNONE \ >> + (DRM_MODE_PICTURE_ASPECT_NONE << 19) >> +#define DRM_MODE_FLAG_PAR4_3 \ >> + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) >> +#define DRM_MODE_FLAG_PAR16_9 \ >> + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) >> > Afaict all these flags are internal/implementation specific thus we > shouldn't expose them to userspace. Right ? > > -Emil
On Thu, Aug 04, 2016 at 03:46:09PM +0530, Sharma, Shashank wrote: > Hello Emil, > > Thanks for your time. > > I have got mixed opinion on this. > > IMHO we should expose them to userspace too, as UI agents like Hardware > composer/X/Wayland must know what does these > > flags means, so that they can display them on the end user screen (like > settings menu) > > But few people even think thats its too complex to be exposed to userspace > agents. > > So your suggestions are welcome. These are flags for the internal mode representation, not for the uapi one. They really don't belong into the uapi header, but instead into include/drm/drm_modes.h. I.e. I fully concur with Emil. -Daniel > > Regards > Shashank > > On 8/4/2016 3:12 PM, Emil Velikov wrote: > > On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote: > > > > > --- a/include/uapi/drm/drm_mode.h > > > +++ b/include/uapi/drm/drm_mode.h > > > + > > > +/* Aspect ratio flag bitmask (4 bits 22:19) */ > > > +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) > > > +#define DRM_MODE_FLAG_PARNONE \ > > > + (DRM_MODE_PICTURE_ASPECT_NONE << 19) > > > +#define DRM_MODE_FLAG_PAR4_3 \ > > > + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) > > > +#define DRM_MODE_FLAG_PAR16_9 \ > > > + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) > > > > > Afaict all these flags are internal/implementation specific thus we > > shouldn't expose them to userspace. Right ? > > > > -Emil > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> wrote: > Hello Emil, > > Thanks for your time. > > I have got mixed opinion on this. > > IMHO we should expose them to userspace too, as UI agents like Hardware > composer/X/Wayland must know what does these > > flags means, so that they can display them on the end user screen (like > settings menu) > > But few people even think thats its too complex to be exposed to userspace > agents. > If we want these/such flags passed between kernel and user space one must: - Provide a kernel interface how to do that - Provide a userspace (acked by respective developers/maintainers) that the approach is sane and proves useful. Since the above can take some time, I'd suggest dropping those from the UAPI header(s)... for now. -Emil
Thanks Daniel. My comments, inline. Regards Shashank On 8/4/2016 4:06 PM, Daniel Vetter wrote: > On Thu, Aug 04, 2016 at 03:46:09PM +0530, Sharma, Shashank wrote: >> Hello Emil, >> >> Thanks for your time. >> >> I have got mixed opinion on this. >> >> IMHO we should expose them to userspace too, as UI agents like Hardware >> composer/X/Wayland must know what does these >> >> flags means, so that they can display them on the end user screen (like >> settings menu) >> >> But few people even think thats its too complex to be exposed to userspace >> agents. >> >> So your suggestions are welcome. > These are flags for the internal mode representation, not for the uapi > one. They really don't belong into the uapi header, but instead into > include/drm/drm_modes.h. I.e. I fully concur with Emil. > -Daniel Please correct me if I am missing anything, but In that case, how will HWC/X apply a modeset 1920x1080@60 16:9 (Not 1920x1080@60 4:3) Regards Shashank >> Regards >> Shashank >> >> On 8/4/2016 3:12 PM, Emil Velikov wrote: >>> On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote: >>> >>>> --- a/include/uapi/drm/drm_mode.h >>>> +++ b/include/uapi/drm/drm_mode.h >>>> + >>>> +/* Aspect ratio flag bitmask (4 bits 22:19) */ >>>> +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) >>>> +#define DRM_MODE_FLAG_PARNONE \ >>>> + (DRM_MODE_PICTURE_ASPECT_NONE << 19) >>>> +#define DRM_MODE_FLAG_PAR4_3 \ >>>> + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) >>>> +#define DRM_MODE_FLAG_PAR16_9 \ >>>> + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) >>>> >>> Afaict all these flags are internal/implementation specific thus we >>> shouldn't expose them to userspace. Right ? >>> >>> -Emil >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Regards Shashank On 8/4/2016 5:04 PM, Emil Velikov wrote: > On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> wrote: >> Hello Emil, >> >> Thanks for your time. >> >> I have got mixed opinion on this. >> >> IMHO we should expose them to userspace too, as UI agents like Hardware >> composer/X/Wayland must know what does these >> >> flags means, so that they can display them on the end user screen (like >> settings menu) >> >> But few people even think thats its too complex to be exposed to userspace >> agents. >> > If we want these/such flags passed between kernel and user space one must: > - Provide a kernel interface how to do that > - Provide a userspace (acked by respective developers/maintainers) > that the approach is sane and proves useful. > > Since the above can take some time, I'd suggest dropping those from > the UAPI header(s)... for now. > > -Emil Please guide me a bit more on this problem, Emil, Daniel. The reason why I want to pass this to userspace is, so that, HWC/X/any other UI manager, can send a modeset which is accurate upto aspect ratio. Please think of this complete scenario: - HDMI compliance testing is going on, and we need to apply a CEA mode (for example, VIC=3, mode - 720x480@60Hz aspect 16:9) - But HWC doesnt pass the aspect ratio information, and passed only 720x480@60Hz - Kernel gets the first matching mode, from edid_cea_modes, which is VIC=2 720x480@60Hz (aspect 4:3) - Kernel does the modeset of VIC = 2, and AVI IF contain VIC=2 - Compliance equipment expects VIC = 3, so fails the test case. In this case, its essential that the userspace, which is triggering the modeset, must know about the aspect ratio field too. So does this make it a reason to pass this information to usp ? Regards Shashank
On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote: > On 8/4/2016 5:04 PM, Emil Velikov wrote: >> >> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> >> wrote: >>> >>> Hello Emil, >>> >>> Thanks for your time. >>> >>> I have got mixed opinion on this. >>> >>> IMHO we should expose them to userspace too, as UI agents like Hardware >>> composer/X/Wayland must know what does these >>> >>> flags means, so that they can display them on the end user screen (like >>> settings menu) >>> >>> But few people even think thats its too complex to be exposed to >>> userspace >>> agents. >>> >> If we want these/such flags passed between kernel and user space one must: >> - Provide a kernel interface how to do that >> - Provide a userspace (acked by respective developers/maintainers) >> that the approach is sane and proves useful. >> >> Since the above can take some time, I'd suggest dropping those from >> the UAPI header(s)... for now. >> >> -Emil > > Please guide me a bit more on this problem, Emil, Daniel. > The reason why I want to pass this to userspace is, so that, HWC/X/any other > UI manager, can send a modeset > which is accurate upto aspect ratio. > Nobody(?) is arguing that you don't to pass such information to/from userspace :-) Your series does the internal parsing/management of the attribute and has no actual UAPI implementation and/or userspace references (to code/discussions). Thus the UAPI changes should _not_ be part of these patches. Daniel's blog [1] has many educational materials why and how things are done upstream. I would kindly invite you to give them (another?) courtesy read. Regards, Emil [1] http://blog.ffwll.ch/
On Thu, Aug 04, 2016 at 06:35:52PM +0530, Sharma, Shashank wrote: > Thanks Daniel. > > My comments, inline. > > Regards > Shashank > On 8/4/2016 4:06 PM, Daniel Vetter wrote: > > On Thu, Aug 04, 2016 at 03:46:09PM +0530, Sharma, Shashank wrote: > > > Hello Emil, > > > > > > Thanks for your time. > > > > > > I have got mixed opinion on this. > > > > > > IMHO we should expose them to userspace too, as UI agents like Hardware > > > composer/X/Wayland must know what does these > > > > > > flags means, so that they can display them on the end user screen (like > > > settings menu) > > > > > > But few people even think thats its too complex to be exposed to userspace > > > agents. > > > > > > So your suggestions are welcome. > > These are flags for the internal mode representation, not for the uapi > > one. They really don't belong into the uapi header, but instead into > > include/drm/drm_modes.h. I.e. I fully concur with Emil. > > -Daniel > Please correct me if I am missing anything, but In that case, how will HWC/X > apply a modeset > 1920x1080@60 16:9 (Not 1920x1080@60 4:3) Argh sorry, I mixed up the two mode structures again. You're correct. -Daniel > > Regards > Shashank > > > Regards > > > Shashank > > > > > > On 8/4/2016 3:12 PM, Emil Velikov wrote: > > > > On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote: > > > > > > > > > --- a/include/uapi/drm/drm_mode.h > > > > > +++ b/include/uapi/drm/drm_mode.h > > > > > + > > > > > +/* Aspect ratio flag bitmask (4 bits 22:19) */ > > > > > +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) > > > > > +#define DRM_MODE_FLAG_PARNONE \ > > > > > + (DRM_MODE_PICTURE_ASPECT_NONE << 19) > > > > > +#define DRM_MODE_FLAG_PAR4_3 \ > > > > > + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) > > > > > +#define DRM_MODE_FLAG_PAR16_9 \ > > > > > + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) > > > > > > > > > Afaict all these flags are internal/implementation specific thus we > > > > shouldn't expose them to userspace. Right ? > > > > > > > > -Emil > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote: > On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote: > > On 8/4/2016 5:04 PM, Emil Velikov wrote: > >> > >> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> > >> wrote: > >>> > >>> Hello Emil, > >>> > >>> Thanks for your time. > >>> > >>> I have got mixed opinion on this. > >>> > >>> IMHO we should expose them to userspace too, as UI agents like Hardware > >>> composer/X/Wayland must know what does these > >>> > >>> flags means, so that they can display them on the end user screen (like > >>> settings menu) > >>> > >>> But few people even think thats its too complex to be exposed to > >>> userspace > >>> agents. > >>> > >> If we want these/such flags passed between kernel and user space one must: > >> - Provide a kernel interface how to do that > >> - Provide a userspace (acked by respective developers/maintainers) > >> that the approach is sane and proves useful. > >> > >> Since the above can take some time, I'd suggest dropping those from > >> the UAPI header(s)... for now. > >> > >> -Emil > > > > Please guide me a bit more on this problem, Emil, Daniel. > > The reason why I want to pass this to userspace is, so that, HWC/X/any other > > UI manager, can send a modeset > > which is accurate upto aspect ratio. > > > Nobody(?) is arguing that you don't to pass such information to/from > userspace :-) > Your series does the internal parsing/management of the attribute and > has no actual UAPI implementation and/or userspace references (to > code/discussions). Thus the UAPI changes should _not_ be part of these > patches. > > Daniel's blog [1] has many educational materials why and how things > are done upstream. I would kindly invite you to give them (another?) > courtesy read. It reuses the already existing uapi mode structure. And since it extends them both on the probe side and on the modeset set this means userspace can just pass through the right probed mode and it'll all magically work. At least that's the idea. Now if you actually care about the different bits then you can select the right mode, but that's about it. So if you want to compensate for the non-square pixels in rendering then you need to look at it, but otherwise it's just a case of select the mode you want. I don't see what new userspace we'd need for that really, current one should all work nicely as-is. At least the entire point of doing this was seamless support with existing userspace. -Daniel
Regards Shashank On 8/4/2016 9:39 PM, Daniel Vetter wrote: > On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote: >> On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote: >>> On 8/4/2016 5:04 PM, Emil Velikov wrote: >>>> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> >>>> wrote: >>>>> Hello Emil, >>>>> >>>>> Thanks for your time. >>>>> >>>>> I have got mixed opinion on this. >>>>> >>>>> IMHO we should expose them to userspace too, as UI agents like Hardware >>>>> composer/X/Wayland must know what does these >>>>> >>>>> flags means, so that they can display them on the end user screen (like >>>>> settings menu) >>>>> >>>>> But few people even think thats its too complex to be exposed to >>>>> userspace >>>>> agents. >>>>> >>>> If we want these/such flags passed between kernel and user space one must: >>>> - Provide a kernel interface how to do that >>>> - Provide a userspace (acked by respective developers/maintainers) >>>> that the approach is sane and proves useful. >>>> >>>> Since the above can take some time, I'd suggest dropping those from >>>> the UAPI header(s)... for now. >>>> >>>> -Emil >>> Please guide me a bit more on this problem, Emil, Daniel. >>> The reason why I want to pass this to userspace is, so that, HWC/X/any other >>> UI manager, can send a modeset >>> which is accurate upto aspect ratio. >>> >> Nobody(?) is arguing that you don't to pass such information to/from >> userspace :-) >> Your series does the internal parsing/management of the attribute and >> has no actual UAPI implementation and/or userspace references (to >> code/discussions). Thus the UAPI changes should _not_ be part of these >> patches. >> >> Daniel's blog [1] has many educational materials why and how things >> are done upstream. I would kindly invite you to give them (another?) >> courtesy read. > It reuses the already existing uapi mode structure. And since it extends > them both on the probe side and on the modeset set this means userspace > can just pass through the right probed mode and it'll all magically work. > At least that's the idea. > > Now if you actually care about the different bits then you can select the > right mode, but that's about it. So if you want to compensate for the > non-square pixels in rendering then you need to look at it, but otherwise > it's just a case of select the mode you want. I don't see what new > userspace we'd need for that really, current one should all work nicely > as-is. At least the entire point of doing this was seamless support with > existing userspace. > -Daniel Thanks Daniel, you explained the zest of this series better than me :) Regards Shashank
Hi Sharma, On 05-08-2016 04:37, Sharma, Shashank wrote: > Regards > > Shashank > > > On 8/4/2016 9:39 PM, Daniel Vetter wrote: >> On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote: >>> On 4 August 2016 at 14:15, Sharma, Shashank >>> <shashank.sharma@intel.com> wrote: >>>> On 8/4/2016 5:04 PM, Emil Velikov wrote: >>>>> On 4 August 2016 at 11:16, Sharma, Shashank >>>>> <shashank.sharma@intel.com> >>>>> wrote: >>>>>> Hello Emil, >>>>>> >>>>>> Thanks for your time. >>>>>> >>>>>> I have got mixed opinion on this. >>>>>> >>>>>> IMHO we should expose them to userspace too, as UI agents >>>>>> like Hardware >>>>>> composer/X/Wayland must know what does these >>>>>> >>>>>> flags means, so that they can display them on the end user >>>>>> screen (like >>>>>> settings menu) >>>>>> >>>>>> But few people even think thats its too complex to be >>>>>> exposed to >>>>>> userspace >>>>>> agents. >>>>>> >>>>> If we want these/such flags passed between kernel and user >>>>> space one must: >>>>> - Provide a kernel interface how to do that >>>>> - Provide a userspace (acked by respective >>>>> developers/maintainers) >>>>> that the approach is sane and proves useful. >>>>> >>>>> Since the above can take some time, I'd suggest dropping >>>>> those from >>>>> the UAPI header(s)... for now. >>>>> >>>>> -Emil >>>> Please guide me a bit more on this problem, Emil, Daniel. >>>> The reason why I want to pass this to userspace is, so that, >>>> HWC/X/any other >>>> UI manager, can send a modeset >>>> which is accurate upto aspect ratio. >>>> >>> Nobody(?) is arguing that you don't to pass such information >>> to/from >>> userspace :-) >>> Your series does the internal parsing/management of the >>> attribute and >>> has no actual UAPI implementation and/or userspace references >>> (to >>> code/discussions). Thus the UAPI changes should _not_ be part >>> of these >>> patches. >>> >>> Daniel's blog [1] has many educational materials why and how >>> things >>> are done upstream. I would kindly invite you to give them >>> (another?) >>> courtesy read. >> It reuses the already existing uapi mode structure. And since >> it extends >> them both on the probe side and on the modeset set this means >> userspace >> can just pass through the right probed mode and it'll all >> magically work. >> At least that's the idea. >> >> Now if you actually care about the different bits then you can >> select the >> right mode, but that's about it. So if you want to compensate >> for the >> non-square pixels in rendering then you need to look at it, >> but otherwise >> it's just a case of select the mode you want. I don't see what >> new >> userspace we'd need for that really, current one should all >> work nicely >> as-is. At least the entire point of doing this was seamless >> support with >> existing userspace. >> -Daniel > Thanks Daniel, you explained the zest of this series better > than me :) > > Regards > Shashank > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel Will you send a new version of these patches? I have a patch ready that adds the new HDMI 2.0 modes to the CEA modes list in DRM but these modes require the addition of the new picture aspect ratio flags (64:27, 256:135). I can either wait that your patches get accepted or I can add to my patch set one that adds the new PAR flags. Best regards, Jose Miguel Abreu
On Tue, Aug 9, 2016 at 3:12 PM, Jose Abreu <Jose.Abreu@synopsys.com> wrote: > Will you send a new version of these patches? I have a patch > ready that adds the new HDMI 2.0 modes to the CEA modes list in > DRM but these modes require the addition of the new picture > aspect ratio flags (64:27, 256:135). I can either wait that your > patches get accepted or I can add to my patch set one that adds > the new PAR flags. Please base your work on top of Sharma's entire patch series - I suspect that Sharma's patch series already covers all you really need. With the exception of then correctly encoding the chosen aspect ratio into the infoframes. -Daniel
Thanks Daniel. Hi Joes, As Daniel mentioned, I have already written a patch which adds all HDMI 2.0 modes in CEA DB, and it also completes all the VICs in the DB from 1-107 (we have 1-64 now). I was myself waiting for aspect ratio patches to get accepted, as I needed them for the modes. Will keep you in CC. I am planning to send a new patch set by tonight. Regards Shashank -----Original Message----- From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, August 9, 2016 7:14 PM To: Jose Abreu <Jose.Abreu@synopsys.com> Cc: Sharma, Shashank <shashank.sharma@intel.com>; Emil Velikov <emil.l.velikov@gmail.com>; Vetter, Daniel <daniel.vetter@intel.com>; intel-gfx@lists.freedesktop.org; ML dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH 1/4] drm: add picture aspect ratio flags On Tue, Aug 9, 2016 at 3:12 PM, Jose Abreu <Jose.Abreu@synopsys.com> wrote: > Will you send a new version of these patches? I have a patch ready > that adds the new HDMI 2.0 modes to the CEA modes list in DRM but > these modes require the addition of the new picture aspect ratio flags > (64:27, 256:135). I can either wait that your patches get accepted or > I can add to my patch set one that adds the new PAR flags. Please base your work on top of Sharma's entire patch series - I suspect that Sharma's patch series already covers all you really need. With the exception of then correctly encoding the chosen aspect ratio into the infoframes. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On 4 August 2016 at 17:09, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote: >> On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote: >> > On 8/4/2016 5:04 PM, Emil Velikov wrote: >> >> >> >> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> >> >> wrote: >> >>> >> >>> Hello Emil, >> >>> >> >>> Thanks for your time. >> >>> >> >>> I have got mixed opinion on this. >> >>> >> >>> IMHO we should expose them to userspace too, as UI agents like Hardware >> >>> composer/X/Wayland must know what does these >> >>> >> >>> flags means, so that they can display them on the end user screen (like >> >>> settings menu) >> >>> >> >>> But few people even think thats its too complex to be exposed to >> >>> userspace >> >>> agents. >> >>> >> >> If we want these/such flags passed between kernel and user space one must: >> >> - Provide a kernel interface how to do that >> >> - Provide a userspace (acked by respective developers/maintainers) >> >> that the approach is sane and proves useful. >> >> >> >> Since the above can take some time, I'd suggest dropping those from >> >> the UAPI header(s)... for now. >> >> >> >> -Emil >> > >> > Please guide me a bit more on this problem, Emil, Daniel. >> > The reason why I want to pass this to userspace is, so that, HWC/X/any other >> > UI manager, can send a modeset >> > which is accurate upto aspect ratio. >> > >> Nobody(?) is arguing that you don't to pass such information to/from >> userspace :-) >> Your series does the internal parsing/management of the attribute and >> has no actual UAPI implementation and/or userspace references (to >> code/discussions). Thus the UAPI changes should _not_ be part of these >> patches. >> >> Daniel's blog [1] has many educational materials why and how things >> are done upstream. I would kindly invite you to give them (another?) >> courtesy read. > > It reuses the already existing uapi mode structure. And since it extends > them both on the probe side and on the modeset set this means userspace > can just pass through the right probed mode and it'll all magically work. > At least that's the idea. > > Now if you actually care about the different bits then you can select the > right mode, but that's about it. So if you want to compensate for the > non-square pixels in rendering then you need to look at it, but otherwise > it's just a case of select the mode you want. I don't see what new > userspace we'd need for that really, current one should all work nicely > as-is. At least the entire point of doing this was seamless support with > existing userspace. I failed to click that drm_mode_convert_umode does input validation, apart from the actual conversion. Thanks a lot gents and sorry for the noise. Emil
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 49a7265..cd66a95 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -77,6 +77,19 @@ extern "C" { #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14) #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF (8<<14) +/* Picture aspect ratio options */ +#define DRM_MODE_PICTURE_ASPECT_NONE 0 +#define DRM_MODE_PICTURE_ASPECT_4_3 1 +#define DRM_MODE_PICTURE_ASPECT_16_9 2 + +/* Aspect ratio flag bitmask (4 bits 22:19) */ +#define DRM_MODE_FLAG_PARMASK (0x0F<<19) +#define DRM_MODE_FLAG_PARNONE \ + (DRM_MODE_PICTURE_ASPECT_NONE << 19) +#define DRM_MODE_FLAG_PAR4_3 \ + (DRM_MODE_PICTURE_ASPECT_4_3 << 19) +#define DRM_MODE_FLAG_PAR16_9 \ + (DRM_MODE_PICTURE_ASPECT_16_9 << 19) /* DPMS flags */ /* bit compatible with the xorg definitions. */ @@ -92,11 +105,6 @@ extern "C" { #define DRM_MODE_SCALE_CENTER 2 /* Centered, no scaling */ #define DRM_MODE_SCALE_ASPECT 3 /* Full screen, preserve aspect */ -/* Picture aspect ratio options */ -#define DRM_MODE_PICTURE_ASPECT_NONE 0 -#define DRM_MODE_PICTURE_ASPECT_4_3 1 -#define DRM_MODE_PICTURE_ASPECT_16_9 2 - /* Dithering mode options */ #define DRM_MODE_DITHERING_OFF 0 #define DRM_MODE_DITHERING_ON 1
This patch adds drm flag bits for aspect ratio information Currently drm flag bits don't have field for mode's picture aspect ratio. This field will help the driver to pick mode with right aspect ratio, and help in setting right VIC field in avi infoframes. Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- include/uapi/drm/drm_mode.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)