Hi Maxime,
First of all, nice idea with the helper function that can be reused by different
drivers. This is neat!
But looking at this function, it feels a bit overcomplicated. You're creating
the two modes, then checking which one is the default, then set the preferred
one and possibly reorder them. Maybe it can be simplified somehow?
Although when I tried to refactor it myself, I ended up with something that's
not better at all. Maybe it needs to be complicated, after all :(
Anyway, the current version seems to have a couple of bugs:
> + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) {
> + mode = drm_mode_analog_pal_576i(connector->dev);
> + if (!mode)
> + return 0;
> +
> + tv_modes[count++] = mode;
> + }
If the 480i mode has been created properly, but there's an error creating the
576i one (we enter the if (!mode) clause), the 480i mode will leak.
> + if (count == 1) {
You're handling the count == 1 case specially, but if count == 0, the rest of
the code will assume that two modes exist and probably segfault in the process.
> + ret = drm_object_property_get_default_value(&connector->base,
> + dev->mode_config.tv_mode_property,
> + &default_mode);
> + if (ret)
> + return 0;
> +
> + if (cmdline->tv_mode_specified)
> + default_mode = cmdline->tv_mode;
In case of an error (ret != 0), the modes created so far in the tv_modes array
will leak.
Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go
first? If we're going to use the default from cmdline, there's no point in even
querying the property default value.
Best regards,
Mateusz Kwiatkowski
Hi Mateusz,
On Thu, Oct 27, 2022 at 12:02:24AM +0200, Mateusz Kwiatkowski wrote:
> First of all, nice idea with the helper function that can be reused by
> different drivers. This is neat!
Yeah, it looked to me that given how complex it is, we don't want to
duplicate it in each and every driver.
> But looking at this function, it feels a bit overcomplicated. You're
> creating the two modes,
If reported as supported by the connector, yes.
> then checking which one is the default, then set the preferred one and
> possibly reorder them. Maybe it can be simplified somehow?
Possibly, but I couldn't find something simpler. We should only expose
the modes that the driver reports as supported, so we can have 0-2
modes. Then the preferred flag needs to be set on the default one like
you suggested.
But also, EDIDs define the preferred mode as either the mode with the
flag set or the first mode listed. So a lot of program just use the
heuristic to just pick the first mode listed.
So it might be that I'm too careful, but it still seems useful to me.
> Although when I tried to refactor it myself, I ended up with something that's
> not better at all. Maybe it needs to be complicated, after all :(
Yeah, that was my conclusion too :/
> Anyway, the current version seems to have a couple of bugs:
>
> > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) ||
> > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) ||
> > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) {
> > + mode = drm_mode_analog_pal_576i(connector->dev);
> > + if (!mode)
> > + return 0;
> > +
> > + tv_modes[count++] = mode;
> > + }
>
> If the 480i mode has been created properly, but there's an error creating the
> 576i one (we enter the if (!mode) clause), the 480i mode will leak.
>
> > + if (count == 1) {
>
> You're handling the count == 1 case specially, but if count == 0, the rest of
> the code will assume that two modes exist and probably segfault in the process.
>
> > + ret = drm_object_property_get_default_value(&connector->base,
> > + dev->mode_config.tv_mode_property,
> > + &default_mode);
> > + if (ret)
> > + return 0;
> > +
> > + if (cmdline->tv_mode_specified)
> > + default_mode = cmdline->tv_mode;
>
> In case of an error (ret != 0), the modes created so far in the tv_modes array
> will leak.
Thanks for the review, I'll fix these bugs
> Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go
> first? If we're going to use the default from cmdline, there's no point in even
> querying the property default value.
Maybe, I don't know. I find the flow of the code more readable that way,
but if you disagree I'll change it.
Maxime
Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski:
> Hi Maxime,
>
> First of all, nice idea with the helper function that can be reused by different
> drivers. This is neat!
>
> But looking at this function, it feels a bit overcomplicated. You're creating
> the two modes, then checking which one is the default, then set the preferred
> one and possibly reorder them. Maybe it can be simplified somehow?
>
> Although when I tried to refactor it myself, I ended up with something that's
> not better at all. Maybe it needs to be complicated, after all :(
>
I also thought that the function was complicated/difficult to read, in
particular the index stuff at the end, but I also failed in finding a
"better" solution, just a different one ;)
Noralf.
My version:
int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
struct drm_property *tv_mode_property = dev->mode_config.tv_mode_property;
struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
unsigned int ntsc_modes = BIT(DRM_MODE_TV_MODE_NTSC) |
BIT(DRM_MODE_TV_MODE_NTSC_443) |
BIT(DRM_MODE_TV_MODE_NTSC_J) |
BIT(DRM_MODE_TV_MODE_PAL_M);
unsigned int pal_modes = BIT(DRM_MODE_TV_MODE_PAL) |
BIT(DRM_MODE_TV_MODE_PAL_N) |
BIT(DRM_MODE_TV_MODE_SECAM);
unsigned int tv_modes[2] = { UINT_MAX, UINT_MAX };
unsigned int i, supported_tv_modes = 0;
if (!tv_mode_property)
return 0;
for (i = 0; i < tv_mode_property->num_values; i++)
supported_tv_modes |= BIT(tv_mode_property->values[i]);
if ((supported_tv_modes & ntsc_modes) && (supported_tv_modes &
pal_modes)) {
uint64_t default_mode;
if (drm_object_property_get_default_value(&connector->base,
tv_mode_property,
&default_mode))
return 0;
if (cmdline->tv_mode_specified)
default_mode = cmdline->tv_mode;
if (BIT(default_mode) & ntsc_modes) {
tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
tv_modes[1] = DRM_MODE_TV_MODE_PAL;
} else {
tv_modes[0] = DRM_MODE_TV_MODE_PAL;
tv_modes[1] = DRM_MODE_TV_MODE_NTSC;
}
} else if (supported_tv_modes & ntsc_modes) {
tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
} else if (supported_tv_modes & pal_modes) {
tv_modes[0] = DRM_MODE_TV_MODE_PAL;
} else {
return 0;
}
for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
struct drm_display_mode *mode;
if (tv_modes[i] == DRM_MODE_TV_MODE_NTSC)
mode = drm_mode_analog_ntsc_480i(dev);
else if (tv_modes[i] == DRM_MODE_TV_MODE_PAL)
mode = drm_mode_analog_pal_576i(dev);
else
break;
if (!mode)
return i;
if (!i)
mode->type |= DRM_MODE_TYPE_PREFERRED;
drm_mode_probed_add(connector, mode);
}
return i;
}
Hi Noralf,
On Sun, Nov 06, 2022 at 05:59:23PM +0100, Noralf Tr?nnes wrote:
>
>
> Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski:
> > Hi Maxime,
> >
> > First of all, nice idea with the helper function that can be reused by different
> > drivers. This is neat!
> >
> > But looking at this function, it feels a bit overcomplicated. You're creating
> > the two modes, then checking which one is the default, then set the preferred
> > one and possibly reorder them. Maybe it can be simplified somehow?
> >
> > Although when I tried to refactor it myself, I ended up with something that's
> > not better at all. Maybe it needs to be complicated, after all :(
> >
>
> I also thought that the function was complicated/difficult to read, in
> particular the index stuff at the end, but I also failed in finding a
> "better" solution, just a different one ;)
I think I like yours better still :)
Can I bring it into my series, with your authorship and SoB?
Maxime
Den 07.11.2022 11.07, skrev Maxime Ripard:
> Hi Noralf,
>
> On Sun, Nov 06, 2022 at 05:59:23PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski:
>>> Hi Maxime,
>>>
>>> First of all, nice idea with the helper function that can be reused by different
>>> drivers. This is neat!
>>>
>>> But looking at this function, it feels a bit overcomplicated. You're creating
>>> the two modes, then checking which one is the default, then set the preferred
>>> one and possibly reorder them. Maybe it can be simplified somehow?
>>>
>>> Although when I tried to refactor it myself, I ended up with something that's
>>> not better at all. Maybe it needs to be complicated, after all :(
>>>
>>
>> I also thought that the function was complicated/difficult to read, in
>> particular the index stuff at the end, but I also failed in finding a
>> "better" solution, just a different one ;)
>
> I think I like yours better still :)
>
> Can I bring it into my series, with your authorship and SoB?
>
Sure, no problem.
Noralf.