| Message ID | 1476705880-15600-3-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote: > Current DRM layer functions don't parse aspect ratio information > while converting a user mode->kernel mode or vice versa. This > causes modeset to pick mode with wrong aspect ratio, eventually > causing failures in HDMI compliance test cases, due to wrong VIC. > > This patch adds aspect ratio information in DRM's mode conversion > and mode comparision functions, to make sure kernel picks mode > with right aspect ratio (as per the VIC). > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Lin, Jia <lin.a.jia@intel.com> > Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> > Reviewed-by: Jim Bride <jim.bride@linux.intel.com> > Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > V2: Addressed review comments from Sean: > - Fix spellings/typo > - No need to handle aspect ratio none > - Add a break, for default case too > V3: Rebase > V4: Added r-b from Jose > --- > drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 53f07ac..fde927a 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, > mode1->vsync_end == mode2->vsync_end && > mode1->vtotal == mode2->vtotal && > mode1->vscan == mode2->vscan && > + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && > (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == > (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) > return true; > @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, > out->vrefresh = in->vrefresh; > out->flags = in->flags; > out->type = in->type; > + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > + > + switch (in->picture_aspect_ratio) { > + case HDMI_PICTURE_ASPECT_4_3: > + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; > + break; > + case HDMI_PICTURE_ASPECT_16_9: > + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; > + break; > + case HDMI_PICTURE_ASPECT_RESERVED: > + default: > + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; > + break; > + } > + > strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > } > @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out, > strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > > + /* Clearing picture aspect ratio bits from out flags */ > + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > + > + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { > + case DRM_MODE_FLAG_PIC_AR_4_3: > + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; > + break; > + case DRM_MODE_FLAG_PIC_AR_16_9: > + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; > + break; > + default: > + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > + break; > + } Hmm. So now we have the mode aspect ratio infromation duplicated in two different places. Not sure that won't lead to some confusion. Although we do want the user to be able to override via the property I suppose, so we'd have to change that (+ the inforframe code) to look at the mode flags instead if we would eliminate 'picture_aspect_ratio'. And that brings me to the other point. At least i915 will simply override the mode's aspect ration with the property. So this looks like a big no-op for now on i915. It's looking like we might need a new propetty value to differentiate between "auto" and "none" for real. > + > out->status = drm_mode_validate_basic(out); > if (out->status != MODE_OK) > goto out; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Regards Shashank On 10/17/2016 6:01 PM, Ville Syrjälä wrote: > On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote: >> Current DRM layer functions don't parse aspect ratio information >> while converting a user mode->kernel mode or vice versa. This >> causes modeset to pick mode with wrong aspect ratio, eventually >> causing failures in HDMI compliance test cases, due to wrong VIC. >> >> This patch adds aspect ratio information in DRM's mode conversion >> and mode comparision functions, to make sure kernel picks mode >> with right aspect ratio (as per the VIC). >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Lin, Jia <lin.a.jia@intel.com> >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> >> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> >> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Emil Velikov <emil.l.velikov@gmail.com> >> >> V2: Addressed review comments from Sean: >> - Fix spellings/typo >> - No need to handle aspect ratio none >> - Add a break, for default case too >> V3: Rebase >> V4: Added r-b from Jose >> --- >> drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >> index 53f07ac..fde927a 100644 >> --- a/drivers/gpu/drm/drm_modes.c >> +++ b/drivers/gpu/drm/drm_modes.c >> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, >> mode1->vsync_end == mode2->vsync_end && >> mode1->vtotal == mode2->vtotal && >> mode1->vscan == mode2->vscan && >> + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && >> (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == >> (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) >> return true; >> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, >> out->vrefresh = in->vrefresh; >> out->flags = in->flags; >> out->type = in->type; >> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; >> + >> + switch (in->picture_aspect_ratio) { >> + case HDMI_PICTURE_ASPECT_4_3: >> + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; >> + break; >> + case HDMI_PICTURE_ASPECT_16_9: >> + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; >> + break; >> + case HDMI_PICTURE_ASPECT_RESERVED: >> + default: >> + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; >> + break; >> + } >> + >> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); >> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; >> } >> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out, >> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); >> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; >> >> + /* Clearing picture aspect ratio bits from out flags */ >> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; >> + >> + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { >> + case DRM_MODE_FLAG_PIC_AR_4_3: >> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; >> + break; >> + case DRM_MODE_FLAG_PIC_AR_16_9: >> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; >> + break; >> + default: >> + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >> + break; >> + } > Hmm. So now we have the mode aspect ratio infromation duplicated > in two different places. Not sure that won't lead to some confusion. In drm layer, this is the only place. Actually till now, DRM layer dint have the support for aspect ratio at all. This was causing HDMI compliance tests like 7-27 fail, which expect a particular unique VIC in AVI infoframe on modeset. I have given some details about this in cover-letter. > Although we do want the user to be able to override via the property I > suppose, so we'd have to change that (+ the inforframe code) to > look at the mode flags instead if we would eliminate > 'picture_aspect_ratio'. I had a small discussion on this with Daniel, and we discussed that it doesnt make sense to override just the aspect ratio if the monitor doesn't even support that aspect ratio. And currently the was how this property is implemented is, we just override the aspect ratio without any validity check. Now as we have all the supported aspect ratio added properly in the mode info itself, we need not to have this property at all, So Daniel suggested me to remove that property too. > > And that brings me to the other point. At least i915 will simply > override the mode's aspect ration with the property. So this looks like > a big no-op for now on i915. Yes, This is a bug in I915. When I published the first version of this series, I had one patch, which was overriding the value only when the property is set. This should be the right case. And then Daniel suggested to remove the property all together (and it makes sense as we have proper aspect ratios in mode information itself) So I kept that patch separate, to be merged separately. > It's looking like we might need a new > propetty value to differentiate between "auto" and "none" for real. Now we have the exact aspect ratio given by monitor EDID, so IMHO it would be better if we can just have real aspect. Regards Shashank >> + >> out->status = drm_mode_validate_basic(out); >> if (out->status != MODE_OK) >> goto out; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 10/17/2016 6:01 PM, Ville Syrjälä wrote: > > On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote: > >> Current DRM layer functions don't parse aspect ratio information > >> while converting a user mode->kernel mode or vice versa. This > >> causes modeset to pick mode with wrong aspect ratio, eventually > >> causing failures in HDMI compliance test cases, due to wrong VIC. > >> > >> This patch adds aspect ratio information in DRM's mode conversion > >> and mode comparision functions, to make sure kernel picks mode > >> with right aspect ratio (as per the VIC). > >> > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> Signed-off-by: Lin, Jia <lin.a.jia@intel.com> > >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> > >> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> > >> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: Emil Velikov <emil.l.velikov@gmail.com> > >> > >> V2: Addressed review comments from Sean: > >> - Fix spellings/typo > >> - No need to handle aspect ratio none > >> - Add a break, for default case too > >> V3: Rebase > >> V4: Added r-b from Jose > >> --- > >> drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++ > >> 1 file changed, 31 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > >> index 53f07ac..fde927a 100644 > >> --- a/drivers/gpu/drm/drm_modes.c > >> +++ b/drivers/gpu/drm/drm_modes.c > >> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, > >> mode1->vsync_end == mode2->vsync_end && > >> mode1->vtotal == mode2->vtotal && > >> mode1->vscan == mode2->vscan && > >> + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && > >> (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == > >> (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) > >> return true; > >> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, > >> out->vrefresh = in->vrefresh; > >> out->flags = in->flags; > >> out->type = in->type; > >> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > >> + > >> + switch (in->picture_aspect_ratio) { > >> + case HDMI_PICTURE_ASPECT_4_3: > >> + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; > >> + break; > >> + case HDMI_PICTURE_ASPECT_16_9: > >> + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; > >> + break; > >> + case HDMI_PICTURE_ASPECT_RESERVED: > >> + default: > >> + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; > >> + break; > >> + } > >> + > >> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > >> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > >> } > >> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out, > >> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > >> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > >> > >> + /* Clearing picture aspect ratio bits from out flags */ > >> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > >> + > >> + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { > >> + case DRM_MODE_FLAG_PIC_AR_4_3: > >> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; > >> + break; > >> + case DRM_MODE_FLAG_PIC_AR_16_9: > >> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; > >> + break; > >> + default: > >> + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >> + break; > >> + } > > Hmm. So now we have the mode aspect ratio infromation duplicated > > in two different places. Not sure that won't lead to some confusion. > In drm layer, this is the only place. Actually till now, DRM layer dint > have the support for aspect ratio at all. This was causing > HDMI compliance tests like 7-27 fail, which expect a particular unique > VIC in AVI infoframe on modeset. > > I have given some details about this in cover-letter. > > Although we do want the user to be able to override via the property I > > suppose, so we'd have to change that (+ the inforframe code) to > > look at the mode flags instead if we would eliminate > > 'picture_aspect_ratio'. > I had a small discussion on this with Daniel, and we discussed that it > doesnt make sense to override just the aspect ratio if the monitor > doesn't even support that aspect ratio. > And currently the was how this property is implemented is, we just > override the aspect ratio without any validity check. > > Now as we have all the supported aspect ratio added properly in the mode > info itself, we need not to have this property at all, So Daniel > suggested me to remove that property too. > > > > And that brings me to the other point. At least i915 will simply > > override the mode's aspect ration with the property. So this looks like > > a big no-op for now on i915. > Yes, This is a bug in I915. When I published the first version of this > series, I had one patch, which was overriding the value only when the > property is set. > This should be the right case. And then Daniel suggested to remove the > property all together (and it makes sense as we have proper aspect > ratios in mode information > itself) So I kept that patch separate, to be merged separately. Yeah, removing the property entirely seems like an OKish solution since userspace can just stuff that information into the mode flags instead. The only slight concern is that someone's setup might depend on the property, and now we're removing it. > > It's looking like we might need a new > > propetty value to differentiate between "auto" and "none" for real. > Now we have the exact aspect ratio given by monitor EDID, so IMHO it > would be better if we can just have real aspect. Well, "real" isn't quite the right term. It's still a user specified thing, which means the user can ask for somehting the display didn't even advertise. Which is fine as such, however since the AVI infoframe lacks the capability to transmit these new aspect ratios we have a bit of problem on our hands if the user asks for something we can't even tell the display to do. I guess we would just need to return an error to the user in that case.
Regards Shashank On 10/17/2016 7:36 PM, Ville Syrjälä wrote: > On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 10/17/2016 6:01 PM, Ville Syrjälä wrote: >>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote: >>>> Current DRM layer functions don't parse aspect ratio information >>>> while converting a user mode->kernel mode or vice versa. This >>>> causes modeset to pick mode with wrong aspect ratio, eventually >>>> causing failures in HDMI compliance test cases, due to wrong VIC. >>>> >>>> This patch adds aspect ratio information in DRM's mode conversion >>>> and mode comparision functions, to make sure kernel picks mode >>>> with right aspect ratio (as per the VIC). >>>> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> Signed-off-by: Lin, Jia <lin.a.jia@intel.com> >>>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> >>>> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> >>>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Cc: Emil Velikov <emil.l.velikov@gmail.com> >>>> >>>> V2: Addressed review comments from Sean: >>>> - Fix spellings/typo >>>> - No need to handle aspect ratio none >>>> - Add a break, for default case too >>>> V3: Rebase >>>> V4: Added r-b from Jose >>>> --- >>>> drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++ >>>> 1 file changed, 31 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >>>> index 53f07ac..fde927a 100644 >>>> --- a/drivers/gpu/drm/drm_modes.c >>>> +++ b/drivers/gpu/drm/drm_modes.c >>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, >>>> mode1->vsync_end == mode2->vsync_end && >>>> mode1->vtotal == mode2->vtotal && >>>> mode1->vscan == mode2->vscan && >>>> + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && >>>> (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == >>>> (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) >>>> return true; >>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, >>>> out->vrefresh = in->vrefresh; >>>> out->flags = in->flags; >>>> out->type = in->type; >>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; >>>> + >>>> + switch (in->picture_aspect_ratio) { >>>> + case HDMI_PICTURE_ASPECT_4_3: >>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; >>>> + break; >>>> + case HDMI_PICTURE_ASPECT_16_9: >>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; >>>> + break; >>>> + case HDMI_PICTURE_ASPECT_RESERVED: >>>> + default: >>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; >>>> + break; >>>> + } >>>> + >>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); >>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; >>>> } >>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out, >>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); >>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; >>>> >>>> + /* Clearing picture aspect ratio bits from out flags */ >>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; >>>> + >>>> + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { >>>> + case DRM_MODE_FLAG_PIC_AR_4_3: >>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; >>>> + break; >>>> + case DRM_MODE_FLAG_PIC_AR_16_9: >>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; >>>> + break; >>>> + default: >>>> + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>> + break; >>>> + } >>> Hmm. So now we have the mode aspect ratio infromation duplicated >>> in two different places. Not sure that won't lead to some confusion. >> In drm layer, this is the only place. Actually till now, DRM layer dint >> have the support for aspect ratio at all. This was causing >> HDMI compliance tests like 7-27 fail, which expect a particular unique >> VIC in AVI infoframe on modeset. >> >> I have given some details about this in cover-letter. >>> Although we do want the user to be able to override via the property I >>> suppose, so we'd have to change that (+ the inforframe code) to >>> look at the mode flags instead if we would eliminate >>> 'picture_aspect_ratio'. >> I had a small discussion on this with Daniel, and we discussed that it >> doesnt make sense to override just the aspect ratio if the monitor >> doesn't even support that aspect ratio. >> And currently the was how this property is implemented is, we just >> override the aspect ratio without any validity check. >> >> Now as we have all the supported aspect ratio added properly in the mode >> info itself, we need not to have this property at all, So Daniel >> suggested me to remove that property too. >>> And that brings me to the other point. At least i915 will simply >>> override the mode's aspect ration with the property. So this looks like >>> a big no-op for now on i915. >> Yes, This is a bug in I915. When I published the first version of this >> series, I had one patch, which was overriding the value only when the >> property is set. >> This should be the right case. And then Daniel suggested to remove the >> property all together (and it makes sense as we have proper aspect >> ratios in mode information >> itself) So I kept that patch separate, to be merged separately. > Yeah, removing the property entirely seems like an OKish solution since > userspace can just stuff that information into the mode flags instead. > The only slight concern is that someone's setup might depend on the > property, and now we're removing it. Thanks, will send the patch for it soon, adding you in the mail chain. >>> It's looking like we might need a new >>> propetty value to differentiate between "auto" and "none" for real. >> Now we have the exact aspect ratio given by monitor EDID, so IMHO it >> would be better if we can just have real aspect. > Well, "real" isn't quite the right term. It's still a user specified > thing, which means the user can ask for somehting the display didn't > even advertise. Which is fine as such, however since the AVI infoframe > lacks the capability to transmit these new aspect ratios we have a bit > of problem on our hands if the user asks for something we can't even > tell the display to do. I guess we would just need to return an error > to the user in that case. The current implementation which we have in andoroid, is hardware composer shows the whole mode in the list like: - 1920x1080@60 16:9 /* From CEA block of EDID */ - 1920x1080@60 0:0 /* From detailed descriptor block of EDID */ - 1280x720@60 4:3 /* From CEA block of EDID */ -etc These mode is the connector->probed_modes which we populated while parsing the EDID. Now, when user selects the mode, its a complete set of resolution + refresh rate + aspect ratio, so user can only pick what we are showing to it. In this case, there is no option which we cant support. Do you think that there would be some other model of use case, where we can mix something among these? Regards Shashank When user picks mode, its picks
On Mon, Oct 17, 2016 at 08:21:21PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 10/17/2016 7:36 PM, Ville Syrjälä wrote: > > On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote: > >> Regards > >> > >> Shashank > >> > >> > >> On 10/17/2016 6:01 PM, Ville Syrjälä wrote: > >>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote: > >>>> Current DRM layer functions don't parse aspect ratio information > >>>> while converting a user mode->kernel mode or vice versa. This > >>>> causes modeset to pick mode with wrong aspect ratio, eventually > >>>> causing failures in HDMI compliance test cases, due to wrong VIC. > >>>> > >>>> This patch adds aspect ratio information in DRM's mode conversion > >>>> and mode comparision functions, to make sure kernel picks mode > >>>> with right aspect ratio (as per the VIC). > >>>> > >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>>> Signed-off-by: Lin, Jia <lin.a.jia@intel.com> > >>>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> > >>>> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> > >>>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> > >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>> Cc: Emil Velikov <emil.l.velikov@gmail.com> > >>>> > >>>> V2: Addressed review comments from Sean: > >>>> - Fix spellings/typo > >>>> - No need to handle aspect ratio none > >>>> - Add a break, for default case too > >>>> V3: Rebase > >>>> V4: Added r-b from Jose > >>>> --- > >>>> drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++ > >>>> 1 file changed, 31 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > >>>> index 53f07ac..fde927a 100644 > >>>> --- a/drivers/gpu/drm/drm_modes.c > >>>> +++ b/drivers/gpu/drm/drm_modes.c > >>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, > >>>> mode1->vsync_end == mode2->vsync_end && > >>>> mode1->vtotal == mode2->vtotal && > >>>> mode1->vscan == mode2->vscan && > >>>> + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && > >>>> (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == > >>>> (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) > >>>> return true; > >>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, > >>>> out->vrefresh = in->vrefresh; > >>>> out->flags = in->flags; > >>>> out->type = in->type; > >>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > >>>> + > >>>> + switch (in->picture_aspect_ratio) { > >>>> + case HDMI_PICTURE_ASPECT_4_3: > >>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; > >>>> + break; > >>>> + case HDMI_PICTURE_ASPECT_16_9: > >>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; > >>>> + break; > >>>> + case HDMI_PICTURE_ASPECT_RESERVED: > >>>> + default: > >>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; > >>>> + break; > >>>> + } > >>>> + > >>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > >>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > >>>> } > >>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out, > >>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > >>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > >>>> > >>>> + /* Clearing picture aspect ratio bits from out flags */ > >>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > >>>> + > >>>> + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { > >>>> + case DRM_MODE_FLAG_PIC_AR_4_3: > >>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; > >>>> + break; > >>>> + case DRM_MODE_FLAG_PIC_AR_16_9: > >>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; > >>>> + break; > >>>> + default: > >>>> + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >>>> + break; > >>>> + } > >>> Hmm. So now we have the mode aspect ratio infromation duplicated > >>> in two different places. Not sure that won't lead to some confusion. > >> In drm layer, this is the only place. Actually till now, DRM layer dint > >> have the support for aspect ratio at all. This was causing > >> HDMI compliance tests like 7-27 fail, which expect a particular unique > >> VIC in AVI infoframe on modeset. > >> > >> I have given some details about this in cover-letter. > >>> Although we do want the user to be able to override via the property I > >>> suppose, so we'd have to change that (+ the inforframe code) to > >>> look at the mode flags instead if we would eliminate > >>> 'picture_aspect_ratio'. > >> I had a small discussion on this with Daniel, and we discussed that it > >> doesnt make sense to override just the aspect ratio if the monitor > >> doesn't even support that aspect ratio. > >> And currently the was how this property is implemented is, we just > >> override the aspect ratio without any validity check. > >> > >> Now as we have all the supported aspect ratio added properly in the mode > >> info itself, we need not to have this property at all, So Daniel > >> suggested me to remove that property too. > >>> And that brings me to the other point. At least i915 will simply > >>> override the mode's aspect ration with the property. So this looks like > >>> a big no-op for now on i915. > >> Yes, This is a bug in I915. When I published the first version of this > >> series, I had one patch, which was overriding the value only when the > >> property is set. > >> This should be the right case. And then Daniel suggested to remove the > >> property all together (and it makes sense as we have proper aspect > >> ratios in mode information > >> itself) So I kept that patch separate, to be merged separately. > > Yeah, removing the property entirely seems like an OKish solution since > > userspace can just stuff that information into the mode flags instead. > > The only slight concern is that someone's setup might depend on the > > property, and now we're removing it. > Thanks, will send the patch for it soon, adding you in the mail chain. > >>> It's looking like we might need a new > >>> propetty value to differentiate between "auto" and "none" for real. > >> Now we have the exact aspect ratio given by monitor EDID, so IMHO it > >> would be better if we can just have real aspect. > > Well, "real" isn't quite the right term. It's still a user specified > > thing, which means the user can ask for somehting the display didn't > > even advertise. Which is fine as such, however since the AVI infoframe > > lacks the capability to transmit these new aspect ratios we have a bit > > of problem on our hands if the user asks for something we can't even > > tell the display to do. I guess we would just need to return an error > > to the user in that case. > The current implementation which we have in andoroid, is hardware > composer shows the whole mode in the list like: > - 1920x1080@60 16:9 /* From CEA block of EDID */ > - 1920x1080@60 0:0 /* From detailed descriptor block of EDID */ > - 1280x720@60 4:3 /* From CEA block of EDID */ > -etc > These mode is the connector->probed_modes which we populated while > parsing the EDID. > Now, when user selects the mode, its a complete set of resolution + > refresh rate + aspect ratio, so user can only pick what > we are showing to it. In this case, there is no option which we cant > support. > > Do you think that there would be some other model of use case, where we > can mix something among these? You can't assume the user just picks the mode off the list. They can stuff whatever random garbage into the mode if they wish.
Regards Shashank On 10/17/2016 8:30 PM, Ville Syrjälä wrote: > On Mon, Oct 17, 2016 at 08:21:21PM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 10/17/2016 7:36 PM, Ville Syrjälä wrote: >>> On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote: >>>> Regards >>>> >>>> Shashank >>>> >>>> >>>> On 10/17/2016 6:01 PM, Ville Syrjälä wrote: >>>>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote: >>>>>> Current DRM layer functions don't parse aspect ratio information >>>>>> while converting a user mode->kernel mode or vice versa. This >>>>>> causes modeset to pick mode with wrong aspect ratio, eventually >>>>>> causing failures in HDMI compliance test cases, due to wrong VIC. >>>>>> >>>>>> This patch adds aspect ratio information in DRM's mode conversion >>>>>> and mode comparision functions, to make sure kernel picks mode >>>>>> with right aspect ratio (as per the VIC). >>>>>> >>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>>>> Signed-off-by: Lin, Jia <lin.a.jia@intel.com> >>>>>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> >>>>>> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> >>>>>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> >>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com> >>>>>> >>>>>> V2: Addressed review comments from Sean: >>>>>> - Fix spellings/typo >>>>>> - No need to handle aspect ratio none >>>>>> - Add a break, for default case too >>>>>> V3: Rebase >>>>>> V4: Added r-b from Jose >>>>>> --- >>>>>> drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 31 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >>>>>> index 53f07ac..fde927a 100644 >>>>>> --- a/drivers/gpu/drm/drm_modes.c >>>>>> +++ b/drivers/gpu/drm/drm_modes.c >>>>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, >>>>>> mode1->vsync_end == mode2->vsync_end && >>>>>> mode1->vtotal == mode2->vtotal && >>>>>> mode1->vscan == mode2->vscan && >>>>>> + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && >>>>>> (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == >>>>>> (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) >>>>>> return true; >>>>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, >>>>>> out->vrefresh = in->vrefresh; >>>>>> out->flags = in->flags; >>>>>> out->type = in->type; >>>>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; >>>>>> + >>>>>> + switch (in->picture_aspect_ratio) { >>>>>> + case HDMI_PICTURE_ASPECT_4_3: >>>>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; >>>>>> + break; >>>>>> + case HDMI_PICTURE_ASPECT_16_9: >>>>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; >>>>>> + break; >>>>>> + case HDMI_PICTURE_ASPECT_RESERVED: >>>>>> + default: >>>>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); >>>>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; >>>>>> } >>>>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out, >>>>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); >>>>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0; >>>>>> >>>>>> + /* Clearing picture aspect ratio bits from out flags */ >>>>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; >>>>>> + >>>>>> + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { >>>>>> + case DRM_MODE_FLAG_PIC_AR_4_3: >>>>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; >>>>>> + break; >>>>>> + case DRM_MODE_FLAG_PIC_AR_16_9: >>>>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; >>>>>> + break; >>>>>> + default: >>>>>> + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>>>> + break; >>>>>> + } >>>>> Hmm. So now we have the mode aspect ratio infromation duplicated >>>>> in two different places. Not sure that won't lead to some confusion. >>>> In drm layer, this is the only place. Actually till now, DRM layer dint >>>> have the support for aspect ratio at all. This was causing >>>> HDMI compliance tests like 7-27 fail, which expect a particular unique >>>> VIC in AVI infoframe on modeset. >>>> >>>> I have given some details about this in cover-letter. >>>>> Although we do want the user to be able to override via the property I >>>>> suppose, so we'd have to change that (+ the inforframe code) to >>>>> look at the mode flags instead if we would eliminate >>>>> 'picture_aspect_ratio'. >>>> I had a small discussion on this with Daniel, and we discussed that it >>>> doesnt make sense to override just the aspect ratio if the monitor >>>> doesn't even support that aspect ratio. >>>> And currently the was how this property is implemented is, we just >>>> override the aspect ratio without any validity check. >>>> >>>> Now as we have all the supported aspect ratio added properly in the mode >>>> info itself, we need not to have this property at all, So Daniel >>>> suggested me to remove that property too. >>>>> And that brings me to the other point. At least i915 will simply >>>>> override the mode's aspect ration with the property. So this looks like >>>>> a big no-op for now on i915. >>>> Yes, This is a bug in I915. When I published the first version of this >>>> series, I had one patch, which was overriding the value only when the >>>> property is set. >>>> This should be the right case. And then Daniel suggested to remove the >>>> property all together (and it makes sense as we have proper aspect >>>> ratios in mode information >>>> itself) So I kept that patch separate, to be merged separately. >>> Yeah, removing the property entirely seems like an OKish solution since >>> userspace can just stuff that information into the mode flags instead. >>> The only slight concern is that someone's setup might depend on the >>> property, and now we're removing it. >> Thanks, will send the patch for it soon, adding you in the mail chain. >>>>> It's looking like we might need a new >>>>> propetty value to differentiate between "auto" and "none" for real. >>>> Now we have the exact aspect ratio given by monitor EDID, so IMHO it >>>> would be better if we can just have real aspect. >>> Well, "real" isn't quite the right term. It's still a user specified >>> thing, which means the user can ask for somehting the display didn't >>> even advertise. Which is fine as such, however since the AVI infoframe >>> lacks the capability to transmit these new aspect ratios we have a bit >>> of problem on our hands if the user asks for something we can't even >>> tell the display to do. I guess we would just need to return an error >>> to the user in that case. >> The current implementation which we have in andoroid, is hardware >> composer shows the whole mode in the list like: >> - 1920x1080@60 16:9 /* From CEA block of EDID */ >> - 1920x1080@60 0:0 /* From detailed descriptor block of EDID */ >> - 1280x720@60 4:3 /* From CEA block of EDID */ >> -etc >> These mode is the connector->probed_modes which we populated while >> parsing the EDID. >> Now, when user selects the mode, its a complete set of resolution + >> refresh rate + aspect ratio, so user can only pick what >> we are showing to it. In this case, there is no option which we cant >> support. >> >> Do you think that there would be some other model of use case, where we >> can mix something among these? > You can't assume the user just picks the mode off the list. They can > stuff whatever random garbage into the mode if they wish. > I agree, there can be a use case model. Regards Shashank
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 53f07ac..fde927a 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, mode1->vsync_end == mode2->vsync_end && mode1->vtotal == mode2->vtotal && mode1->vscan == mode2->vscan && + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) return true; @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, out->vrefresh = in->vrefresh; out->flags = in->flags; out->type = in->type; + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; + + switch (in->picture_aspect_ratio) { + case HDMI_PICTURE_ASPECT_4_3: + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; + break; + case HDMI_PICTURE_ASPECT_16_9: + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; + break; + case HDMI_PICTURE_ASPECT_RESERVED: + default: + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; + break; + } + strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); out->name[DRM_DISPLAY_MODE_LEN-1] = 0; } @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out, strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); out->name[DRM_DISPLAY_MODE_LEN-1] = 0; + /* Clearing picture aspect ratio bits from out flags */ + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; + + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { + case DRM_MODE_FLAG_PIC_AR_4_3: + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; + break; + case DRM_MODE_FLAG_PIC_AR_16_9: + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; + break; + default: + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; + break; + } + out->status = drm_mode_validate_basic(out); if (out->status != MODE_OK) goto out;